Crash in DOMWindowExtension::suspendForPageCache
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Feb 2019 00:06:27 +0000 (00:06 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Feb 2019 00:06:27 +0000 (00:06 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194871

Reviewed by Chris Dumez.

This is a speculative fix for a crash in DOMWindowExtension::suspendForPageCache.

We think it's possible for DOMWindowExtension::suspendForPageCache notifying the clients via
dispatchWillDisconnectDOMWindowExtensionFromGlobalObject to remove other DOMWindowExtension's.
Check that each DOMWindowProperty is still in m_properties before invoking suspendForPageCache
to avoid the crash.

* page/DOMWindow.cpp:
(WebCore::DOMWindow::willDestroyCachedFrame):
(WebCore::DOMWindow::willDestroyDocumentInFrame):
(WebCore::DOMWindow::willDetachDocumentFromFrame):
(WebCore::DOMWindow::suspendForPageCache):
(WebCore::DOMWindow::resumeFromPageCache):
* page/DOMWindowExtension.cpp:
(WebCore::DOMWindowExtension::suspendForPageCache):

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

Source/WebCore/ChangeLog
Source/WebCore/page/DOMWindow.cpp
Source/WebCore/page/DOMWindowExtension.cpp

index 7a6b613..bc8218f 100644 (file)
@@ -1,3 +1,26 @@
+2019-02-20  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Crash in DOMWindowExtension::suspendForPageCache
+        https://bugs.webkit.org/show_bug.cgi?id=194871
+
+        Reviewed by Chris Dumez.
+
+        This is a speculative fix for a crash in DOMWindowExtension::suspendForPageCache.
+
+        We think it's possible for DOMWindowExtension::suspendForPageCache notifying the clients via
+        dispatchWillDisconnectDOMWindowExtensionFromGlobalObject to remove other DOMWindowExtension's.
+        Check that each DOMWindowProperty is still in m_properties before invoking suspendForPageCache
+        to avoid the crash.
+
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::willDestroyCachedFrame):
+        (WebCore::DOMWindow::willDestroyDocumentInFrame):
+        (WebCore::DOMWindow::willDetachDocumentFromFrame):
+        (WebCore::DOMWindow::suspendForPageCache):
+        (WebCore::DOMWindow::resumeFromPageCache):
+        * page/DOMWindowExtension.cpp:
+        (WebCore::DOMWindowExtension::suspendForPageCache):
+
 2019-02-20  Alex Christensen  <achristensen@webkit.org>
 
         Always call CompletionHandlers after r240909
index a7db07b..8fd4119 100644 (file)
@@ -456,16 +456,20 @@ void DOMWindow::willDestroyCachedFrame()
 {
     // 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 willDestroyGlobalObjectInCachedFrame.
-    for (auto& property : copyToVector(m_properties))
-        property->willDestroyGlobalObjectInCachedFrame();
+    for (auto* property : copyToVector(m_properties)) {
+        if (m_properties.contains(property))
+            property->willDestroyGlobalObjectInCachedFrame();
+    }
 }
 
 void DOMWindow::willDestroyDocumentInFrame()
 {
     // 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 willDestroyGlobalObjectInFrame.
-    for (auto& property : copyToVector(m_properties))
-        property->willDestroyGlobalObjectInFrame();
+    for (auto* property : copyToVector(m_properties)) {
+        if (m_properties.contains(property))
+            property->willDestroyGlobalObjectInFrame();
+    }
 }
 
 void DOMWindow::willDetachDocumentFromFrame()
@@ -475,8 +479,10 @@ void DOMWindow::willDetachDocumentFromFrame()
 
     // 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))
-        property->willDetachGlobalObjectFromFrame();
+    for (auto& property : copyToVector(m_properties)) {
+        if (m_properties.contains(property))
+            property->willDetachGlobalObjectFromFrame();
+    }
 
     if (m_performance)
         m_performance->clearResourceTimings();
@@ -520,16 +526,20 @@ void DOMWindow::resetUnlessSuspendedForDocumentSuspension()
 
 void DOMWindow::suspendForPageCache()
 {
-    for (auto& property : copyToVector(m_properties))
-        property->suspendForPageCache();
+    for (auto* property : copyToVector(m_properties)) {
+        if (m_properties.contains(property))
+            property->suspendForPageCache();
+    }
 
     m_suspendedForDocumentSuspension = true;
 }
 
 void DOMWindow::resumeFromPageCache()
 {
-    for (auto& property : copyToVector(m_properties))
-        property->resumeFromPageCache();
+    for (auto* property : copyToVector(m_properties)) {
+        if (m_properties.contains(property))
+            property->resumeFromPageCache();
+    }
 
     m_suspendedForDocumentSuspension = false;
 }
index 07c5d05..c4aaaad 100644 (file)
@@ -48,11 +48,11 @@ void DOMWindowExtension::suspendForPageCache()
     // Calling out to the client might result in this DOMWindowExtension being destroyed
     // while there is still work to do.
     Ref<DOMWindowExtension> protectedThis(*this);
-    
-    Frame* frame = this->frame();
+
+    auto frame = makeRef(*this->frame());
     frame->loader().client().dispatchWillDisconnectDOMWindowExtensionFromGlobalObject(this);
 
-    m_disconnectedFrame = frame;
+    m_disconnectedFrame = WTFMove(frame);
 
     DOMWindowProperty::suspendForPageCache();
 }