Layout Test fast/events/beforeunload-dom-manipulation-crash.html is crashing
authorpvollan@apple.com <pvollan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 Feb 2018 04:18:38 +0000 (04:18 +0000)
committerpvollan@apple.com <pvollan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 Feb 2018 04:18:38 +0000 (04:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181204
<rdar://problem/36256274>

Reviewed by Ryosuke Niwa.

Source/WebCore:

When a frame element is moved in the DOM tree during the execution of a beforeunload handler,
the frame will be detached when removed from its previous position in the DOM tree. When being
detached, an attempt will also be made to stop the load by calling FrameLoader::stopAllLoaders().
However, this method will return early when executed in a beforeunload handler, since navigation
is not allowed then. The end result is a detached frame which will continue to load, and hitting
asserts in DocumentLoader::dataReceived(), and DocumentLoader::notifyFinished(). It should be
possible to stop a frame load, even when executing a beforeunload handler.

No new tests. Covered by the existing test fast/events/beforeunload-dom-manipulation-crash.html.

* history/PageCache.cpp:
(WebCore::PageCache::addIfCacheable): Fix a failing API test by allowing scripts to be executed
under the PageCache::prune method.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::isStopLoadingAllowed const):
(WebCore::FrameLoader::stopAllLoaders):
* loader/FrameLoader.h:
* svg/graphics/SVGImage.cpp:
(WebCore::SVGImage::~SVGImage): Disable scripts disallowed assertions in this scope, since it is
safe in this context.

Tools:

Implement 'testRunner.forceImmediateCompletion()' for WK1.

* DumpRenderTree/TestRunner.cpp:
(forceImmediateCompletionCallback):
(TestRunner::staticFunctions):

LayoutTests:

* fast/events/beforeunload-dom-manipulation-crash.html: Make it clear that the
frame element is a child of the 'del' element.
* fast/events/beforeunload-dom-manipulation-crash-expected.html:
* platform/mac-wk1/TestExpectations: Unskip test.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/beforeunload-dom-manipulation-crash-expected.txt
LayoutTests/fast/events/beforeunload-dom-manipulation-crash.html
LayoutTests/platform/mac-wk1/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/history/PageCache.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h
Source/WebCore/svg/graphics/SVGImage.cpp
Tools/ChangeLog
Tools/DumpRenderTree/TestRunner.cpp

index 18acbc6..ac43266 100644 (file)
@@ -1,3 +1,16 @@
+2018-01-31  Per Arne Vollan  <pvollan@apple.com>
+
+        Layout Test fast/events/beforeunload-dom-manipulation-crash.html is crashing
+        https://bugs.webkit.org/show_bug.cgi?id=181204
+        <rdar://problem/36256274>
+
+        Reviewed by Ryosuke Niwa.
+
+        * fast/events/beforeunload-dom-manipulation-crash.html: Make it clear that the
+        frame element is a child of the 'del' element.
+        * fast/events/beforeunload-dom-manipulation-crash-expected.html:
+        * platform/mac-wk1/TestExpectations: Unskip test.
+
 2018-01-31  Javier Fernandez  <jfernandez@igalia.com>
 
         inline-block baseline not computed correctly for vertical-lr
index 8714ac4..a4fd9d8 100644 (file)
@@ -23,5 +23,6 @@ function nextStep() {
 <body onload="runTest()">
     <p>This test passes if it does not crash.</p>
     <del id="del">
-    <iframe id="iframe"></iframe>
+        <iframe id="iframe"></iframe>
+    </del>
 </body>
index 1227dcd..ad56fd4 100644 (file)
@@ -453,8 +453,6 @@ webkit.org/b/176920 imported/w3c/web-platform-tests/streams/piping/error-propaga
 
 webkit.org/b/175886 svg/animations/smil-leak-element-instances.svg [ Pass Failure ]
 
-webkit.org/b/177020 fast/events/beforeunload-dom-manipulation-crash.html [ Skip ]
-
 # <rdar://problem/29201698> DumpRenderTree crashed in com.apple.CoreGraphics: CGDataProviderCopyData + 377
 [ HighSierra+ ] fast/canvas/webgl/tex-image-and-sub-image-2d-with-potentially-subsampled-image.html [ Crash ]
 
index 185385f..8ad11df 100644 (file)
@@ -1,3 +1,32 @@
+2018-01-31  Per Arne Vollan  <pvollan@apple.com>
+
+        Layout Test fast/events/beforeunload-dom-manipulation-crash.html is crashing
+        https://bugs.webkit.org/show_bug.cgi?id=181204
+        <rdar://problem/36256274>
+
+        Reviewed by Ryosuke Niwa.
+
+        When a frame element is moved in the DOM tree during the execution of a beforeunload handler,
+        the frame will be detached when removed from its previous position in the DOM tree. When being
+        detached, an attempt will also be made to stop the load by calling FrameLoader::stopAllLoaders().
+        However, this method will return early when executed in a beforeunload handler, since navigation
+        is not allowed then. The end result is a detached frame which will continue to load, and hitting
+        asserts in DocumentLoader::dataReceived(), and DocumentLoader::notifyFinished(). It should be
+        possible to stop a frame load, even when executing a beforeunload handler.
+
+        No new tests. Covered by the existing test fast/events/beforeunload-dom-manipulation-crash.html.
+
+        * history/PageCache.cpp:
+        (WebCore::PageCache::addIfCacheable): Fix a failing API test by allowing scripts to be executed
+        under the PageCache::prune method.
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::isStopLoadingAllowed const):
+        (WebCore::FrameLoader::stopAllLoaders):
+        * loader/FrameLoader.h:
+        * svg/graphics/SVGImage.cpp:
+        (WebCore::SVGImage::~SVGImage): Disable scripts disallowed assertions in this scope, since it is
+        safe in this context.
+
 2018-01-31  Javier Fernandez  <jfernandez@igalia.com>
 
         inline-block baseline not computed correctly for vertical-lr
index b001c47..3365eb7 100644 (file)
@@ -431,13 +431,14 @@ void PageCache::addIfCacheable(HistoryItem& item, Page* page)
 
     setPageCacheState(*page, Document::InPageCache);
 
-    // Make sure we no longer fire any JS events past this point.
-    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+    {
+        // Make sure we don't fire any JS events in this scope.
+        ScriptDisallowedScope::InMainThread scriptDisallowedScope;
 
-    item.m_cachedPage = std::make_unique<CachedPage>(*page);
-    item.m_pruningReason = PruningReason::None;
-    m_items.add(&item);
-    
+        item.m_cachedPage = std::make_unique<CachedPage>(*page);
+        item.m_pruningReason = PruningReason::None;
+        m_items.add(&item);
+    }
     prune(PruningReason::ReachedMaxSize);
 }
 
index 77b2435..c4f5f86 100644 (file)
@@ -1254,6 +1254,11 @@ bool FrameLoader::isNavigationAllowed() const
     return m_pageDismissalEventBeingDispatched == PageDismissalType::None && NavigationDisabler::isNavigationAllowed(m_frame);
 }
 
+bool FrameLoader::isStopLoadingAllowed() const
+{
+    return m_pageDismissalEventBeingDispatched == PageDismissalType::None;
+}
+
 struct SharedBool : public RefCounted<SharedBool> {
     bool value { false };
 };
@@ -1667,13 +1672,16 @@ void FrameLoader::reload(OptionSet<ReloadOption> options)
 void FrameLoader::stopAllLoaders(ClearProvisionalItemPolicy clearProvisionalItemPolicy)
 {
     ASSERT(!m_frame.document() || m_frame.document()->pageCacheState() != Document::InPageCache);
-    if (!isNavigationAllowed())
+    if (!isStopLoadingAllowed())
         return;
 
     // If this method is called from within this method, infinite recursion can occur (3442218). Avoid this.
     if (m_inStopAllLoaders)
         return;
-    
+
+    // This method might dispatch events.
+    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(ScriptDisallowedScope::InMainThread::isScriptAllowed());
+
     // Calling stopLoading() on the provisional document loader can blow away
     // the frame from underneath.
     Ref<Frame> protect(m_frame);
index a6c9819..ede5dcd 100644 (file)
@@ -389,6 +389,7 @@ private:
     void dispatchGlobalObjectAvailableInAllWorlds();
 
     bool isNavigationAllowed() const;
+    bool isStopLoadingAllowed() const;
 
     Frame& m_frame;
     FrameLoaderClient& m_client;
index d76e71e..f707d2b 100644 (file)
@@ -76,6 +76,7 @@ SVGImage::SVGImage(ImageObserver& observer)
 SVGImage::~SVGImage()
 {
     if (m_page) {
+        ScriptDisallowedScope::DisableAssertionsInScope disabledScope;
         // Store m_page in a local variable, clearing m_page, so that SVGImageChromeClient knows we're destructed.
         std::unique_ptr<Page> currentPage = WTFMove(m_page);
         currentPage->mainFrame().loader().frameDetached(); // Break both the loader and view references to the frame
index 7b13585..b2c1236 100644 (file)
@@ -1,3 +1,17 @@
+2018-01-31  Per Arne Vollan  <pvollan@apple.com>
+
+        Layout Test fast/events/beforeunload-dom-manipulation-crash.html is crashing
+        https://bugs.webkit.org/show_bug.cgi?id=181204
+        <rdar://problem/36256274>
+
+        Reviewed by Ryosuke Niwa.
+
+        Implement 'testRunner.forceImmediateCompletion()' for WK1.
+
+        * DumpRenderTree/TestRunner.cpp:
+        (forceImmediateCompletionCallback):
+        (TestRunner::staticFunctions):
+
 2018-01-31  Alex Christensen  <achristensen@webkit.org>
 
         Unreviewed, rolling out r227942.
index 3828990..40afd10 100644 (file)
@@ -1986,6 +1986,13 @@ static JSValueRef simulateWebNotificationClickCallback(JSContextRef context, JSO
     return JSValueMakeUndefined(context);
 }
 
+static JSValueRef forceImmediateCompletionCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
+{
+    TestRunner* controller = static_cast<TestRunner*>(JSObjectGetPrivate(thisObject));
+    controller->forceImmediateCompletion();
+    return JSValueMakeUndefined(context);
+}
+
 static JSValueRef failNextNewCodeBlock(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
 {
     if (argumentCount < 1)
@@ -2242,6 +2249,7 @@ JSStaticFunction* TestRunner::staticFunctions()
         { "imageCountInGeneralPasteboard", imageCountInGeneralPasteboardCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
         { "setSpellCheckerLoggingEnabled", setSpellCheckerLoggingEnabledCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
         { "setOpenPanelFiles", setOpenPanelFilesCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
+        { "forceImmediateCompletion", forceImmediateCompletionCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
         { 0, 0, 0 }
     };