FrameLoader::open can execute scritps via style recalc in Frame::setDocument
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Aug 2019 05:18:11 +0000 (05:18 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Aug 2019 05:18:11 +0000 (05:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200377

Reviewed by Antti Koivisto.

Source/WebCore:

Fixed the bug that FrameLoader::open can execute arbitrary author scripts via post style update callbacks
by adding PostResolutionCallbackDisabler, WidgetHierarchyUpdatesSuspensionScope, and NavigationDisabler
to CachedFrameBase::restore and FrameLoader::open.

This ensures all frames are restored from the page cache before any of them would start running scripts.

Test: fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update.html

* history/CachedFrame.cpp:
(WebCore::CachedFrameBase::restore):
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::open):
* page/FrameViewLayoutContext.cpp:
(WebCore::FrameViewLayoutContext::layout): Fixed the debug assertion. The layout of a document may be
updated while we're preparing to put a page into the page cache.
* rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::updateCompositingLayers): Ditto.

LayoutTests:

Added a regression test.

* fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update-expected.txt: Added.
* fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update.html: Added.
* platform/win/TestExpectations: Skip the newly added test.

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

LayoutTests/ChangeLog
LayoutTests/fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update-expected.txt [new file with mode: 0644]
LayoutTests/fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update.html [new file with mode: 0644]
LayoutTests/platform/win/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/history/CachedFrame.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/page/FrameViewLayoutContext.cpp
Source/WebCore/rendering/RenderLayerCompositor.cpp

index 60dda5d..ff14730 100644 (file)
@@ -1,3 +1,16 @@
+2019-08-12  Ryosuke Niwa  <rniwa@webkit.org>
+
+        FrameLoader::open can execute scritps via style recalc in Frame::setDocument
+        https://bugs.webkit.org/show_bug.cgi?id=200377
+
+        Reviewed by Antti Koivisto.
+
+        Added a regression test.
+
+        * fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update-expected.txt: Added.
+        * fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update.html: Added.
+        * platform/win/TestExpectations: Skip the newly added test.
+
 2019-08-12  Daniel Bates  <dabates@apple.com>
 
         Add a test to ensure that we dispatch keydown and keyup events when multiple keys are pressed at the same time
diff --git a/LayoutTests/fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update-expected.txt b/LayoutTests/fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update-expected.txt
new file mode 100644 (file)
index 0000000..30ecdfd
--- /dev/null
@@ -0,0 +1,3 @@
+This tests that pageshow event is fired before the object element loads when a document in the page cache is restored.
+
+PASS
diff --git a/LayoutTests/fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update.html b/LayoutTests/fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update.html
new file mode 100644 (file)
index 0000000..a8e5d4c
--- /dev/null
@@ -0,0 +1,102 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests that pageshow event is fired before the object element loads when a document in the page cache is restored.</p>
+<div id="result"></div>
+<script>
+
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+    testRunner.setCanOpenWindows();
+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+    internals.settings.setPageCacheSupportsPlugins(true);
+}
+
+let newWindow;
+function start() {
+    result.textContent = 'Running...';
+    newWindow = window.open(URL.createObjectURL(newPage));
+}
+
+const newPage = new Blob([`<!DOCTYPE html>
+<html>
+<body>
+<p>This is new page.</p>
+<iframe id="iframe2"></iframe>
+<iframe id="iframe1"></iframe>
+<script>
+window.pageShowed = false;
+
+iframe2.onload = () => {
+    opener.postMessage({step: 'opened'}, '*');
+}
+onmessage = () => {
+    opener.postMessage({step: 'ready'}, '*');
+}
+
+iframe1.contentDocument.body.innerHTML = '<span>hello</span>';
+iframe2.src = URL.createObjectURL(opener.subframePage);
+
+</scr` + `ipt>
+</body>
+</html>`], {'type': 'text/html'});
+
+window.subframePage = new Blob([`<!DOCTYPE html>
+<html>
+<body>
+<object id="object1"></object>
+<script>
+window.addEventListener('pagehide', () => {
+    const object1 = window.object1;
+    object1.remove();
+    document.body.appendChild(object1);
+    object1.addEventListener('beforeload', () => {
+        top.opener.postMessage({step: 'check', pageShowed: !!top.iframe1.contentDocument.body.firstChild.offsetHeight}, '*');
+    }, {once: true});
+});
+</sc` + `ript>
+</body>
+</html>`], {'type': 'text/html'});
+
+const secondPage = new Blob([`<!DOCTYPE html>
+<html>
+<body onload="opener.postMessage({step: 'navigated'}, '*')">
+<p>second page.</p>
+</body>
+</html>`], {'type': 'text/html'});
+
+let isLastStep = false;
+onmessage = (event) => {
+    if (isLastStep && event.data.step != 'check')
+        return;
+    switch (event.data.step) {
+    case 'opened':
+        newWindow.postMessage('getready', '*');
+        break;
+    case 'ready':
+        newWindow.location = URL.createObjectURL(secondPage);
+        break;
+    case 'navigated':
+        isLastStep = true;
+        newWindow.history.back();
+        break;
+    case 'check':
+        result.textContent = event.data.pageShowed ? 'PASS' : 'FAIL';
+        newWindow.close();
+        if (window.testRunner)
+            testRunner.notifyDone();
+        break;
+    }
+}
+
+if (window.testRunner)
+    window.onload = start;
+else
+    document.write('<button onclick="start()">Start</button>');
+
+setTimeout(() => testRunner.notifyDone(), 3000);
+
+</script>
+</body>
+</html>
index 1a2aec3..72d634b 100644 (file)
@@ -2299,6 +2299,7 @@ http/tests/history/back-during-onload-triggered-by-back.html [ Skip ] # Timeout
 fast/frames/restoring-page-cache-should-not-run-scripts.html [ Skip ]
 http/tests/security/mixedContent/blob-url-in-iframe.html [ Skip ]
 http/tests/security/contentSecurityPolicy/navigate-self-to-blob.html [ Skip ]
+fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update.html [ Skip ]
 
 # Clear Key not implemented
 http/tests/media/clearkey/clear-key-hls-aes128.html [ Skip ] # Timeout
index ce33d31..9e0ebfc 100644 (file)
@@ -1,3 +1,28 @@
+2019-08-12  Ryosuke Niwa  <rniwa@webkit.org>
+
+        FrameLoader::open can execute scritps via style recalc in Frame::setDocument
+        https://bugs.webkit.org/show_bug.cgi?id=200377
+
+        Reviewed by Antti Koivisto.
+
+        Fixed the bug that FrameLoader::open can execute arbitrary author scripts via post style update callbacks
+        by adding PostResolutionCallbackDisabler, WidgetHierarchyUpdatesSuspensionScope, and NavigationDisabler
+        to CachedFrameBase::restore and FrameLoader::open.
+
+        This ensures all frames are restored from the page cache before any of them would start running scripts.
+
+        Test: fast/frames/restoring-page-cache-should-not-run-scripts-via-style-update.html
+
+        * history/CachedFrame.cpp:
+        (WebCore::CachedFrameBase::restore):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::open):
+        * page/FrameViewLayoutContext.cpp:
+        (WebCore::FrameViewLayoutContext::layout): Fixed the debug assertion. The layout of a document may be
+        updated while we're preparing to put a page into the page cache.
+        * rendering/RenderLayerCompositor.cpp:
+        (WebCore::RenderLayerCompositor::updateCompositingLayers): Ditto.
+
 2019-08-12  Sam Weinig  <weinig@apple.com>
 
         Replace multiparameter overloads of append() in StringBuilder as a first step toward standardizinging on the flexibleAppend() implementation
index daae931..6ae552c 100644 (file)
 #include "FrameLoaderClient.h"
 #include "FrameView.h"
 #include "Logging.h"
+#include "NavigationDisabler.h"
 #include "Page.h"
 #include "PageCache.h"
+#include "RenderWidget.h"
 #include "RuntimeEnabledFeatures.h"
 #include "SVGDocumentExtensions.h"
 #include "ScriptController.h"
 #include "SerializedScriptValue.h"
+#include "StyleTreeResolver.h"
 #include <wtf/RefCountedLeakCounter.h>
 #include <wtf/text/CString.h>
 
@@ -93,44 +96,50 @@ void CachedFrameBase::restore()
     if (m_isMainFrame)
         m_view->setParentVisible(true);
 
-    Frame& frame = m_view->frame();
-    m_cachedFrameScriptData->restore(frame);
+    auto frame = makeRef(m_view->frame());
+    {
+        Style::PostResolutionCallbackDisabler disabler(*m_document);
+        WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
+        NavigationDisabler disableNavigation { nullptr }; // Disable navigation globally.
 
-    if (m_document->svgExtensions())
-        m_document->accessSVGExtensions().unpauseAnimations();
+        m_cachedFrameScriptData->restore(frame.get());
 
-    m_document->resume(ReasonForSuspension::PageCache);
+        if (m_document->svgExtensions())
+            m_document->accessSVGExtensions().unpauseAnimations();
 
-    // It is necessary to update any platform script objects after restoring the
-    // cached page.
-    frame.script().updatePlatformScriptObjects();
+        m_document->resume(ReasonForSuspension::PageCache);
 
-    frame.loader().client().didRestoreFromPageCache();
+        // It is necessary to update any platform script objects after restoring the
+        // cached page.
+        frame->script().updatePlatformScriptObjects();
 
-    pruneDetachedChildFrames();
+        frame->loader().client().didRestoreFromPageCache();
 
-    // Reconstruct the FrameTree. And open the child CachedFrames in their respective FrameLoaders.
-    for (auto& childFrame : m_childFrames) {
-        ASSERT(childFrame->view()->frame().page());
-        frame.tree().appendChild(childFrame->view()->frame());
-        childFrame->open();
-        ASSERT_WITH_SECURITY_IMPLICATION(m_document == frame.document());
+        pruneDetachedChildFrames();
+
+        // Reconstruct the FrameTree. And open the child CachedFrames in their respective FrameLoaders.
+        for (auto& childFrame : m_childFrames) {
+            ASSERT(childFrame->view()->frame().page());
+            frame->tree().appendChild(childFrame->view()->frame());
+            childFrame->open();
+            ASSERT_WITH_SECURITY_IMPLICATION(m_document == frame->document());
+        }
     }
 
 #if PLATFORM(IOS_FAMILY)
     if (m_isMainFrame) {
-        frame.loader().client().didRestoreFrameHierarchyForCachedFrame();
+        frame->loader().client().didRestoreFrameHierarchyForCachedFrame();
 
         if (DOMWindow* domWindow = m_document->domWindow()) {
             // FIXME: Add SCROLL_LISTENER to the list of event types on Document, and use m_document->hasListenerType(). See <rdar://problem/9615482>.
             // FIXME: Can use Document::hasListenerType() now.
-            if (domWindow->scrollEventListenerCount() && frame.page())
-                frame.page()->chrome().client().setNeedsScrollNotifications(frame, true);
+            if (domWindow->scrollEventListenerCount() && frame->page())
+                frame->page()->chrome().client().setNeedsScrollNotifications(frame, true);
         }
     }
 #endif
 
-    frame.view()->didRestoreFromPageCache();
+    frame->view()->didRestoreFromPageCache();
 }
 
 CachedFrame::CachedFrame(Frame& frame)
index 26cf2b3..e0f4614 100644 (file)
 #include "SerializedScriptValue.h"
 #include "Settings.h"
 #include "ShouldTreatAsContinuingLoad.h"
+#include "StyleTreeResolver.h"
 #include "SubframeLoader.h"
 #include "SubresourceLoader.h"
 #include "TextResourceDecoder.h"
@@ -2297,11 +2298,10 @@ void FrameLoader::open(CachedFrameBase& cachedFrame)
         url.setPath("/");
 
     started();
-    Document* document = cachedFrame.document();
-    ASSERT(document);
+    auto document = makeRef(*cachedFrame.document());
     ASSERT(document->domWindow());
 
-    clear(document, true, true, cachedFrame.isMainFrame());
+    clear(document.ptr(), true, true, cachedFrame.isMainFrame());
 
     document->attachToCachedFrame(cachedFrame);
     document->setPageCacheState(Document::NotInPageCache);
@@ -2324,13 +2324,15 @@ void FrameLoader::open(CachedFrameBase& cachedFrame)
     if (previousViewFrameRect)
         view->setFrameRect(previousViewFrameRect.value());
 
-    {
-        // Setting the document builds the render tree and runs post style resolution callbacks that can do anything,
-        // including loading a child frame before its been re-attached to the frame tree as part of this restore.
-        // For example, the HTML object element may load its content into a frame in a post style resolution callback.
-        NavigationDisabler disableNavigation { &m_frame };
-        m_frame.setDocument(document);
-    }
+
+    // Setting the document builds the render tree and runs post style resolution callbacks that can do anything,
+    // including loading a child frame before its been re-attached to the frame tree as part of this restore.
+    // For example, the HTML object element may load its content into a frame in a post style resolution callback.
+    Style::PostResolutionCallbackDisabler disabler(document.get());
+    WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
+    NavigationDisabler disableNavigation { &m_frame };
+    
+    m_frame.setDocument(document.copyRef());
 
     document->domWindow()->resumeFromPageCache();
 
index 2c53015..73098f1 100644 (file)
@@ -145,7 +145,8 @@ void FrameViewLayoutContext::layout()
     ASSERT(!view().isPainting());
     ASSERT(frame().view() == &view());
     ASSERT(frame().document());
-    ASSERT(frame().document()->pageCacheState() == Document::NotInPageCache);
+    ASSERT(frame().document()->pageCacheState() == Document::NotInPageCache
+        || frame().document()->pageCacheState() == Document::AboutToEnterPageCache);
     if (!canPerformLayout()) {
         LOG(Layout, "  is not allowed, bailing");
         return;
index 4d2246d..abe4d4d 100644 (file)
@@ -691,7 +691,8 @@ bool RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType update
 
     m_updateCompositingLayersTimer.stop();
 
-    ASSERT(m_renderView.document().pageCacheState() == Document::NotInPageCache);
+    ASSERT(m_renderView.document().pageCacheState() == Document::NotInPageCache
+        || m_renderView.document().pageCacheState() == Document::AboutToEnterPageCache);
     
     // Compositing layers will be updated in Document::setVisualUpdatesAllowed(bool) if suppressed here.
     if (!m_renderView.document().visualUpdatesAllowed())