Bug 228089 – [ MacOS & iOS & GTK ] imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html is flaky failing

• Component: New Bugs
• Status: RESOLVED FIXED

imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html 

Is flaky failing on MacOS wk1 

History: https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Flinks%2Flinks-created-by-a-and-area-elements%2Fhtmlanchorelement_noopener.html

Diff:

========================

--- /Volumes/Data/worker/bigsur-debug-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-expected.txt
+++ /Volumes/Data/worker/bigsur-debug-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-actual.txt
@@ -3,6 +3,6 @@
 PASS Check that rel=noopener with target=_self does a normal load
 PASS Check that rel=noopener with target=_parent does a normal load
 PASS Check that rel=noopener with target=_top does a normal load
-PASS Check that targeting of rel=noopener with a given name reuses an existing window with that name
+FAIL Check that targeting of rel=noopener with a given name reuses an existing window with that name assert_equals: expected object "[object Window]" but got null
 PASS Check that targeting of rel=noopener with a given name reuses an existing subframe with that name

========================

I was able to reproduce the failure on tip-of-tree and on r27971 using run-webkit-tests imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html -1 -f -iterations 500 --exit-after-n-failures 2 --exit-after-n-crashes-or-timeouts 2
 
On r279969, the test failed, but I received a different diff:

========================

--- /Volumes/Data/Builds/test-279969/layout-test-results/imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-expected.txt
+++ /Volumes/Data/Builds/test-279969/layout-test-results/imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-actual.txt
@@ -1,8 +1,10 @@
+CONSOLE MESSAGE: ReferenceError: Can't find variable: BroadcastChannel
 
+
+Harness Error (FAIL), message = ReferenceError: Can't find variable: BroadcastChannel
 
 PASS Check that rel=noopener with target=_self does a normal load
 PASS Check that rel=noopener with target=_parent does a normal load
 PASS Check that rel=noopener with target=_top does a normal load
-PASS Check that targeting of rel=noopener with a given name reuses an existing window with that name
-PASS Check that targeting of rel=noopener with a given name reuses an existing subframe with that name
+NOTRUN Check that targeting of rel=noopener with a given name reuses an existing window with that name

========================

[Radar WebKit Bug Importer]

<rdar://problem/80801807>

[Chris Dumez]

This test is indeed relying on BroadcastChannel. Prior to my patch that added BroadcastChannel, it was not properly running due to lack of BroadcastChannel support.

When I added support, the test started running and I landed baselines for it. However, it appears the test is flaky.

This is not really an issue with my patch but really an issue with the test (or WebKit not properly supporting what this test is checking).

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> This test is indeed relying on BroadcastChannel. Prior to my patch that
> added BroadcastChannel, it was not properly running due to lack of
> BroadcastChannel support.
> 
> When I added support, the test started running and I landed baselines for
> it. However, it appears the test is flaky.
> 
> This is not really an issue with my patch but really an issue with the test
> (or WebKit not properly supporting what this test is checking).

w.opener is sometimes null in the message event handler. It is unclear why at this point but this seems to be a navigation with target="foo" issue, not a BroadcastChannel issue.

[Chris Dumez]

Committed r280051 (239786@main): <https://commits.webkit.org/239786@main>

[Chris Dumez]

Marked test as flaky on WK1 via <https://commits.webkit.org/r280051>.

This is not super high priority given that it is not really a regression, merely a test that started running as a recent of us enabling BroadcastChannel, which the test required to run.

[Chris Dumez]

Reopening since the bug wasn't fixed.

The test is now failing on iOS and Mac wk2. (Started at around r282378)

Added expectations: https://trac.webkit.org/changeset/282388/webkit

[Chris Dumez]

Created attachment 438248 [details]
Patch

[Chris Dumez]

Created attachment 438252 [details]
Patch

[Chris Dumez]

Created attachment 438256 [details]
Patch

[Carlos Alberto Lopez Perez]

This test is also flaky on GTK, with the same failure:

--- /home/buildbot/worker/gtk-linux-64-release-skip-failing-tests/build/layout-test-results/imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-expected.txt
+++ /home/buildbot/worker/gtk-linux-64-release-skip-failing-tests/build/layout-test-results/imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-actual.txt
@@ -3,6 +3,6 @@
 PASS Check that rel=noopener with target=_self does a normal load
 PASS Check that rel=noopener with target=_parent does a normal load
 PASS Check that rel=noopener with target=_top does a normal load
-FAIL Check that targeting of rel=noopener with a given name reuses an existing window with that name assert_equals: expected object "[object Window]" but got null
+PASS Check that targeting of rel=noopener with a given name reuses an existing window with that name
 PASS Check that targeting of rel=noopener with a given name reuses an existing subframe with that name

[Carlos Alberto Lopez Perez]

This test is also flaky on GTK, with the same failure:

--- /home/clopez/webkit/webkit-flatpak/layout-test-results/imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-expected.txt
+++ /home/clopez/webkit/webkit-flatpak/layout-test-results/imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-actual.txt
@@ -3,6 +3,6 @@
 PASS Check that rel=noopener with target=_self does a normal load
 PASS Check that rel=noopener with target=_parent does a normal load
 PASS Check that rel=noopener with target=_top does a normal load
-PASS Check that targeting of rel=noopener with a given name reuses an existing window with that name
+FAIL Check that targeting of rel=noopener with a given name reuses an existing window with that name assert_equals: expected object "[object Window]" but got null
 PASS Check that targeting of rel=noopener with a given name reuses an existing subframe with that name

[Chris Dumez]

Created attachment 438281 [details]
Patch

[Carlos Alberto Lopez Perez]

GTK expectations updated in r282471

[Chris Dumez]

Created attachment 438286 [details]
Patch

[Darin Adler]

Comment on attachment 438286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438286&action=review

> Source/WebCore/page/DOMWindow.cpp:1062
> +            page->chrome().closeWindowSoon();

Is the "soon" still needed? Seems like the queued task is now taking care of the scheduling. Can we clean up further by just doing closeWindow; at some point?

[Chris Dumez]

(In reply to Darin Adler from comment #16)
> Comment on attachment 438286 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438286&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:1062
> > +            page->chrome().closeWindowSoon();
> 
> Is the "soon" still needed? Seems like the queued task is now taking care of
> the scheduling. Can we clean up further by just doing closeWindow; at some
> point?

I considered it but technically WebKit2 ends up doing an async IPC to the UIProcess so the closing does not happen synchronously and the "soon" part is not a lie.

[Chris Dumez]

Created attachment 438349 [details]
Patch

[EWS]

Committed r282606 (241767@main): <https://commits.webkit.org/241767@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438349 [details].