- Revision
- 227280
- Author
- [email protected]
- Date
- 2018-01-21 19:39:36 -0800 (Sun, 21 Jan 2018)
Log Message
Release assertion in canExecuteScript when executing scripts during page cache restore
https://bugs.webkit.org/show_bug.cgi?id=181902
Reviewed by Antti Koivisto.
Source/WebCore:
The crash was caused by an erroneous instantiation of ScriptDisallowedScope::InMainThread in CachedPage::restore.
It can execute arbitrary scripts since CachedFrame::open can update style, layout, and evaluate media queries.
This is fine because there is no way to put this page back into a page cache until the load is commited via
FrameLoader::commitProvisionalLoad is invoked later which only happens after CachedPage::restore had exited.
Also added a release assert to make sure this condition holds.
Tests: fast/history/page-cache-execute-script-during-restore.html
fast/history/page-cache-navigate-during-restore.html
* history/CachedPage.cpp:
(WebCore::CachedPageRestorationScope::CachedPageRestorationScope): Added.
(WebCore::CachedPageRestorationScope::~CachedPageRestorationScope): Added.
(WebCore::CachedPage::restore): Don't instantiate ScriptDisallowedScope::InMainThread. Set isRestoringCachedPage
on the cached pate to release-assert that there won't be any attempt to put this very page back into the cache.
* history/PageCache.cpp:
(WebCore::canCachePage): Added a release assert to make sure the page which is in the process of being restored
from the page cache is not put into the page cache.
* page/Page.h:
(WebCore::Page::setIsRestoringCachedPage): Added.
(WebCore::Page::isRestoringCachedPage const): Added.
LayoutTests:
Added regression tests for the release assertion and navigating while a document is being restored from the page cache.
WebKit should not hit any assertions in either situations.
* fast/history/page-cache-execute-script-during-restore-expected.txt: Added.
* fast/history/page-cache-execute-script-during-restore.html: Added.
* fast/history/page-cache-navigate-during-restore-expected.txt: Added.
* fast/history/page-cache-navigate-during-restore.html: Added.
* fast/history/resources/navigate-back-with-finish-test-stage.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (227279 => 227280)
--- trunk/LayoutTests/ChangeLog 2018-01-22 03:30:35 UTC (rev 227279)
+++ trunk/LayoutTests/ChangeLog 2018-01-22 03:39:36 UTC (rev 227280)
@@ -1,3 +1,19 @@
+2018-01-19 Ryosuke Niwa <[email protected]>
+
+ Release assertion in canExecuteScript when executing scripts during page cache restore
+ https://bugs.webkit.org/show_bug.cgi?id=181902
+
+ Reviewed by Antti Koivisto.
+
+ Added regression tests for the release assertion and navigating while a document is being restored from the page cache.
+ WebKit should not hit any assertions in either situations.
+
+ * fast/history/page-cache-execute-script-during-restore-expected.txt: Added.
+ * fast/history/page-cache-execute-script-during-restore.html: Added.
+ * fast/history/page-cache-navigate-during-restore-expected.txt: Added.
+ * fast/history/page-cache-navigate-during-restore.html: Added.
+ * fast/history/resources/navigate-back-with-finish-test-stage.html: Added.
+
2018-01-21 Jer Noble <[email protected]>
REGRESSION (macOS 10.13.2): imported/w3c/web-platform-tests/media-source/mediasource-* LayoutTests failing
Added: trunk/LayoutTests/fast/history/page-cache-execute-script-during-restore-expected.txt (0 => 227280)
--- trunk/LayoutTests/fast/history/page-cache-execute-script-during-restore-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/history/page-cache-execute-script-during-restore-expected.txt 2018-01-22 03:39:36 UTC (rev 227280)
@@ -0,0 +1,3 @@
+This tests executing a script while being restored from a page cache. WebKit should not hit a release assertion.
+
+PASS
Added: trunk/LayoutTests/fast/history/page-cache-execute-script-during-restore.html (0 => 227280)
--- trunk/LayoutTests/fast/history/page-cache-execute-script-during-restore.html (rev 0)
+++ trunk/LayoutTests/fast/history/page-cache-execute-script-during-restore.html 2018-01-22 03:39:36 UTC (rev 227280)
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests executing a script while being restored from a page cache. WebKit should not hit a release assertion.</p>
+<div id="result">
+<input id="input">
+<button _onclick_="runTest()">Start test</button>
+</div>
+<script>
+
+if (window.testRunner) {
+ testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+ testRunner.clearBackForwardList();
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+window.addEventListener("pageshow", (event) => {
+ if (event.persisted)
+ return;
+ if (window.testRunner)
+ setTimeout(runTest, 0);
+});
+
+function runTest()
+{
+ input.setAttribute('autofocus', '');
+ document.getElementById('input').addEventListener('focus', finish);
+ location.href = '';
+}
+
+function finish()
+{
+ document.getElementById('result').textContent = 'PASS';
+ setTimeout(() => {
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }, 0);
+}
+
+</script>
+</body>
+</html>
Added: trunk/LayoutTests/fast/history/page-cache-navigate-during-restore-expected.txt (0 => 227280)
--- trunk/LayoutTests/fast/history/page-cache-navigate-during-restore-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/history/page-cache-navigate-during-restore-expected.txt 2018-01-22 03:39:36 UTC (rev 227280)
@@ -0,0 +1,3 @@
+This tests navigation while being restored from a page cache. WebKit should not hit any debug assertions.
+
+PASS
Added: trunk/LayoutTests/fast/history/page-cache-navigate-during-restore.html (0 => 227280)
--- trunk/LayoutTests/fast/history/page-cache-navigate-during-restore.html (rev 0)
+++ trunk/LayoutTests/fast/history/page-cache-navigate-during-restore.html 2018-01-22 03:39:36 UTC (rev 227280)
@@ -0,0 +1,67 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests navigation while being restored from a page cache. WebKit should not hit any debug assertions.</p>
+<div id="result">
+<input id="input">
+<button _onclick_="delete sessionStorage.testStage; runTest()">Start test</button>
+</div>
+<script>
+
+if (window.testRunner) {
+ testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+ testRunner.clearBackForwardList();
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+ internals.clearPageCache();
+}
+sessionStorage.testStage = 'initial';
+let persisted = false;
+
+window.addEventListener("pageshow", () => {
+ switch (sessionStorage.testStage) {
+ case 'initial':
+ if (window.testRunner)
+ setTimeout(runTest, 0);
+ break;
+ case 'navigate':
+ location.href = '';
+ break;
+ case 'finish':
+ document.getElementById('result').textContent = persisted ? 'PASS' : 'FAIL - Not put into the page cache';
+ delete sessionStorage.testStage;
+ if (window.testRunner)
+ testRunner.notifyDone();
+ break;
+ }
+});
+
+function runTest()
+{
+ sessionStorage.testStage = 'navigate';
+ input.setAttribute('autofocus', '');
+ document.getElementById('input').addEventListener('focus', navigate);
+ persisted = true;
+ location.href = '';
+}
+
+function navigate()
+{
+ if (sessionStorage.testStage != 'navigate')
+ return;
+ location.href = '';
+ document.getElementById('result').textContent = 'FAIL';
+}
+
+function finish() {
+ document.getElementById('result').textContent = 'PASS';
+ setTimeout(() => {
+ delete sessionStorage.testStage;
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }, 0);
+}
+
+</script>
+</body>
+</html>
Added: trunk/LayoutTests/fast/history/resources/navigate-back-with-finish-test-stage.html (0 => 227280)
--- trunk/LayoutTests/fast/history/resources/navigate-back-with-finish-test-stage.html (rev 0)
+++ trunk/LayoutTests/fast/history/resources/navigate-back-with-finish-test-stage.html 2018-01-22 03:39:36 UTC (rev 227280)
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+window._onload_ = () => {
+ sessionStorage.testStage = 'finish';
+ setTimeout(() => history.back(), 0);
+}
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (227279 => 227280)
--- trunk/Source/WebCore/ChangeLog 2018-01-22 03:30:35 UTC (rev 227279)
+++ trunk/Source/WebCore/ChangeLog 2018-01-22 03:39:36 UTC (rev 227280)
@@ -1,3 +1,33 @@
+2018-01-19 Ryosuke Niwa <[email protected]>
+
+ Release assertion in canExecuteScript when executing scripts during page cache restore
+ https://bugs.webkit.org/show_bug.cgi?id=181902
+
+ Reviewed by Antti Koivisto.
+
+ The crash was caused by an erroneous instantiation of ScriptDisallowedScope::InMainThread in CachedPage::restore.
+ It can execute arbitrary scripts since CachedFrame::open can update style, layout, and evaluate media queries.
+
+ This is fine because there is no way to put this page back into a page cache until the load is commited via
+ FrameLoader::commitProvisionalLoad is invoked later which only happens after CachedPage::restore had exited.
+
+ Also added a release assert to make sure this condition holds.
+
+ Tests: fast/history/page-cache-execute-script-during-restore.html
+ fast/history/page-cache-navigate-during-restore.html
+
+ * history/CachedPage.cpp:
+ (WebCore::CachedPageRestorationScope::CachedPageRestorationScope): Added.
+ (WebCore::CachedPageRestorationScope::~CachedPageRestorationScope): Added.
+ (WebCore::CachedPage::restore): Don't instantiate ScriptDisallowedScope::InMainThread. Set isRestoringCachedPage
+ on the cached pate to release-assert that there won't be any attempt to put this very page back into the cache.
+ * history/PageCache.cpp:
+ (WebCore::canCachePage): Added a release assert to make sure the page which is in the process of being restored
+ from the page cache is not put into the page cache.
+ * page/Page.h:
+ (WebCore::Page::setIsRestoringCachedPage): Added.
+ (WebCore::Page::isRestoringCachedPage const): Added.
+
2018-01-21 Eric Carlson <[email protected]>
Resign NowPlaying status when no media element is eligible
Modified: trunk/Source/WebCore/history/CachedPage.cpp (227279 => 227280)
--- trunk/Source/WebCore/history/CachedPage.cpp 2018-01-22 03:30:35 UTC (rev 227279)
+++ trunk/Source/WebCore/history/CachedPage.cpp 2018-01-22 03:39:36 UTC (rev 227280)
@@ -99,6 +99,23 @@
}
}
+class CachedPageRestorationScope {
+public:
+ CachedPageRestorationScope(Page& page)
+ : m_page(page)
+ {
+ m_page.setIsRestoringCachedPage(true);
+ }
+
+ ~CachedPageRestorationScope()
+ {
+ m_page.setIsRestoringCachedPage(false);
+ }
+
+private:
+ Page& m_page;
+};
+
void CachedPage::restore(Page& page)
{
ASSERT(m_cachedMainFrame);
@@ -105,14 +122,9 @@
ASSERT(m_cachedMainFrame->view()->frame().isMainFrame());
ASSERT(!page.subframeCount());
- {
- // Do not dispatch DOM events as their _javascript_ listeners could cause the page to be put
- // into the page cache before we have finished restoring it from the page cache.
- ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+ CachedPageRestorationScope restorationScope(page);
+ m_cachedMainFrame->open();
- m_cachedMainFrame->open();
- }
-
// Restore the focus appearance for the focused element.
// FIXME: Right now we don't support pages w/ frames in the b/f cache. This may need to be tweaked when we add support for that.
Document* focusedDocument = page.focusController().focusedOrMainFrame().document();
Modified: trunk/Source/WebCore/history/PageCache.cpp (227279 => 227280)
--- trunk/Source/WebCore/history/PageCache.cpp 2018-01-22 03:30:35 UTC (rev 227279)
+++ trunk/Source/WebCore/history/PageCache.cpp 2018-01-22 03:39:36 UTC (rev 227280)
@@ -189,12 +189,14 @@
static bool canCachePage(Page& page)
{
+ RELEASE_ASSERT(!page.isRestoringCachedPage());
+
unsigned indentLevel = 0;
PCLOG("--------\n Determining if page can be cached:");
DiagnosticLoggingClient& diagnosticLoggingClient = page.diagnosticLoggingClient();
bool isCacheable = canCacheFrame(page.mainFrame(), diagnosticLoggingClient, indentLevel + 1);
-
+
if (!page.settings().usesPageCache() || page.isResourceCachingDisabled()) {
PCLOG(" -Page settings says b/f cache disabled");
logPageCacheFailureDiagnosticMessage(diagnosticLoggingClient, DiagnosticLoggingKeys::isDisabledKey());
Modified: trunk/Source/WebCore/page/Page.h (227279 => 227280)
--- trunk/Source/WebCore/page/Page.h 2018-01-22 03:30:35 UTC (rev 227279)
+++ trunk/Source/WebCore/page/Page.h 2018-01-22 03:39:36 UTC (rev 227280)
@@ -374,6 +374,9 @@
void setIsClosing() { m_isClosing = true; }
bool isClosing() const { return m_isClosing; }
+ void setIsRestoringCachedPage(bool value) { m_isRestoringCachedPage = value; }
+ bool isRestoringCachedPage() const { return m_isRestoringCachedPage; }
+
void addActivityStateChangeObserver(ActivityStateChangeObserver&);
void removeActivityStateChangeObserver(ActivityStateChangeObserver&);
@@ -787,6 +790,7 @@
PAL::SessionID m_sessionID;
bool m_isClosing { false };
+ bool m_isRestoringCachedPage { false };
MediaProducer::MediaStateFlags m_mediaState { MediaProducer::IsNotPlaying };
