[ MacOS & iOS ] imported/w3c/web-platform-tests/html/semantics/links/…

…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
WebKit · Sep 16, 2021 · 64b0d86 · 64b0d86
1 parent fe9cf89
commit 64b0d86
Show file tree

Hide file tree

Showing 22 changed files with 69 additions and 102 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2021-09-16  Chris Dumez  <cdumez@apple.com>
+
+        [ MacOS & iOS ] imported/w3c/web-platform-tests/html/semantics/links/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.
+
+        Unskip test that should no longer be flaky.
+
+        * platform/ios-wk2/TestExpectations:
+        * platform/mac-wk1/TestExpectations:
+        * platform/mac-wk2/TestExpectations:
+
 2021-09-16  Ayumi Kojima  <ayumi_kojima@apple.com>
 
         [ iOS ] http/tests/workers/service/client-removed-from-clients-while-in-page-cache.html is a flaky timeout.

diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,15 @@
+2021-09-16  Chris Dumez  <cdumez@apple.com>
+
+        [ MacOS & iOS ] imported/w3c/web-platform-tests/html/semantics/links/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.
+
+        Rebaseline test that is now consistently passing.
+
+        * web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-expected.txt:
+
 2021-09-16  Chris Dumez  <cdumez@apple.com>
 
         Add violations reporting support for Cross-Origin-Embedder-Policy

diff --git a/...antics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-expected.txt b/...antics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener-expected.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
 
diff --git a/LayoutTests/platform/gtk/TestExpectations b/LayoutTests/platform/gtk/TestExpectations
@@ -1232,8 +1232,6 @@ webkit.org/b/229270 fast/events/monotonic-event-time.html [ Failure Pass ]
 
 webkit.org/b/230017 http/wpt/cross-origin-opener-policy/non-secure-to-secure-context-navigation.https.html [ Failure Pass ]
 
-webkit.org/b/228089 imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html  [ Pass Failure ]
-
 #////////////////////////////////////////////////////////////////////////////////////////
 # End of Flaky tests
 #////////////////////////////////////////////////////////////////////////////////////////

diff --git a/LayoutTests/platform/ios-wk2/TestExpectations b/LayoutTests/platform/ios-wk2/TestExpectations
@@ -1449,8 +1449,6 @@ webkit.org/b/209006 imported/w3c/web-platform-tests/html/semantics/scripting-1/t
 # Newly imported WPT test that is flaky on iOS only.
 imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/image-compositing-change.html [ ImageOnlyFailure Pass ]
 
-webkit.org/b/228089 imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html [ Pass Failure ]
-
 webkit.org/b/209205 fast/events/autoscroll-in-iframe.html [ Pass Failure ]
 
 webkit.org/b/209080 imported/w3c/web-platform-tests/css/css-writing-modes/wm-propagation-body-scroll-offset-vertical-lr.html [ Failure ]

diff --git a/LayoutTests/platform/mac-wk1/TestExpectations b/LayoutTests/platform/mac-wk1/TestExpectations
@@ -1333,9 +1333,6 @@ webkit.org/b/227085 imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-t
 [ BigSur ] imported/w3c/web-platform-tests/css/css-will-change/will-change-stacking-context-filter-1.html [ Pass ImageOnlyFailure ]
 [ BigSur ] imported/w3c/web-platform-tests/css/css-will-change/will-change-stacking-context-opacity-1.html [ Pass ImageOnlyFailure ]
 
-# This test relies on BroadcastChannel and has been flaky on WK1 since we added support for BroadcastChannel.
-webkit.org/b/228089 imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html  [ Pass Failure ]
-
 # rdar://80331434 ([ Mac WK1 Release ] accessibility/loading-iframe-sends-notification.html is a flaky timeout)
 [ Release ] accessibility/loading-iframe-sends-notification.html [ Pass Timeout ]
 

diff --git a/LayoutTests/platform/mac-wk2/TestExpectations b/LayoutTests/platform/mac-wk2/TestExpectations
@@ -1063,8 +1063,6 @@ webkit.org/b/208889 fast/images/decode-render-animated-image.html [ Pass ImageOn
 
 webkit.org/b/209006 imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/execution-timing/085.html [ Pass Failure ]
 
-webkit.org/b/228089 imported/w3c/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html [ Pass Failure ]
-
 webkit.org/b/209077 svg/custom/object-sizing-explicit-width.xhtml [ Pass Failure ]
 
 webkit.org/b/209052 fast/scrolling/mac/absolute-in-overflow-scroll-dynamic.html [ Pass ImageOnlyFailure ]

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,25 @@
+2021-09-16  Chris Dumez  <cdumez@apple.com>
+
+        [ MacOS & iOS ] imported/w3c/web-platform-tests/html/semantics/links/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.
+
+        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:
+        - https://github.com/web-platform-tests/wpt/pull/30001
+
+        No new tests, unskipped existing test.
+
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::close):
+
 2021-09-16  Chris Dumez  <cdumez@apple.com>
 
         Add violations reporting support for Cross-Origin-Embedder-Policy

diff --git a/Source/WebCore/loader/EmptyClients.h b/Source/WebCore/loader/EmptyClients.h
@@ -87,7 +87,7 @@ class EmptyChromeClient : public ChromeClient {
     bool canRunBeforeUnloadConfirmPanel() final { return false; }
     bool runBeforeUnloadConfirmPanel(const String&, Frame&) final { return true; }
 
-    void closeWindowSoon() final { }
+    void closeWindow() final { }
 
     void runJavaScriptAlert(Frame&, const String&) final { }
     bool runJavaScriptConfirm(Frame&, const String&) final { return false; }

diff --git a/Source/WebCore/page/Chrome.cpp b/Source/WebCore/page/Chrome.cpp
@@ -286,9 +286,9 @@ bool Chrome::runBeforeUnloadConfirmPanel(const String& message, Frame& frame)
     return m_client.runBeforeUnloadConfirmPanel(message, frame);
 }
 
-void Chrome::closeWindowSoon()
+void Chrome::closeWindow()
 {
-    m_client.closeWindowSoon();
+    m_client.closeWindow();
 }
 
 void Chrome::runJavaScriptAlert(Frame& frame, const String& message)

diff --git a/Source/WebCore/page/Chrome.h b/Source/WebCore/page/Chrome.h
@@ -140,7 +140,7 @@ class Chrome : public HostWindow {
     bool canRunBeforeUnloadConfirmPanel();
     bool runBeforeUnloadConfirmPanel(const String& message, Frame&);
 
-    void closeWindowSoon();
+    void closeWindow();
 
     void runJavaScriptAlert(Frame&, const String&);
     bool runJavaScriptConfirm(Frame&, const String&);

diff --git a/Source/WebCore/page/ChromeClient.h b/Source/WebCore/page/ChromeClient.h
@@ -185,7 +185,7 @@ class ChromeClient {
     virtual bool canRunBeforeUnloadConfirmPanel() = 0;
     virtual bool runBeforeUnloadConfirmPanel(const String& message, Frame&) = 0;
 
-    virtual void closeWindowSoon() = 0;
+    virtual void closeWindow() = 0;
 
     virtual void runJavaScriptAlert(Frame&, const String&) = 0;
     virtual bool runJavaScriptConfirm(Frame&, const String&) = 0;

diff --git a/Source/WebCore/page/DOMWindow.cpp b/Source/WebCore/page/DOMWindow.cpp
@@ -1056,7 +1056,11 @@ void DOMWindow::close()
     ResourceLoadObserver::shared().updateCentralStatisticsStore([] { });
 
     page->setIsClosing();
-    page->chrome().closeWindowSoon();
+
+    document()->eventLoop().queueTask(TaskSource::DOMManipulation, [this, protectedThis = makeRef(*this)] {
+        if (auto* page = this->page())
+            page->chrome().closeWindow();
+    });
 }
 
 void DOMWindow::print()

diff --git a/Source/WebCore/page/FrameTree.cpp b/Source/WebCore/page/FrameTree.cpp
@@ -270,7 +270,7 @@ Frame* FrameTree::find(const AtomString& name, Frame& activeFrame) const
         return nullptr;
 
     for (auto& otherPage : page->group().pages()) {
-        if (&otherPage == page)
+        if (&otherPage == page || otherPage.isClosing())
             continue;
         for (auto* frame = &otherPage.mainFrame(); frame; frame = frame->tree().traverseNext()) {
             if (frame->tree().uniqueName() == name && isFrameFamiliarWith(activeFrame, *frame))

diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp
@@ -421,7 +421,7 @@ bool WebChromeClient::runBeforeUnloadConfirmPanel(const String& message, Frame&
     return shouldClose;
 }
 
-void WebChromeClient::closeWindowSoon()
+void WebChromeClient::closeWindow()
 {
     // FIXME: This code assumes that the client will respond to a close page
     // message by actually closing the page. Safari does this, but there is

diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h b/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h
@@ -102,7 +102,7 @@ class WebChromeClient final : public WebCore::ChromeClient {
     bool canRunBeforeUnloadConfirmPanel() final;
     bool runBeforeUnloadConfirmPanel(const String& message, WebCore::Frame&) final;
 
-    void closeWindowSoon() final;
+    void closeWindow() final;
 
     void runJavaScriptAlert(WebCore::Frame&, const String&) final;
     bool runJavaScriptConfirm(WebCore::Frame&, const String&) final;

diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.h b/Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.h
@@ -89,7 +89,7 @@ class WebChromeClient : public WebCore::ChromeClient {
     bool canRunBeforeUnloadConfirmPanel() final;
     bool runBeforeUnloadConfirmPanel(const String& message, WebCore::Frame&) final;
 
-    void closeWindowSoon() final;
+    void closeWindow() final;
 
     void runJavaScriptAlert(WebCore::Frame&, const String&) override;
     bool runJavaScriptConfirm(WebCore::Frame&, const String&) override;

diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm b/Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm
@@ -467,7 +467,7 @@ - (void)setIsSelected:(BOOL)isSelected;
     return CallUIDelegateReturningBoolean(true, m_webView, @selector(webView:runBeforeUnloadConfirmPanelWithMessage:initiatedByFrame:), message, kit(&frame));
 }
 
-void WebChromeClient::closeWindowSoon()
+void WebChromeClient::closeWindow()
 {
     // We need to remove the parent WebView from WebViewSets here, before it actually
     // closes, to make sure that JavaScript code that executes before it closes
@@ -484,7 +484,7 @@ - (void)setIsSelected:(BOOL)isSelected;
 
     [m_webView setGroupName:nil];
     [m_webView stopLoading:nil];
-    [m_webView performSelector:@selector(_closeWindow) withObject:nil afterDelay:0.0];
+    [m_webView _closeWindow];
 }
 
 void WebChromeClient::runJavaScriptAlert(Frame& frame, const String& message)

diff --git a/Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.cpp b/Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.cpp
@@ -373,7 +373,7 @@ bool WebChromeClient::runBeforeUnloadConfirmPanel(const String& message, Frame&
     return !!result;
 }
 
-void WebChromeClient::closeWindowSoon()
+void WebChromeClient::closeWindow()
 {
     // We need to remove the parent WebView from WebViewSets here, before it actually
     // closes, to make sure that JavaScript code that executes before it closes
@@ -390,7 +390,7 @@ void WebChromeClient::closeWindowSoon()
 
     m_webView->setGroupName(0);
     m_webView->stopLoading(0);
-    m_webView->closeWindowSoon();
+    m_webView->closeWindow();
 }
 
 void WebChromeClient::runJavaScriptAlert(Frame&, const String& message)

diff --git a/Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.h b/Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.h
@@ -85,7 +85,7 @@ class WebChromeClient final : public WebCore::ChromeClient {
     bool canRunBeforeUnloadConfirmPanel() final;
     bool runBeforeUnloadConfirmPanel(const WTF::String& message, WebCore::Frame&) final;
 
-    void closeWindowSoon() final;
+    void closeWindow() final;
 
     void runJavaScriptAlert(WebCore::Frame&, const WTF::String&) final;
     bool runJavaScriptConfirm(WebCore::Frame&, const WTF::String&) final;

diff --git a/Source/WebKitLegacy/win/WebView.cpp b/Source/WebKitLegacy/win/WebView.cpp
@@ -1456,78 +1456,6 @@ void WebView::frameRect(RECT* rect)
     ::GetWindowRect(m_viewWindow, rect);
 }
 
-class WindowCloseTimer final : public WebCore::SuspendableTimerBase {
-public:
-    static WindowCloseTimer* create(WebView*);
-
-private:
-    WindowCloseTimer(ScriptExecutionContext&, WebView*);
-
-    // ActiveDOMObject API.
-    void contextDestroyed() override;
-    const char* activeDOMObjectName() const override { return "WindowCloseTimer"; }
-
-    // SuspendableTimerBase API.
-    void fired() override;
-
-    WebView* m_webView;
-};
-
-WindowCloseTimer* WindowCloseTimer::create(WebView* webView)
-{
-    ASSERT_ARG(webView, webView);
-    Frame* frame = core(webView->topLevelFrame());
-    ASSERT(frame);
-    if (!frame)
-        return nullptr;
-
-    Document* document = frame->document();
-    ASSERT(document);
-    if (!document)
-        return nullptr;
-
-    auto closeTimer = new WindowCloseTimer(*document, webView);
-    closeTimer->suspendIfNeeded();
-    return closeTimer;
-}
-
-WindowCloseTimer::WindowCloseTimer(ScriptExecutionContext& context, WebView* webView)
-    : SuspendableTimerBase(&context)
-    , m_webView(webView)
-{
-    ASSERT_ARG(webView, webView);
-}
-
-void WindowCloseTimer::contextDestroyed()
-{
-    SuspendableTimerBase::contextDestroyed();
-    delete this;
-}
-
-void WindowCloseTimer::fired()
-{
-    m_webView->closeWindowTimerFired();
-}
-
-void WebView::closeWindowSoon()
-{
-    if (m_closeWindowTimer)
-        return;
-
-    m_closeWindowTimer = WindowCloseTimer::create(this);
-    if (!m_closeWindowTimer)
-        return;
-    m_closeWindowTimer->startOneShot(0_s);
-
-    AddRef();
-}
-
-void WebView::closeWindowTimerFired()
-{
-    closeWindow();
-    Release();
-}
-
 void WebView::closeWindow()
 {
     if (m_hasSpellCheckerDocumentTag) {
@@ -5557,8 +5485,7 @@ HRESULT WebView::notifyPreferencesChanged(IWebNotification* notification)
 
     settings.setShowsToolTipOverTruncatedText(enabled);
 
-    if (!m_closeWindowTimer)
-        m_mainFrame->invalidate(); // FIXME
+    m_mainFrame->invalidate(); // FIXME
 
     hr = updateSharedSettingsFromPreferencesIfNeeded(preferences.get());
     if (FAILED(hr))

diff --git a/Source/WebKitLegacy/win/WebView.h b/Source/WebKitLegacy/win/WebView.h
@@ -438,8 +438,6 @@ class WebView final
     void repaint(const WebCore::IntRect&, bool contentChanged, bool immediate = false, bool repaintContentOnly = false);
     void frameRect(RECT* rect);
     void closeWindow();
-    void closeWindowSoon();
-    void closeWindowTimerFired();
     bool didClose() const { return m_didClose; }
 
     bool transparent() const { return m_transparent; }
@@ -680,7 +678,6 @@ class WebView final
 
     static bool s_allowSiteSpecificHacks;
 
-    WebCore::SuspendableTimerBase* m_closeWindowTimer { nullptr };
     std::unique_ptr<TRACKMOUSEEVENT> m_mouseOutTracker;
 
     HWND m_topLevelParent { nullptr };