Fix execution context setting for iframes #1254

ankur22 · 2024-03-21T17:29:14Z

What?

This fixes a problem with setting the execution context when testing a website that uses iframes. Without this fix it could end up in an infinite loop when waitForExecutionContext is called.

Why?

There is a race condition when it comes to attaching iframes and the execution context that apply to these frames. What usually occurs is:

An exec context for about:blank is first set;
A new set event is received for exec context for the url pointing to the actual destination for the iframe;
Finally the execution context for about:blank is destroyed, not for the second execution context.

This is the order of events when iframes are in use on a site, and so it is safe to nil the original execution context and overwrite it with the second one.

The exec context destroyed event will not remove the new exec context since the ids do not match.

If we didn't overwrite the first execCtx with the new one, then waitForExecutionContext could end up waiting indefinitely since all execCtx were destroyed.

Reference: Playwright

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

Since this fixes an interaction with iframes, we now see more panic: GoError: dispose: canceled errors, which there is an open issue for: #1089.

common/frame.go

inancgumus

Unfortunately, I still see the same error (after running the user's script with this branch) 😭 The reason you might not see the error is probably because of a race condition since the script uses a lot of sleeps.

ankur22 · 2024-04-25T08:55:07Z

Unfortunately, I still see the same error (after running the user's script with this branch) 😭 The reason you might not see the error is probably because of a race condition since the script uses a lot of sleeps.

Yep, you are correct, the same error that was posted in the original post still occurs 😢, when ran in headless mode:

panic: GoError: dispose: disposing element with ID -28511178728990825.29.1: Cannot find context with specified id (-32000)

However this same issue doesn't occur in headful mode with:

K6_BROWSER_HEADLESS=false k6 run e2e/test.js

Without this fix, in headful mode, the test ends up in an infinite loop when waitForExecutionContext is called. I think it's still a valid fix, but I will need to go back to the post and correct my assumption. I'll then look at resolving the original error 👍

inancgumus · 2024-04-25T11:51:24Z

However this same issue doesn't occur in headful mode with:
K6_BROWSER_HEADLESS=false k6 run e2e/test.js
Without this fix, in headful mode, the test ends up in an infinite loop when waitForExecutionContext is called. I think it's still a valid fix, but I will need to go back to the post and correct my assumption. I'll then look at resolving the original error 👍

On the main branch, VUs get stuck after entering the email address. Yeah, this definitely fixes the "infinite loop" issue in headful mode 👏

On this fix's branch, when I run this fix in headful mode, I receive the following error. I also noticed that the Chromium processes didn't shut down.

panic: GoError: dispose: canceled

goroutine 403 [running]:
go.k6.io/k6/js/common.Throw(...)
        go.k6.io/k6@v0.50.0/js/common/util.go:20
github.com/grafana/xk6-browser/k6ext.Panic.func1(0x140028ee008, {0x14005e6cdb0?, 0x10220d601?, 0x14004670f50?})
        github.com/grafana/xk6-browser@v1.4.3/k6ext/panic.go:35 +0x7c
github.com/grafana/xk6-browser/k6ext.sharedPanic({0x102511488, 0x14006e26fa0}, 0x14006df5a40, {0x14005e6cdb0, 0x1, 0x1})
        github.com/grafana/xk6-browser@v1.4.3/k6ext/panic.go:64 +0x20c
github.com/grafana/xk6-browser/k6ext.Panic({0x102511488?, 0x14006e26fa0?}, {0x101d97046?, 0x102222420?}, {0x14005e6cdb0?, 0x102222420?, 0x140067e6660?})
        github.com/grafana/xk6-browser@v1.4.3/k6ext/panic.go:37 +0x58
github.com/grafana/xk6-browser/common.(*BaseJSHandle).Dispose(0x1400896d590)
        github.com/grafana/xk6-browser@v1.4.3/common/js_handle.go:83 +0xa0
github.com/grafana/xk6-browser/common.(*Frame).detach(0x1400e63a540)
        github.com/grafana/xk6-browser@v1.4.3/common/frame.go:243 +0x100
github.com/grafana/xk6-browser/common.(*FrameManager).removeFramesRecursively(0x14007104300, 0x1400e63a540)
        github.com/grafana/xk6-browser@v1.4.3/common/frame_manager.go:419 +0x1a8
github.com/grafana/xk6-browser/common.(*FrameManager).frameDetached(0x14007104300, {0x14004215aa0, 0x20}, {0x101d8eb21, 0x6})
        github.com/grafana/xk6-browser@v1.4.3/common/frame_manager.go:185 +0x110
github.com/grafana/xk6-browser/common.(*FrameSession).onFrameDetached(0x1400713c160, {0x14004215aa0, 0x20}, {0x101d8eb21, 0x6})
        github.com/grafana/xk6-browser@v1.4.3/common/frame_session.go:748 +0x11c
github.com/grafana/xk6-browser/common.(*FrameSession).initEvents.func1()
        github.com/grafana/xk6-browser@v1.4.3/common/frame_session.go:254 +0x664
created by github.com/grafana/xk6-browser/common.(*FrameSession).initEvents in goroutine 187
        github.com/grafana/xk6-browser@v1.4.3/common/frame_session.go:220 +0x180

What do you suggest about the next steps for issue #1252? I mean we can merge this PR, but keep the issue open. Or?

ankur22 · 2024-04-25T11:58:42Z

On this fix's branch, when I run this fix in headful mode, I receive the following error. I also noticed that the Chromium processes didn't shut down.

panic: GoError: dispose: canceled

goroutine 403 [running]:
go.k6.io/k6/js/common.Throw(...)
        go.k6.io/k6@v0.50.0/js/common/util.go:20
github.com/grafana/xk6-browser/k6ext.Panic.func1(0x140028ee008, {0x14005e6cdb0?, 0x10220d601?, 0x14004670f50?})
        github.com/grafana/xk6-browser@v1.4.3/k6ext/panic.go:35 +0x7c
github.com/grafana/xk6-browser/k6ext.sharedPanic({0x102511488, 0x14006e26fa0}, 0x14006df5a40, {0x14005e6cdb0, 0x1, 0x1})
        github.com/grafana/xk6-browser@v1.4.3/k6ext/panic.go:64 +0x20c
github.com/grafana/xk6-browser/k6ext.Panic({0x102511488?, 0x14006e26fa0?}, {0x101d97046?, 0x102222420?}, {0x14005e6cdb0?, 0x102222420?, 0x140067e6660?})
        github.com/grafana/xk6-browser@v1.4.3/k6ext/panic.go:37 +0x58
github.com/grafana/xk6-browser/common.(*BaseJSHandle).Dispose(0x1400896d590)
        github.com/grafana/xk6-browser@v1.4.3/common/js_handle.go:83 +0xa0
github.com/grafana/xk6-browser/common.(*Frame).detach(0x1400e63a540)
        github.com/grafana/xk6-browser@v1.4.3/common/frame.go:243 +0x100
github.com/grafana/xk6-browser/common.(*FrameManager).removeFramesRecursively(0x14007104300, 0x1400e63a540)
        github.com/grafana/xk6-browser@v1.4.3/common/frame_manager.go:419 +0x1a8
github.com/grafana/xk6-browser/common.(*FrameManager).frameDetached(0x14007104300, {0x14004215aa0, 0x20}, {0x101d8eb21, 0x6})
        github.com/grafana/xk6-browser@v1.4.3/common/frame_manager.go:185 +0x110
github.com/grafana/xk6-browser/common.(*FrameSession).onFrameDetached(0x1400713c160, {0x14004215aa0, 0x20}, {0x101d8eb21, 0x6})
        github.com/grafana/xk6-browser@v1.4.3/common/frame_session.go:748 +0x11c
github.com/grafana/xk6-browser/common.(*FrameSession).initEvents.func1()
        github.com/grafana/xk6-browser@v1.4.3/common/frame_session.go:254 +0x664
created by github.com/grafana/xk6-browser/common.(*FrameSession).initEvents in goroutine 187
        github.com/grafana/xk6-browser@v1.4.3/common/frame_session.go:220 +0x180

This shouldn't happen since the fix was merged into main and main was rebased to this fix branch. Looking at the stack trace it seems that you may be running an older version of the branch?

> github.com/grafana/xk6-browser/common.(*BaseJSHandle).Dispose(0x1400896d590)
>         github.com/grafana/xk6-browser@v1.4.3/common/js_handle.go:83 +0xa0

In the latest version this points to a comment and not to a call to panic. Do you mind rebasing/pulling and trying again?

inancgumus · 2024-04-25T13:22:57Z

🤦 You're right. It somehow cached the version. I should have checked the versions in the stack trace. Apologies for the noise. WDYT about this since the fix only applies to the headful version?

What do you suggest about the next steps for the issue #1252? I mean, we can merge this PR but keep the issue open. Or?

inancgumus

Nice catch!

ankur22 · 2024-04-25T13:29:10Z

🤦 You're right. It somehow cached the version. I should have checked the versions in the stack trace. Apologies for the noise. WDYT about this?

🙂 👍

What do you suggest about the next steps for the issue #1252? I mean, we can merge this PR but keep the issue open. Or?

I believe that this PR does fix the issue it is attached to (#1252), and so we can close that issue once this PR is merged.

Unfortunately it doesn't fix the original issue from the community post, but there is an open issue for that already #1144. I plan to work on that next.

It doesn't make sense for this operation to continue when the frame is not present, so return early. If the frame is present continue as normal.

The events are as follows when an iframe is attached: 1. execCtx for about:blank for new frame is set; 2. execCtx for navigated frame with url is set; 3. original about:blank execCtx is destroyed. If we only work with the original execCtx, then it will be destroyed by the 3rd event. To prevent this we overwrite the original one with the new one in the 2nd event. What this means is that we do not end up in an infinite loop when waitForExecutionContext is called.

ankur22 marked this pull request as draft March 21, 2024 17:29

ankur22 commented Mar 21, 2024

View reviewed changes

common/frame.go Show resolved Hide resolved

ankur22 mentioned this pull request Mar 21, 2024

frame.waitForSelector waits indefinitely #1252

Closed

ankur22 marked this pull request as ready for review March 27, 2024 13:31

ankur22 requested a review from inancgumus March 27, 2024 13:31

ankur22 marked this pull request as draft March 27, 2024 17:08

ankur22 mentioned this pull request Apr 5, 2024

Dispose panic after clicking on an element inside another frame #1089

Closed

ankur22 marked this pull request as ready for review April 5, 2024 16:34

ankur22 force-pushed the fix/exec-context-session branch from b398508 to 3405f11 Compare April 5, 2024 16:34

inancgumus reviewed Apr 15, 2024

View reviewed changes

ankur22 force-pushed the fix/exec-context-session branch 2 times, most recently from a752709 to 349c9e0 Compare April 25, 2024 08:48

inancgumus approved these changes Apr 25, 2024

View reviewed changes

ankur22 added 2 commits April 25, 2024 14:32

Fix possible NPD

200c717

It doesn't make sense for this operation to continue when the frame is not present, so return early. If the frame is present continue as normal.

ankur22 force-pushed the fix/exec-context-session branch from 349c9e0 to d02f88d Compare April 25, 2024 13:32

ankur22 merged commit 64c825d into main Apr 25, 2024
17 checks passed

ankur22 deleted the fix/exec-context-session branch April 25, 2024 13:39

BrewTestBot mentioned this pull request May 13, 2024

k6 0.51.0 Homebrew/homebrew-core#171570

Merged

inancgumus mentioned this pull request Jul 23, 2024

Panic on attaching iframe when getting browser window ID #1224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix execution context setting for iframes #1254

Fix execution context setting for iframes #1254

ankur22 commented Mar 21, 2024 •

edited

Loading

inancgumus left a comment •

edited

Loading

ankur22 commented Apr 25, 2024

inancgumus commented Apr 25, 2024 •

edited

Loading

ankur22 commented Apr 25, 2024

inancgumus commented Apr 25, 2024 •

edited

Loading

inancgumus left a comment

ankur22 commented Apr 25, 2024

Fix execution context setting for iframes #1254

Fix execution context setting for iframes #1254

Conversation

ankur22 commented Mar 21, 2024 • edited Loading

What?

Why?

Checklist

Related PR(s)/Issue(s)

inancgumus left a comment • edited Loading

Choose a reason for hiding this comment

ankur22 commented Apr 25, 2024

inancgumus commented Apr 25, 2024 • edited Loading

ankur22 commented Apr 25, 2024

inancgumus commented Apr 25, 2024 • edited Loading

inancgumus left a comment

Choose a reason for hiding this comment

ankur22 commented Apr 25, 2024

ankur22 commented Mar 21, 2024 •

edited

Loading

inancgumus left a comment •

edited

Loading

inancgumus commented Apr 25, 2024 •

edited

Loading

inancgumus commented Apr 25, 2024 •

edited

Loading