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

Popup window is closing before the end of the test in html/semantics/… #30001

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Aug 11, 2021

…links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html

In html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html, the "Check that targeting of rel=noopener with
a given name reuses an existing window with that name" test (t1) opens a popup window via window.open() and then navigates this window to
"support/noopener-target-2.html" via link targetting. The issue is that the JS inside noopener-target-2.html sends a BroadcastChannel message
and then calls window.close() right away. Then htmlanchorelement_noopener.html receives the BroadcastChannel messages, it does the following
check: assert_equals(w.opener, window);. This check is failing in WebKit because the popup window |w| has already been closed by the time
the message is received. The test already takes care of calling w.close() as a clean up step so there is actually no need for
noopener-target-2.html to call window.close() itself.

…links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html

In html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html, the "Check that targeting of rel=noopener with
a given name reuses an existing window with that name" test (t1) opens a popup window via window.open() and then navigates this window to
"support/noopener-target-2.html" via link targetting. The issue is that the JS inside noopener-target-2.html sends a BroadcastChannel message
and then calls `window.close()` right away. Then htmlanchorelement_noopener.html receives the BroadcastChannel messages, it does the following
check: `assert_equals(w.opener, window);`. This check is failing in WebKit because the popup window |w| has already been closed by the time
the message is received. The test already takes care of calling `w.close()` as a clean up step so there is actually no need for
noopener-target-2.html to call `window.close()` itself.
@domenic
Copy link
Member

domenic commented Aug 12, 2021

Hmm. By my reading of the spec, if you do BroadcastChannel + window.close(), those both post a task on the same task source (the DOM manipulation task source). So they should execute in order; it shouldn't be possible for the window to close before the message is delivered.

@cdumez
Copy link
Contributor Author

cdumez commented Aug 12, 2021

Hmm. By my reading of the spec, if you do BroadcastChannel + window.close(), those both post a task on the same task source (the DOM manipulation task source). So they should execute in order; it shouldn't be possible for the window to close before the message is delivered.

Oh I see, this is likely a bug in the WebKit implementation of BroadcastChannel then. This would explain why the test is passing in Firefox and Chrome. It is a bit tricky because BroadcastChannel allows communication cross-process (and thus involves IPC in WebKit).

@cdumez cdumez closed this Aug 12, 2021
@domenic
Copy link
Member

domenic commented Aug 12, 2021

Yeah, I'm not sure that part of the spec was written very well from that point of view, so I at least would be open to changing the spec. But maybe if it's working in 2/3 browsers people are depending on it... hard to say.

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this pull request Sep 16, 2021
…links-created-by-a-and-area-elements/htmlanchorelement_noopener.html is flaky failing

https://bugs.webkit.org/show_bug.cgi?id=228089
<rdar://problem/80801807>

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Rebaseline test that is now consistently passing.

* web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-expected.txt:

Source/WebCore:

As per the HTML specification, window.close() should schedule a task on the event loop to actually
close the window:
- https://html.spec.whatwg.org/multipage/window-object.html#dom-window-close

We were failing to do so and this was causing flakiness because event ordering was inconsistent.

This was discussed on upstream WPT here:
- web-platform-tests/wpt#30001

No new tests, unskipped existing test.

* page/DOMWindow.cpp:
(WebCore::DOMWindow::close):

LayoutTests:

Unskip test that should no longer be flaky.

* platform/ios-wk2/TestExpectations:
* platform/mac-wk1/TestExpectations:
* platform/mac-wk2/TestExpectations:


Canonical link: https://commits.webkit.org/241767@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@282606 268f45cc-cd09-0410-ab3c-d52691b4dbfc
bertogg pushed a commit to Igalia/webkit that referenced this pull request Sep 17, 2021
…links-created-by-a-and-area-elements/htmlanchorelement_noopener.html is flaky failing

https://bugs.webkit.org/show_bug.cgi?id=228089
<rdar://problem/80801807>

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Rebaseline test that is now consistently passing.

* web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-expected.txt:

Source/WebCore:

As per the HTML specification, window.close() should schedule a task on the event loop to actually
close the window:
- https://html.spec.whatwg.org/multipage/window-object.html#dom-window-close

We were failing to do so and this was causing flakiness because event ordering was inconsistent.

This was discussed on upstream WPT here:
- web-platform-tests/wpt#30001

No new tests, unskipped existing test.

* page/DOMWindow.cpp:
(WebCore::DOMWindow::close):

LayoutTests:

Unskip test that should no longer be flaky.

* platform/ios-wk2/TestExpectations:
* platform/mac-wk1/TestExpectations:
* platform/mac-wk2/TestExpectations:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@282606 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants