Regression(r236862): Crash under DOMWindowExtension::willDetachGlobalObjectFromFrame()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Oct 2018 22:25:49 +0000 (22:25 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Oct 2018 22:25:49 +0000 (22:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=190320
<rdar://problem/45044814>

Reviewed by Geoffrey Garen.

Source/WebCore:

r236862 caused DOMWindowProperty::willDetachGlobalObjectFromFrame() to get called several
times. There was no effect for most DOMWindowProperty objects. However, it would cause
crashes for DOMWindowExtension objects, which subclass DOMWindowProperty and override
DOMWindowProperty::willDetachGlobalObjectFromFrame() because they dereference the frame
without null checking it.

To address the issue, we now make sure DOMWindowProperty::willDetachGlobalObjectFromFrame()
is not called several times.

* dom/Document.cpp:
(WebCore::Document::detachFromFrame):
Stop calling DOMWindow::willDetachDocumentFromFrame() here as most call sites already
take care of calling DOMWindow::willDetachDocumentFromFrame() beforehand (e.g.
Document::prepareForDestruction()).
Also, return early if the Document is already detached from its frame.

(WebCore::Document::frameWasDisconnectedFromOwner):
Add new utility function called when a Frame is disconnected from its owner which
calls both Document::detachFromFrame() and DOMWindow::willDetachDocumentFromFrame().

* dom/Document.h:
* page/DOMWindow.cpp:
(WebCore::DOMWindow::willDetachDocumentFromFrame):
Return early if the Window is already detached from its frame.

* page/Frame.cpp:
(WebCore::Frame::disconnectOwnerElement):

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic.cpp:
(TestWebKitAPI::TEST):
* TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic_Bundle.cpp:
(TestWebKitAPI::DOMWindowExtensionBasic::willDestroyGlobalObjectForDOMWindowExtension):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236888 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/page/DOMWindow.cpp
Source/WebCore/page/Frame.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic.cpp
Tools/TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic_Bundle.cpp

index b25b44e..214d0e6 100644 (file)
@@ -1,3 +1,39 @@
+2018-10-05  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r236862): Crash under DOMWindowExtension::willDetachGlobalObjectFromFrame()
+        https://bugs.webkit.org/show_bug.cgi?id=190320
+        <rdar://problem/45044814>
+
+        Reviewed by Geoffrey Garen.
+
+        r236862 caused DOMWindowProperty::willDetachGlobalObjectFromFrame() to get called several
+        times. There was no effect for most DOMWindowProperty objects. However, it would cause
+        crashes for DOMWindowExtension objects, which subclass DOMWindowProperty and override
+        DOMWindowProperty::willDetachGlobalObjectFromFrame() because they dereference the frame
+        without null checking it.
+
+        To address the issue, we now make sure DOMWindowProperty::willDetachGlobalObjectFromFrame()
+        is not called several times.
+
+        * dom/Document.cpp:
+        (WebCore::Document::detachFromFrame):
+        Stop calling DOMWindow::willDetachDocumentFromFrame() here as most call sites already
+        take care of calling DOMWindow::willDetachDocumentFromFrame() beforehand (e.g.
+        Document::prepareForDestruction()).
+        Also, return early if the Document is already detached from its frame.
+
+        (WebCore::Document::frameWasDisconnectedFromOwner):
+        Add new utility function called when a Frame is disconnected from its owner which
+        calls both Document::detachFromFrame() and DOMWindow::willDetachDocumentFromFrame().
+
+        * dom/Document.h:
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::willDetachDocumentFromFrame):
+        Return early if the Window is already detached from its frame.
+
+        * page/Frame.cpp:
+        (WebCore::Frame::disconnectOwnerElement):
+
 2018-10-05  Jer Noble  <jer.noble@apple.com>
 
         Further unreviewed watchOS build fix: videoPerformanceMetrics unavailable on watchOS.
index a7b09ca..752c850 100644 (file)
@@ -8256,10 +8256,18 @@ bool Document::registerCSSProperty(CSSRegisteredCustomProperty&& prop)
 
 void Document::detachFromFrame()
 {
+    observeFrame(nullptr);
+}
+
+void Document::frameWasDisconnectedFromOwner()
+{
+    if (!frame())
+        return;
+
     if (auto* window = domWindow())
         window->willDetachDocumentFromFrame();
 
-    observeFrame(nullptr);
+    detachFromFrame();
 }
 
 } // namespace WebCore
index f778e6c..c6b15d7 100644 (file)
@@ -1505,7 +1505,7 @@ public:
     void setAsRunningUserScripts() { m_isRunningUserScripts = true; }
     bool isRunningUserScripts() const { return m_isRunningUserScripts; }
 
-    void detachFromFrame();
+    void frameWasDisconnectedFromOwner();
 
 protected:
     enum ConstructionFlags { Synthesized = 1, NonRenderedPlaceholder = 1 << 1 };
@@ -1572,6 +1572,8 @@ private:
     void pendingTasksTimerFired();
     bool isCookieAverse() const;
 
+    void detachFromFrame();
+
     template<CollectionType> Ref<HTMLCollection> ensureCachedCollection();
 
 #if ENABLE(FULLSCREEN_API)
index f6aa23c..f0c02d5 100644 (file)
@@ -505,6 +505,9 @@ void DOMWindow::willDestroyDocumentInFrame()
 
 void DOMWindow::willDetachDocumentFromFrame()
 {
+    if (!frame())
+        return;
+
     // It is necessary to copy m_properties to a separate vector because the DOMWindowProperties may
     // unregister themselves from the DOMWindow as a result of the call to willDetachGlobalObjectFromFrame.
     for (auto& property : copyToVector(m_properties))
index cbe45e3..3183949 100644 (file)
@@ -833,7 +833,7 @@ void Frame::disconnectOwnerElement()
     m_ownerElement = nullptr;
 
     if (auto* document = this->document())
-        document->detachFromFrame();
+        document->frameWasDisconnectedFromOwner();
 }
 
 String Frame::displayStringModifiedByEncoding(const String& str) const
index 69008e8..93f0ca6 100644 (file)
@@ -1,3 +1,18 @@
+2018-10-05  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r236862): Crash under DOMWindowExtension::willDetachGlobalObjectFromFrame()
+        https://bugs.webkit.org/show_bug.cgi?id=190320
+        <rdar://problem/45044814>
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic.cpp:
+        (TestWebKitAPI::TEST):
+        * TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic_Bundle.cpp:
+        (TestWebKitAPI::DOMWindowExtensionBasic::willDestroyGlobalObjectForDOMWindowExtension):
+
 2018-10-03  Jer Noble  <jer.noble@apple.com>
 
         Add support for reporting "display composited video frames" through the VideoPlaybackQuality object.
index eae01b0..981f49e 100644 (file)
@@ -135,6 +135,39 @@ TEST(WebKit, DISABLED_DOMWindowExtensionBasic)
         EXPECT_WK_STREQ(expectedMessages[i], messages[i].get());
 }
 
+TEST(WebKit, DOMWindowExtensionCrashOnReload)
+{
+    WKRetainPtr<WKPageGroupRef> pageGroup(AdoptWK, WKPageGroupCreateWithIdentifier(WKStringCreateWithUTF8CString("DOMWindowExtensionBasicPageGroup")));
+
+    WKRetainPtr<WKContextRef> context(AdoptWK, Util::createContextForInjectedBundleTest("DOMWindowExtensionBasic", pageGroup.get()));
+
+    WKContextInjectedBundleClientV0 injectedBundleClient;
+    memset(&injectedBundleClient, 0, sizeof(injectedBundleClient));
+
+    injectedBundleClient.base.version = 0;
+    injectedBundleClient.didReceiveMessageFromInjectedBundle = didReceiveMessageFromInjectedBundle;
+
+    WKContextSetInjectedBundleClient(context.get(), &injectedBundleClient.base);
+
+    // The default cache model has a capacity of 0, so it is necessary to switch to a cache
+    // model that actually allows for a page cache.
+    WKContextSetCacheModel(context.get(), kWKCacheModelDocumentBrowser);
+
+    PlatformWebView webView(context.get(), pageGroup.get());
+
+    finished = false;
+
+    // Make sure the extensions for each frame are installed in each world.
+    WKRetainPtr<WKURLRef> url1(AdoptWK, Util::createURLForResource("simple-iframe", "html"));
+    WKPageLoadURL(webView.page(), url1.get());
+
+    Util::run(&finished);
+    finished = false;
+
+    WKPageReload(webView.page());
+    Util::run(&finished);
+}
+
 } // namespace TestWebKitAPI
 
 #endif
index 9bdc683..184a8c5 100644 (file)
@@ -229,9 +229,6 @@ void DOMWindowExtensionBasic::didReconnectDOMWindowExtensionToGlobalObject(WKBun
 
 void DOMWindowExtensionBasic::willDestroyGlobalObjectForDOMWindowExtension(WKBundleDOMWindowExtensionRef)
 {
-    // All of the items are candidates for the page cache and should not be evicted from the page
-    // cache before the test completes.
-    ASSERT_NOT_REACHED();
 }
 
 static void didFinishLoadForFrameCallback(WKBundlePageRef, WKBundleFrameRef frame, WKTypeRef*, const void *clientInfo)