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.

Sign up for GitHub

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

Jump to bottom

Fix panic on iframe attach #1406

Merged
merged 4 commits into from
Aug 21, 2024
Merged

Fix panic on iframe attach #1406

merged 4 commits into from
Aug 21, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Aug 20, 2024

What?

This PR fixes an issue where the browser module would panic when navigating to certain website with iframes on them. The error is panic: GoError: attaching iframe: attaching iframe target ID 153A0D279584840F49FB1E718A8297AB to session ID DA84F3DFA9B0CA220314557F25BC8B37: getting browser window ID: No web contents for the given target id (-32000)

Why?

The reason for this is when the browser comes to attach an iframe, some iframes do not have any UI elements, which sets a state within chromium such that no windowID can be returned when one is requested for. This causes a panic here. To avoid this issue we have to avoid performing the CDP calls that could cause errors when attaching a new iframe (retrieving a windowID and set the window bounds). PW does the same.

Test

Test procedure

You can test against the following page:

<!DOCTYPE html>
<html>
<head>
    <script src="https://js.stripe.com/v3/"></script>
</head>
<body></body>
</html>

First we need to replicate the issue, so set the current branch to main, and run the following test script (multiple times) until you get the same error as mentioned above:

import { browser } from 'k6/browser';

export const options = {
  scenarios: {
    ui: {
      executor: 'shared-iterations',
      iterations: 1,
      options: {
        browser: {
          type: 'chromium'
        },
      },
    },
  },
}

export default async function () {
  const page = await browser.newPage();

  // You can try against http://2022wfh.ddns.net/static/tmp/panic.html
  await page.goto("URL for test site", {waitUntil:'networkidle'});

  await page.close();
}

Now test the same script against the fix branch. The same error shouldn't appear again.

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: #1224

ankur22 added 3 commits August 20, 2024 13:54
@ankur22
Refactor windowID to pointer
b150ad5 
When working with iframes, there are some cases where the frame that
contains the iframe doesn't have a windowID. Trying to retrieve one
will result in an error. Changing windowID to a pointer will enable us
to either have one or not.
@ankur22
Update NewFrameSession to take hasUIWindow flag
72d2819 
When we create a new FrameSession for a frame associated with a frame,
we don't want to retrieve the windowID as it could cause chromium to
error and therefore the browser module to panic. Instead we will avoid
calling out to retrieve the windowID in those cases.
@ankur22
Use hasUIWindow to avoid getting windowID
0d6514a 
Now we can avoid the call to retrieve windowID and work with it if the
windowID is nil. This change will avoid panics when working with
certain iframes that do not contain UI elements, and therefore chromium
doesn't track a windowID for it.
@ankur22 ankur22 requested a review from inancgumus August 20, 2024 15:54
inancgumus
inancgumus reviewed Aug 20, 2024
View reviewed changes
common/frame_session.go Outdated Show resolved Hide resolved
inancgumus
inancgumus reviewed Aug 20, 2024
View reviewed changes
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.

Glad we're fixing this. I have a question.

@ankur22 ankur22 mentioned this pull request Aug 20, 2024
Closed
@ankur22
Add hasUIWindow as field on FrameSession
ed17d77 
This makes it more explicit as to what is going on when windowID has
not been retrieved and not used later on.

This change also means we can revert windowID back to a non-pointer
field.
@ankur22 ankur22 mentioned this pull request Aug 21, 2024
Merged
5 tasks
inancgumus
inancgumus approved these changes Aug 21, 2024
View reviewed changes
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! 🎉

@ankur22 ankur22 merged commit a343afb into main Aug 21, 2024
23 checks passed
@ankur22 ankur22 deleted the fix/1224-panic-iframe-attach branch August 21, 2024 14:15
@BrewTestBot BrewTestBot mentioned this pull request Sep 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inancgumus inancgumus inancgumus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ankur22 @inancgumus