Changeset 249762 in webkit

Timestamp:

Sep 11, 2019 7:52:18 AM (5 years ago)

Author:

ajuma@chromium.org

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.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/FrameLoader.cpp (modified) (4 diffs)
• loader/FrameLoader.h (modified) (1 diff)
• loader/FrameLoaderTypes.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -1807 +1807 @@
 }
 
-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;
 
@@ -2821 +2821 @@
         // 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);
     }
 
@@ -3272 +3272 @@
 {
     if (!m_frame.document())
+        return;
+
+    if (m_pageDismissalEventBeingDispatched != PageDismissalType::None)
         return;
 
@@ -3352 +3355 @@
     
     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())

trunk/Source/WebCore/loader/FrameLoader.h

@@ -146 +146 @@
     // 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();

trunk/Source/WebCore/loader/FrameLoaderTypes.h

@@ -142 +142 @@
 };
 
+enum class StopLoadingPolicy {
+    PreventDuringUnloadEvents,
+    AlwaysStopLoading
+};
+
 enum class ObjectContentType : uint8_t {
     None,