Title: [249762] trunk/Source/WebCore
Revision
249762
Author
aj...@chromium.org
Date
2019-09-11 07:52:18 -0700 (Wed, 11 Sep 2019)

Log Message

Prevent reentrancy FrameLoader::dispatchUnloadEvents()
https://bugs.webkit.org/show_bug.cgi?id=200738

Reviewed by Brady Eidson.

Reentrancy causes m_pageDismissalEventBeingDispatched to be incorrectly
updated, so don't allow reentrancy.

Since this prevents m_pageDismissalEventBeingDispatched from being reset
inside a reentrant call, it can have the unintended effect of causing
FrameLoader::stopAllLoaders to early-out when called from
FrameLoader::detachFromParent while a frame's unload event handler
calls document.open() on a parent frame and causes itself to become
detached. Allowing a load to continue in a detached frame will lead to
a crash. To prevent this, add a new argument to FrameLoader::stopAllLoaders
that FrameLoader::detachFromParent can use to prevent an early-out.

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::stopAllLoaders):
(WebCore::FrameLoader::detachFromParent):
(WebCore::FrameLoader::dispatchUnloadEvents):
(WebCore::FrameLoader::dispatchBeforeUnloadEvent):
Ensure that m_pageDismissalEventBeingDispatched is reset to its previous value, even if this is not None.
* loader/FrameLoader.h:
* loader/FrameLoaderTypes.h:
Add a StopLoadingPolicy enum.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (249761 => 249762)


--- trunk/Source/WebCore/ChangeLog	2019-09-11 11:49:14 UTC (rev 249761)
+++ trunk/Source/WebCore/ChangeLog	2019-09-11 14:52:18 UTC (rev 249762)
@@ -1,3 +1,32 @@
+2019-09-11  Ali Juma  <aj...@chromium.org>
+
+        Prevent reentrancy FrameLoader::dispatchUnloadEvents()
+        https://bugs.webkit.org/show_bug.cgi?id=200738
+
+        Reviewed by Brady Eidson.
+
+        Reentrancy causes m_pageDismissalEventBeingDispatched to be incorrectly
+        updated, so don't allow reentrancy.
+
+        Since this prevents m_pageDismissalEventBeingDispatched from being reset
+        inside a reentrant call, it can have the unintended effect of causing
+        FrameLoader::stopAllLoaders to early-out when called from
+        FrameLoader::detachFromParent while a frame's unload event handler
+        calls document.open() on a parent frame and causes itself to become
+        detached. Allowing a load to continue in a detached frame will lead to
+        a crash. To prevent this, add a new argument to FrameLoader::stopAllLoaders
+        that FrameLoader::detachFromParent can use to prevent an early-out.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::stopAllLoaders):
+        (WebCore::FrameLoader::detachFromParent):
+        (WebCore::FrameLoader::dispatchUnloadEvents):
+        (WebCore::FrameLoader::dispatchBeforeUnloadEvent):
+        Ensure that m_pageDismissalEventBeingDispatched is reset to its previous value, even if this is not None.
+        * loader/FrameLoader.h:
+        * loader/FrameLoaderTypes.h:
+        Add a StopLoadingPolicy enum.
+
 2019-09-11  Charlie Turner  <ctur...@igalia.com>
 
         [GStreamer] Do not adopt floating references.

Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (249761 => 249762)


--- trunk/Source/WebCore/loader/FrameLoader.cpp	2019-09-11 11:49:14 UTC (rev 249761)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp	2019-09-11 14:52:18 UTC (rev 249762)
@@ -1806,12 +1806,12 @@
     loadWithDocumentLoader(loader.ptr(), frameLoadTypeForReloadOptions(options), { }, AllowNavigationToInvalidURL::Yes, ShouldTreatAsContinuingLoad::No);
 }
 
-void FrameLoader::stopAllLoaders(ClearProvisionalItemPolicy clearProvisionalItemPolicy)
+void FrameLoader::stopAllLoaders(ClearProvisionalItemPolicy clearProvisionalItemPolicy, StopLoadingPolicy stopLoadingPolicy)
 {
     if (m_frame.document() && m_frame.document()->pageCacheState() == Document::InPageCache)
         return;
 
-    if (!isStopLoadingAllowed())
+    if (stopLoadingPolicy == StopLoadingPolicy::PreventDuringUnloadEvents && !isStopLoadingAllowed())
         return;
 
     // If this method is called from within this method, infinite recursion can occur (3442218). Avoid this.
@@ -2820,7 +2820,7 @@
         // stopAllLoaders() needs to be called after detachChildren() if the document is not in the page cache,
         // because detachedChildren() will trigger the unload event handlers of any child frames, and those event
         // handlers might start a new subresource load in this frame.
-        stopAllLoaders();
+        stopAllLoaders(ShouldClearProvisionalItem, StopLoadingPolicy::AlwaysStopLoading);
     }
 
     InspectorInstrumentation::frameDetachedFromParent(m_frame);
@@ -3273,6 +3273,9 @@
     if (!m_frame.document())
         return;
 
+    if (m_pageDismissalEventBeingDispatched != PageDismissalType::None)
+        return;
+
     // We store the frame's page in a local variable because the frame might get detached inside dispatchEvent.
     ForbidPromptsScope forbidPrompts(m_frame.page());
     IgnoreOpensDuringUnloadCountIncrementer ignoreOpensDuringUnloadCountIncrementer(m_frame.document());
@@ -3351,15 +3354,13 @@
         return true;
     
     Ref<BeforeUnloadEvent> beforeUnloadEvent = BeforeUnloadEvent::create();
-    m_pageDismissalEventBeingDispatched = PageDismissalType::BeforeUnload;
 
     {
+        SetForScope<PageDismissalType> change(m_pageDismissalEventBeingDispatched, PageDismissalType::BeforeUnload);
         ForbidPromptsScope forbidPrompts(m_frame.page());
         domWindow->dispatchEvent(beforeUnloadEvent, domWindow->document());
     }
 
-    m_pageDismissalEventBeingDispatched = PageDismissalType::None;
-
     if (!beforeUnloadEvent->defaultPrevented())
         document->defaultEventHandler(beforeUnloadEvent.get());
 

Modified: trunk/Source/WebCore/loader/FrameLoader.h (249761 => 249762)


--- trunk/Source/WebCore/loader/FrameLoader.h	2019-09-11 11:49:14 UTC (rev 249761)
+++ trunk/Source/WebCore/loader/FrameLoader.h	2019-09-11 14:52:18 UTC (rev 249762)
@@ -145,7 +145,7 @@
 
     // FIXME: These are all functions which stop loads. We have too many.
     void stopAllLoadersAndCheckCompleteness();
-    WEBCORE_EXPORT void stopAllLoaders(ClearProvisionalItemPolicy = ShouldClearProvisionalItem);
+    WEBCORE_EXPORT void stopAllLoaders(ClearProvisionalItemPolicy = ShouldClearProvisionalItem, StopLoadingPolicy = StopLoadingPolicy::PreventDuringUnloadEvents);
     WEBCORE_EXPORT void stopForUserCancel(bool deferCheckLoadComplete = false);
     void stop();
     void stopLoading(UnloadEventPolicy);

Modified: trunk/Source/WebCore/loader/FrameLoaderTypes.h (249761 => 249762)


--- trunk/Source/WebCore/loader/FrameLoaderTypes.h	2019-09-11 11:49:14 UTC (rev 249761)
+++ trunk/Source/WebCore/loader/FrameLoaderTypes.h	2019-09-11 14:52:18 UTC (rev 249762)
@@ -141,6 +141,11 @@
     ShouldNotClearProvisionalItem
 };
 
+enum class StopLoadingPolicy {
+    PreventDuringUnloadEvents,
+    AlwaysStopLoading
+};
+
 enum class ObjectContentType : uint8_t {
     None,
     Image,
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to