Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix iframe panic on navigation #1346

Merged
merged 3 commits into from
May 24, 2024
Merged

Fix iframe panic on navigation #1346

merged 3 commits into from
May 24, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented May 24, 2024

What?

This fixes an issue where a page with an iframe panics during a navigation.

Why?

The reason are two fold:

  1. Firstly we didn't have a reference to the iframes child frame. This was resolved by adding handling the frame tree of the iframe. It's not a mainframe but it still needs to be handled correctly so that we have a reference to the iframe and its children.
  2. Now that we have the child frame, we don't want it to be removed during a detach (of type swap). We protect against this by checking the session of the incoming detach event and the frame that it is trying to detach. If they don't match we ignore the request.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates: #1341

handleFrameTree is called when a new frame session is created for a new
frame that is attached. Some of these frames are not mainframes but do
contains a tree structures with child frames. If we don't handle the
frame tree for these non-mainframes, this will prevent navigations from
being handled for these frames and a panic/error which would stop the
test iteration.
The only reason we have initialFrame flag is to use in logging, so it's
not an important thing to factor in for the runtime, but it may help in
debugging issues. Rather than remove it, I've changed it so any
mainframe is regarded as the initialFrame.
The change prevents the iframe child frames from being removed when a
detach event comes in from Chrome.

An iframe seems to be associated to multiple sessions. The initial
sessions that is linked to the mainframe that the iframe belongs to and
its own session. When the page navigates, a detach event arrives with
the initial session and a swap type for the iframe. The iframe though
is associated to its own session. In this scenario we want to avoid
removing frames, which prevents panics occurring when the iframe
navigates.
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix and your explanations make total sense 🚀

Since we know the cause of the issue, can we add a test to reproduce this and prevent regressions?

@ankur22
Copy link
Collaborator Author

ankur22 commented May 24, 2024

Since we know the cause of the issue, can we add a test to reproduce this and prevent regressions?

I'm working on the test, but it's difficult one.

@ankur22 ankur22 merged commit 6881602 into main May 24, 2024
17 checks passed
@ankur22 ankur22 deleted the fix/1341-iframe-detach branch May 24, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants