- Revision
- 236888
- Author
- [email protected]
- Date
- 2018-10-05 15:25:49 -0700 (Fri, 05 Oct 2018)
Log Message
Regression(r236862): Crash under DOMWindowExtension::willDetachGlobalObjectFromFrame()
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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (236887 => 236888)
--- trunk/Source/WebCore/ChangeLog 2018-10-05 22:14:12 UTC (rev 236887)
+++ trunk/Source/WebCore/ChangeLog 2018-10-05 22:25:49 UTC (rev 236888)
@@ -1,3 +1,39 @@
+2018-10-05 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Further unreviewed watchOS build fix: videoPerformanceMetrics unavailable on watchOS.
Modified: trunk/Source/WebCore/dom/Document.cpp (236887 => 236888)
--- trunk/Source/WebCore/dom/Document.cpp 2018-10-05 22:14:12 UTC (rev 236887)
+++ trunk/Source/WebCore/dom/Document.cpp 2018-10-05 22:25:49 UTC (rev 236888)
@@ -8256,10 +8256,18 @@
void Document::detachFromFrame()
{
+ observeFrame(nullptr);
+}
+
+void Document::frameWasDisconnectedFromOwner()
+{
+ if (!frame())
+ return;
+
if (auto* window = domWindow())
window->willDetachDocumentFromFrame();
- observeFrame(nullptr);
+ detachFromFrame();
}
} // namespace WebCore
Modified: trunk/Source/WebCore/dom/Document.h (236887 => 236888)
--- trunk/Source/WebCore/dom/Document.h 2018-10-05 22:14:12 UTC (rev 236887)
+++ trunk/Source/WebCore/dom/Document.h 2018-10-05 22:25:49 UTC (rev 236888)
@@ -1505,7 +1505,7 @@
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 @@
void pendingTasksTimerFired();
bool isCookieAverse() const;
+ void detachFromFrame();
+
template<CollectionType> Ref<HTMLCollection> ensureCachedCollection();
#if ENABLE(FULLSCREEN_API)
Modified: trunk/Source/WebCore/page/DOMWindow.cpp (236887 => 236888)
--- trunk/Source/WebCore/page/DOMWindow.cpp 2018-10-05 22:14:12 UTC (rev 236887)
+++ trunk/Source/WebCore/page/DOMWindow.cpp 2018-10-05 22:25:49 UTC (rev 236888)
@@ -505,6 +505,9 @@
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))
Modified: trunk/Source/WebCore/page/Frame.cpp (236887 => 236888)
--- trunk/Source/WebCore/page/Frame.cpp 2018-10-05 22:14:12 UTC (rev 236887)
+++ trunk/Source/WebCore/page/Frame.cpp 2018-10-05 22:25:49 UTC (rev 236888)
@@ -833,7 +833,7 @@
m_ownerElement = nullptr;
if (auto* document = this->document())
- document->detachFromFrame();
+ document->frameWasDisconnectedFromOwner();
}
String Frame::displayStringModifiedByEncoding(const String& str) const
Modified: trunk/Tools/ChangeLog (236887 => 236888)
--- trunk/Tools/ChangeLog 2018-10-05 22:14:12 UTC (rev 236887)
+++ trunk/Tools/ChangeLog 2018-10-05 22:25:49 UTC (rev 236888)
@@ -1,3 +1,18 @@
+2018-10-05 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Add support for reporting "display composited video frames" through the VideoPlaybackQuality object.
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic.cpp (236887 => 236888)
--- trunk/Tools/TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic.cpp 2018-10-05 22:14:12 UTC (rev 236887)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic.cpp 2018-10-05 22:25:49 UTC (rev 236888)
@@ -135,6 +135,39 @@
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
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic_Bundle.cpp (236887 => 236888)
--- trunk/Tools/TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic_Bundle.cpp 2018-10-05 22:14:12 UTC (rev 236887)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKit/DOMWindowExtensionBasic_Bundle.cpp 2018-10-05 22:25:49 UTC (rev 236888)
@@ -229,9 +229,6 @@
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)
