Prevent reentrancy FrameLoader::dispatchUnloadEvents()
authorajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Sep 2019 14:52:18 +0000 (14:52 +0000)
committerajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Sep 2019 14:52:18 +0000 (14:52 +0000)
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.

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

Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h
Source/WebCore/loader/FrameLoaderTypes.h

index 5a21c46..bfb5dd8 100644 (file)
@@ -1,3 +1,32 @@
+2019-09-11  Ali Juma  <ajuma@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  <cturner@igalia.com>
 
         [GStreamer] Do not adopt floating references.
index 00081cc..13e14d5 100644 (file)
@@ -1806,12 +1806,12 @@ void FrameLoader::reload(OptionSet<ReloadOption> options)
     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 @@ void FrameLoader::detachFromParent()
         // 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 @@ void FrameLoader::dispatchUnloadEvents(UnloadEventPolicy unloadEventPolicy)
     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 @@ bool FrameLoader::dispatchBeforeUnloadEvent(Chrome& chrome, FrameLoader* frameLo
         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());
 
index 30a6ff6..682d36f 100644 (file)
@@ -145,7 +145,7 @@ public:
 
     // 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);
index 7a48bc6..3ba1632 100644 (file)
@@ -141,6 +141,11 @@ enum ClearProvisionalItemPolicy {
     ShouldNotClearProvisionalItem
 };
 
+enum class StopLoadingPolicy {
+    PreventDuringUnloadEvents,
+    AlwaysStopLoading
+};
+
 enum class ObjectContentType : uint8_t {
     None,
     Image,