1985997 - Ensure preload scripts are executed before handling statements following a window.open call

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Scripts as installed via script.addPreloadScript are not getting applied to new windows that are opened via script evaluation by calling window.open().

This can be reproduced by the following Playwright test: tests/library/browsercontext-add-init-script.spec.ts :: should work without navigation in popup.

Note that the page as created via context.newPage() will get the preload scripts applied but not the popup window. The test works as well when a navigation takes place and navigates away from about:blank.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually it happens as well after a navigation when this is very short. There seem to be some kind of delay until the preload settings are getting applied.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Holger Benl [:hbenl]]

There seem to be some kind of delay until the preload settings are getting applied.

Note that Playwright expects preload scripts to run synchronously in window.open(): This test exposes a binding (which is implemented using script.addPreloadScript) and uses it immediately after window.open('about:blank').
The test works if I add await new Promise(r => win.onload = r) immediately after window.open('about:blank'). So in this case the preload scripts run, but with some delay.
If I change window.open('about:blank') to window.open(), the test times out, apparently the win.onload callback is never called in this case. If I then change await new Promise(r => win.onload = r) to await new Promise(r => setTimeout(r, 1000)), the test fails with the error message TypeError: win.add is not a function, so apparently the preload script does not run at all in this case.

Comment 4

[Holger Benl [:hbenl]]

So there are 2 issues here:

• preload scripts run with some delay in general
• preload scripts don't run at all for windows opened with window.open()

I opened Bug 1988719 for the second issue.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

window.open() doesn't wait for the navigation to be done and returns immediately. We are waiting for the document inserted to block the parser and to set the preload scripts. But we do not get to this state because the open command returned too early.

Holger is going to create a wdspec test so that we can check how both Chrome and Firefox behave.

Comment 6

[Holger Benl [:hbenl]]

Here's the test: https://github.com/web-platform-tests/wpt/pull/55035

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Lets get the test landed and triage again next week.

Comment 8

[Julian Descottes [:jdescottes]]

The suggestion from triage was to split the followup work in two bugs:

• one to ensure script.callFunction & script.evaluate are only processed once the configuration (incl. preload scripts) is done. This should be relatively easy to enforce on our side, could be P3 m18, or P2 m19
  • "Ensure preload scripts are executed before handling script.evaluate/callFunction"
• one to ensure that the next synchronous JS statement after a window.open() is executed after preload scripts. Probably more challenging - and less realistic for actual tests. P2 or P3 backlog (maybe depending on feedback from Playwright concerning the impacted tests)
  • "Ensure preload scripts are executed before handling statements following a window.open call"

We can either close this one and file two new bugs, or just repurpose and file another one, as we want.

Comment 9

[Holger Benl [:hbenl]]

I've created Bug 1994680 for the first part and repurposed this one for the second part.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The test that Holger added will be syned downstream via bug 1990472.

Comment 11

[James Graham [:jgraham]]

Per the spec this is clearly supposed to work; we should run the preload script more or less as soon as there's a Document object, and that's supposed to be present before window.open returns. Indeed it must be because window.open().document should reliably work rather than throw.

I wonder if there's some optimization in Gecko that means we don't force the document to be constructed until it's accessed? In which case the notification we rely on might be delayed until after the page accesses the DOM. Alternatively maybe it's just an ordering problem with the notification event happening after the content JS has continued executing.

:smaug - are you the right person to give us some insight into what's going on here? To be clear (because the bug got a bit confusing), the issue is code like:

let win = window.open()
// Expect WebDriver preload scripts to run here
win.somePropertySetByPreloadScript // But this is failing

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

What is a preload script? I didn't see a link to a spec here (and I don't know which spec might define it :) ).
Gecko does still in some cases create about:blank lazily, but in window open case I think there should be one in practice always.
https://bugzilla.mozilla.org/show_bug.cgi?id=543435 will change some of this so that about:blank should be there always.

Whatever this preload script thingie is, do we possibly load it asynchronously? And then we likely let window.open() return.

And what triggers executing a preload script in Gecko?

Comment 13

[James Graham [:jgraham]]

It's a WebDriver thing, a script that's supposed to run as soon as the document is created so that later scripts (WebDriver or in the page) can depend on the preload scripts already having run. The HTML PR is https://github.com/whatwg/html/pull/8300/ (which I need to follow up on since it didn't ever really get review).

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Ok, and how are we implementing this stuff? Certainly when window.open() returns we have the relevant window object created, and one can access it's document. Are we not using that as the point when to run the scripts.

Comment 15

[James Graham [:jgraham]]

We're listening for document-element-inserted: https://searchfox.org/firefox-main/source/remote/shared/js-process-actors/WebDriverDocumentInsertedChild.sys.mjs#24

It seems possible (although I'm not sure) that the problem here is that we're doing the actual work async, but consumers require the scripts to be run synchronously.

Comment 16

[Vincent Hilla [:vhilla]]

There seem to be some kind of delay until the preload settings are getting applied.

See my comment on bug 1996385.

I think this issue might go away with bug 543435. Locally with the sync-about-blank patch, I can see preload scripts completing before window.open() returns. I'm a bit surprised by that, but perhaps _configuration.sys.mjs executes them synchronously off of document-element-inserted?

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Holger, are the affected Playwright tests passing now or is more work necessary related to this bug?

Comment 18

[Holger Benl [:hbenl]]

One test is still failing: "BrowserContext.addInitScript should apply to a cross-process popup" in library/popup.spec.ts. If I add a delay, the test passes, so this is still about an evaluation running before the preload script.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thank you. Maybe that is related to some delays when running IPC to sync the data across processes?

Sasha, could you maybe check? I assume we would need a new bug given that this one turned out to be more a meta bug these days.

Comment 20

[Alexandra Borovova [:Sasha]]

BrowserContext.addInitScript should apply to a cross-process popup failure is not related to preload scripts, you can get the same failures if you do instead expect(await popup.evaluate('injected')).toBe(123); just expect(await popup.evaluate('1 + 2')).toBe(3);. Also, the error message is Error: page.evaluate: Execution context was destroyed, most likely because of a navigation, which means that the page is still navigating, which, of course, is finished if you add a delay.
I think this test is just missing await popup.waitForLoadState(); before evaluating the script in the popup, similar to https://github.com/microsoft/playwright/blob/main/tests/library/geolocation.spec.ts#L167-L172 or github.com/microsoft/playwright/blob/main/tests/library/page-clock.spec.ts#L353-L358.

Comment 21

[Holger Benl [:hbenl]]

the error message is Error: page.evaluate: Execution context was destroyed, most likely because of a navigation

When I run the test locally I get the error Error: page.evaluate: ReferenceError: injected is not defined but when I add a 10ms delay, I also get Error: page.evaluate: Execution context was destroyed, most likely because of a navigation. I guess that without the delay, the evaluation is executed in the initial about:blank page before it navigates to the requested URL.

I think this test is just missing await popup.waitForLoadState(); before evaluating the script in the popup

That would fix this test but I think the real fix would be to wait for the browsingContext.navigationCommitted event before emitting the popup event in Playwright but we currently can't do that because we don't know if this event will be emitted or if the new browsing context will stay on about:blank (see this comment).

Comment 22

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1985997 - [webdriver-bidi] Use "content-document-global-created" notification instead "document-element-inserted" to run preload scripts and geolocation earlier.

Comment 23

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1985997 - [wdspec] Add test case for setting preload script for window.open with not "about:blank" url.

Comment 24

[Alexandra Borovova [:Sasha]]

Ok, I've managed to reproduce the issue with the preload script not being run in time by checking the value in the same evaluation that opened the window (similar to previous issues). It looks like switching to the same notification Playwright uses solves the problem (it runs earlier), and I think it should be good enough since we now only set JS-related things, we don't need an actual document.

Holger, since I cannot reproduce the preload script with this playwright, would you mind verifying that with my patch you consistently get "Execution context was destroyed, most likely because of a navigation"?

Comment 25

[Holger Benl [:hbenl]]

With your patch, the evaluation consistently works for me. The test then fails at the next line (await popup.reload()) with the error page.reload: Navigation failed but that's because it tries to reload the popup before it finished its initial navigation (and for some reason Playwright attributes the navigationFailed event to the reload even though it is for the initial navigation). Adding await popup.waitForLoadState() before the reload fixes the test (but the real fix for that would be Playwright waiting for the navigationCommitted event before sending the popup event but it currently can't do that because it doesn't know if this event will ever be sent).
So the patch fixes this issue and the remaining issue is covered by https://github.com/w3c/webdriver-bidi/issues/832.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/da0bcc35e2cd
https://hg.mozilla.org/integration/autoland/rev/75a2ff1771ff
[webdriver-bidi] Use "content-document-global-created" notification instead "document-element-inserted" to run preload scripts and geolocation earlier. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/9aee31a171f8
https://hg.mozilla.org/integration/autoland/rev/bb2f2e40cb04
[wdspec] Add test case for setting preload script for window.open with not "about:blank" url. r=jdescottes

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/57875 for changes under testing/web-platform/tests

Comment 28

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/75a2ff1771ff
https://hg.mozilla.org/mozilla-central/rev/bb2f2e40cb04

Comment 29

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot