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

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-side-effects-ignore-opens-during-unload.window.html looks buggy #14909

Closed
bzbarsky opened this issue Jan 16, 2019 · 5 comments · Fixed by #23667
Labels

Comments

@bzbarsky
Copy link
Contributor

The test says does a load in an iframe and during the beforeunload event sets up a Promise to run the actual test bits that involve calling document.open.

The test has a comment that claims that this should happen before the ignore-opens-during-unload counter is decremented, because microtasks run during "clean up after running script".

But microtasks only run during "clean up after running script" if the JavaScript execution context stack is empty. In this case it's not. Indeed, setting iframe.src from script calls into https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-iframe-element:process-the-iframe-attributes-3 which calls https://html.spec.whatwg.org/multipage/iframe-embed-object.html#process-the-iframe-attributes which calls https://html.spec.whatwg.org/multipage/iframe-embed-object.html#otherwise-steps-for-iframe-or-frame-elements which calls into https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate which in step 8 calls https://html.spec.whatwg.org/multipage/browsing-the-web.html#prompt-to-unload-a-document and fires beforeunload. This is all happening sync, while the script that set src is on the stack. So there is not a microtask checkpoint at the end of the event listener firing.

How exactly does Chrome pass this test? @TimothyGu @domenic

@bzbarsky
Copy link
Contributor Author

The right way to test this if we really need the entry global to be the subframe is probably to enter script inside the subframe (e.g. via a string setTimeout call on it) and then do the src set from that script.

@foolip foolip changed the title html / webappapis / dynamic-markup-insertion / opening-the-input-stream / bailout-side-effects-ignore-opens-during-unload.window.html looks buggy html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-side-effects-ignore-opens-during-unload.window.html looks buggy Jan 17, 2019
@TimothyGu
Copy link
Member

TimothyGu commented Jan 17, 2019

By this logic https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-side-effects-synchronous-script.window.js would need to be changed as well, as it uses Promise.resolve to a similar end. Wait that’s gibberish. Please ignore

@TimothyGu
Copy link
Member

The right way to test this if we really need the entry global to be the subframe

I might be mistaken but it seems like we want the entry realm of the test to not be the subframe, which is the whole point of calling Promise.resolve in the top frame. Indeed, what we want to test here is that the URL doesn’t change, despite the fact that the entry document is different from the document itself and document.open() would ordinarily be effective.

@TimothyGu
Copy link
Member

Drawing purely from memory, I think the way I wrote the test was this: Chrome doesn’t quite follow the spec fully regarding the entry realm of event listeners, so I had to find a way to change the entry realm. (Hence I said “the entry settings object could still be the iframe's.“)

However, Chrome also isn’t fully sync when it comes to src setting-caused unload events, so Promise.resolve worked fine as a method to delay execution.

If Firefox is indeed completely synchronous from src, then the Promise.resolve hack wouldn’t be necessary.

@bzbarsky
Copy link
Contributor Author

If Firefox is indeed completely synchronous from src

It is.

@gsnedders gsnedders added the html label Jan 18, 2019
TimothyGu added a commit that referenced this issue May 18, 2020
…ing-unload.window.html

The call was originally to mitigate a Chrome bug involving incorrectly
set entry realm, but it turns out to not actually work per spec. After
this PR, Firefox and Chrome would both pass this test, as they have
fixed their own respective issues.

Fixes: #14909
domenic pushed a commit that referenced this issue May 18, 2020
…ing-unload.window.html

The call was originally to mitigate a Chrome bug involving incorrectly
set entry realm, but it turns out to not actually work per spec. After
this PR, Firefox and Chrome would both pass this test, as they have
fixed their own respective issues.

Fixes: #14909
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants