Disallow navigations when page cache updates the current document of the frame
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Aug 2018 16:50:20 +0000 (16:50 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Aug 2018 16:50:20 +0000 (16:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188422

Reviewed by Ryosuke Niwa.

Source/WebCore:

Make use of NavigationDisabler to disallow navigations when associating the cached
document back with its frame (i.e. calling Frame::setDocument()).

When we associate a cached document with its frame we will construct its render tree
and run post style resolution callbacks that can do anything, including performing
a frame load. Until page restoration is comnplete the frame tree is in a transient
state that makes reasoning about it difficult and error prone. We should not allow
navigations in this state.

Test: fast/history/go-back-to-object-subframe.html

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::open):

LayoutTests:

Add a test case that ensures that we do not hit the assertion ASSERT(ownerFrame || m_frame.isMainFrame())
in FrameLoader::addExtraFieldsToRequest() when navigating back to a page that loads a nested
page, whose URL contains a fragment, via an HTML object element. This assertion fails if
navigations are allowed when restoring a page from the page cache.

This change does not prevent navigations initiated from a pageshow event handler.

* fast/history/go-back-to-object-subframe-expected.txt: Added.
* fast/history/go-back-to-object-subframe.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/history/go-back-to-object-subframe-expected.txt [new file with mode: 0644]
LayoutTests/fast/history/go-back-to-object-subframe.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp

index 8f4831c..e8948f4 100644 (file)
@@ -1,5 +1,22 @@
 2018-08-21  Daniel Bates  <dabates@apple.com>
 
+        Disallow navigations when page cache updates the current document of the frame
+        https://bugs.webkit.org/show_bug.cgi?id=188422
+
+        Reviewed by Ryosuke Niwa.
+
+        Add a test case that ensures that we do not hit the assertion ASSERT(ownerFrame || m_frame.isMainFrame())
+        in FrameLoader::addExtraFieldsToRequest() when navigating back to a page that loads a nested
+        page, whose URL contains a fragment, via an HTML object element. This assertion fails if
+        navigations are allowed when restoring a page from the page cache.
+
+        This change does not prevent navigations initiated from a pageshow event handler.
+
+        * fast/history/go-back-to-object-subframe-expected.txt: Added.
+        * fast/history/go-back-to-object-subframe.html: Added.
+
+2018-08-21  Daniel Bates  <dabates@apple.com>
+
         [iOS][WK1] Support toggling continuous spell checking from tests
         https://bugs.webkit.org/show_bug.cgi?id=188763
 
diff --git a/LayoutTests/fast/history/go-back-to-object-subframe-expected.txt b/LayoutTests/fast/history/go-back-to-object-subframe-expected.txt
new file mode 100644 (file)
index 0000000..e66bdd3
--- /dev/null
@@ -0,0 +1,2 @@
+PASS. You didn't crash.
+
diff --git a/LayoutTests/fast/history/go-back-to-object-subframe.html b/LayoutTests/fast/history/go-back-to-object-subframe.html
new file mode 100644 (file)
index 0000000..580a42f
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+    testRunner.waitUntilDone();
+}
+
+function runTest(e)
+{
+    if (e.persisted) {
+        if (window.testRunner)
+            testRunner.notifyDone();
+    } else {
+        // Navigate using a timeout to make sure we generate a history entry that we can go back to.
+        setTimeout(() => { location.href = "data:text/html,<script>history.back()</" + "script>"; }, 0);
+    }
+}
+
+window.onpageshow = runTest;
+</script>
+</head>
+<body>
+<div>PASS. You didn't crash.</div>
+<object data="resources/subframe.html#dummy"  width="400" height="200" style="border: 1px solid black"></object>
+</body>
+</html>
index e46193a..f05937a 100644 (file)
@@ -1,5 +1,26 @@
 2018-08-21  Daniel Bates  <dabates@apple.com>
 
+        Disallow navigations when page cache updates the current document of the frame
+        https://bugs.webkit.org/show_bug.cgi?id=188422
+
+        Reviewed by Ryosuke Niwa.
+
+        Make use of NavigationDisabler to disallow navigations when associating the cached
+        document back with its frame (i.e. calling Frame::setDocument()).
+
+        When we associate a cached document with its frame we will construct its render tree
+        and run post style resolution callbacks that can do anything, including performing
+        a frame load. Until page restoration is comnplete the frame tree is in a transient
+        state that makes reasoning about it difficult and error prone. We should not allow
+        navigations in this state.
+
+        Test: fast/history/go-back-to-object-subframe.html
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::open):
+
+2018-08-21  Daniel Bates  <dabates@apple.com>
+
         Replace TextCheckingTypeMask with OptionSet
         https://bugs.webkit.org/show_bug.cgi?id=188678
 
index 908b32a..1598eef 100644 (file)
@@ -2270,8 +2270,15 @@ void FrameLoader::open(CachedFrameBase& cachedFrame)
     // Use the previous ScrollView's frame rect.
     if (previousViewFrameRect)
         view->setFrameRect(previousViewFrameRect.value());
-    
-    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.
+        NavigationDisabler disableNavigation { &m_frame };
+        m_frame.setDocument(document);
+    }
+
     document->domWindow()->resumeFromDocumentSuspension();
 
     updateFirstPartyForCookies();