This test starts a slow load in an iframe and tries to navigate away from the main frame. It expects that the pending iframe navigation will prevent the main frame document from being added to the PageCache. With async decide policy, PageCache::addIfCacheable() may be called (and it is in this test) on the main frame navigation while the subframe is still waiting for policy decision. The subframe loader's state has not yet been updated from FrameStateCompleted to FrameStateProvisional. This causes PageCache to incorrect consider the page as cacheable.
Created attachment 335152 [details] WIP
Attachment 335152 [details] did not pass style-queue: ERROR: Source/WebKit/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/loader/FrameLoader.cpp:2294: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 335152 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=335152&action=review > Source/WebKit/ChangeLog:8 > + Add WKFrameLoadState enum to match FrameLoaderState. indentation problem. > Source/WebCore/history/PageCache.cpp:81 > + if (!frame.isMainFrame() && (frameLoader.state() == FrameStateAsyncDecidePolicy || frameLoader.state() == FrameStateProvisional)) { Instead of adding a new state, would it be possible to rely on frameLoader.policyChecker().delegateIsDecidingNavigationPolicy() ? > LayoutTests/ChangeLog:9 > + Update tests to test the async case. indentation problem. > LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load.html:10 > + if (testRunner.setShouldDecideNavigationPolicyAfterDelay) Please copy the test before adding those lines so we keep both versions of the test.
Turns out my initial analysis was wrong and we can't use delegateIsDecidingNavigationPolicy(). What happens is that the decide policy process for the main frame and the subframe are interleaved. So when FrameLoader::continueLoadAfterNavigationPolicy() is called for the main frame, it calls FrameLoader::stopAllLoaders(), which invalidates the in-progress policy check on the subframe. This triggers FrameLoader::continueLoadAfterNavigationPlicy() on the subframe with PolicyAction::Ignore. Later when PageCache::addIfCacheable() is called after main frame commits the provisional navigation, the sub frame load has already been cancelled, so it is neither in decide policy nor provisional load. This leads to PageCache deciding to cache the page. With sync policy delegate, the main policy decision is fully finished before subframe enters into decide policy. The subframe loader gets a PolicyAction::Use result from the UIProcess and transitions to FrameStateProvisionalLoad before the main frame commits the provisional navigation. It seems to me that canceling outstanding subframe policy checks when main frame commits its provisional navigation makes sense. So despite the difference in timing, I think the behavior here is working as intended. I'm less sure if the page in such a state should be cached. "No" seems to make sense, because the page is now in a broken state with one of its subframe load interrupted and I assume that's why we check for FrameStateProvisionalLoad. Capturing this specific case will be a bit messy, because we'd need to pass a cancelation reason into FrameLoader::stopAllLoaders(). Chris - what do you think?
(In reply to Danyao Wang from comment #4) > Turns out my initial analysis was wrong and we can't use > delegateIsDecidingNavigationPolicy(). > > What happens is that the decide policy process for the main frame and the > subframe are interleaved. So when > FrameLoader::continueLoadAfterNavigationPolicy() is called for the main > frame, it calls FrameLoader::stopAllLoaders(), which invalidates the > in-progress policy check on the subframe. This triggers > FrameLoader::continueLoadAfterNavigationPlicy() on the subframe with > PolicyAction::Ignore. Later when PageCache::addIfCacheable() is called after > main frame commits the provisional navigation, the sub frame load has > already been cancelled, so it is neither in decide policy nor provisional > load. This leads to PageCache deciding to cache the page. > > With sync policy delegate, the main policy decision is fully finished before > subframe enters into decide policy. The subframe loader gets a > PolicyAction::Use result from the UIProcess and transitions to > FrameStateProvisionalLoad before the main frame commits the provisional > navigation. > > It seems to me that canceling outstanding subframe policy checks when main > frame commits its provisional navigation makes sense. So despite the > difference in timing, I think the behavior here is working as intended. > > I'm less sure if the page in such a state should be cached. "No" seems to > make sense, because the page is now in a broken state with one of its > subframe load interrupted and I assume that's why we check for > FrameStateProvisionalLoad. Capturing this specific case will be a bit messy, > because we'd need to pass a cancelation reason into > FrameLoader::stopAllLoaders(). > > Chris - what do you think? I believe this is the check that is supposed to make sure we don't put broken content into PageCache: if (!documentLoader->mainDocumentError().isNull()) { PCLOG(" -Main document has an error"); logPageCacheFailureDiagnosticMessage(diagnosticLoggingClient, DiagnosticLoggingKeys::mainDocumentErrorKey()); if (documentLoader->mainDocumentError().isCancellation() && documentLoader->subresourceLoadersArePageCacheAcceptable()) PCLOG(" -But, it was a cancellation and all loaders during the cancelation were loading images or XHR."); else isCacheable = false; }
(In reply to Chris Dumez from comment #5) > (In reply to Danyao Wang from comment #4) > > Turns out my initial analysis was wrong and we can't use > > delegateIsDecidingNavigationPolicy(). > > > > What happens is that the decide policy process for the main frame and the > > subframe are interleaved. So when > > FrameLoader::continueLoadAfterNavigationPolicy() is called for the main > > frame, it calls FrameLoader::stopAllLoaders(), which invalidates the > > in-progress policy check on the subframe. This triggers > > FrameLoader::continueLoadAfterNavigationPlicy() on the subframe with > > PolicyAction::Ignore. Later when PageCache::addIfCacheable() is called after > > main frame commits the provisional navigation, the sub frame load has > > already been cancelled, so it is neither in decide policy nor provisional > > load. This leads to PageCache deciding to cache the page. > > > > With sync policy delegate, the main policy decision is fully finished before > > subframe enters into decide policy. The subframe loader gets a > > PolicyAction::Use result from the UIProcess and transitions to > > FrameStateProvisionalLoad before the main frame commits the provisional > > navigation. > > > > It seems to me that canceling outstanding subframe policy checks when main > > frame commits its provisional navigation makes sense. So despite the > > difference in timing, I think the behavior here is working as intended. > > > > I'm less sure if the page in such a state should be cached. "No" seems to > > make sense, because the page is now in a broken state with one of its > > subframe load interrupted and I assume that's why we check for > > FrameStateProvisionalLoad. Capturing this specific case will be a bit messy, > > because we'd need to pass a cancelation reason into > > FrameLoader::stopAllLoaders(). > > > > Chris - what do you think? > > I believe this is the check that is supposed to make sure we don't put > broken content into PageCache: > if (!documentLoader->mainDocumentError().isNull()) { > PCLOG(" -Main document has an error"); > logPageCacheFailureDiagnosticMessage(diagnosticLoggingClient, > DiagnosticLoggingKeys::mainDocumentErrorKey()); > > if (documentLoader->mainDocumentError().isCancellation() && > documentLoader->subresourceLoadersArePageCacheAcceptable()) > PCLOG(" -But, it was a cancellation and all loaders during > the cancelation were loading images or XHR."); > else > isCacheable = false; > } Overall, I tend to think that we are behaving fine here. We cancel the subframe load during navigation policy decision, *before* provisional load, and therefore, we put the page into PageCache. It is not broken content, we merely did not navigate. I think one way to address this would be to tweak the test somehow so that we navigate the main frame *after* the subframe has started provisional load. I don't know how yet but there must be a way.
(In reply to Chris Dumez from comment #6) > (In reply to Chris Dumez from comment #5) > > (In reply to Danyao Wang from comment #4) > > > Turns out my initial analysis was wrong and we can't use > > > delegateIsDecidingNavigationPolicy(). > > > > > > What happens is that the decide policy process for the main frame and the > > > subframe are interleaved. So when > > > FrameLoader::continueLoadAfterNavigationPolicy() is called for the main > > > frame, it calls FrameLoader::stopAllLoaders(), which invalidates the > > > in-progress policy check on the subframe. This triggers > > > FrameLoader::continueLoadAfterNavigationPlicy() on the subframe with > > > PolicyAction::Ignore. Later when PageCache::addIfCacheable() is called after > > > main frame commits the provisional navigation, the sub frame load has > > > already been cancelled, so it is neither in decide policy nor provisional > > > load. This leads to PageCache deciding to cache the page. > > > > > > With sync policy delegate, the main policy decision is fully finished before > > > subframe enters into decide policy. The subframe loader gets a > > > PolicyAction::Use result from the UIProcess and transitions to > > > FrameStateProvisionalLoad before the main frame commits the provisional > > > navigation. > > > > > > It seems to me that canceling outstanding subframe policy checks when main > > > frame commits its provisional navigation makes sense. So despite the > > > difference in timing, I think the behavior here is working as intended. > > > > > > I'm less sure if the page in such a state should be cached. "No" seems to > > > make sense, because the page is now in a broken state with one of its > > > subframe load interrupted and I assume that's why we check for > > > FrameStateProvisionalLoad. Capturing this specific case will be a bit messy, > > > because we'd need to pass a cancelation reason into > > > FrameLoader::stopAllLoaders(). > > > > > > Chris - what do you think? > > > > I believe this is the check that is supposed to make sure we don't put > > broken content into PageCache: > > if (!documentLoader->mainDocumentError().isNull()) { > > PCLOG(" -Main document has an error"); > > logPageCacheFailureDiagnosticMessage(diagnosticLoggingClient, > > DiagnosticLoggingKeys::mainDocumentErrorKey()); > > > > if (documentLoader->mainDocumentError().isCancellation() && > > documentLoader->subresourceLoadersArePageCacheAcceptable()) > > PCLOG(" -But, it was a cancellation and all loaders during > > the cancelation were loading images or XHR."); > > else > > isCacheable = false; > > } > > Overall, I tend to think that we are behaving fine here. We cancel the > subframe load during navigation policy decision, *before* provisional load, > and therefore, we put the page into PageCache. It is not broken content, we > merely did not navigate. > I think one way to address this would be to tweak the test somehow so that > we navigate the main frame *after* the subframe has started provisional > load. I don't know how yet but there must be a way. I think we may be able to rely on the beforeunload event of the subframe. We only fire this event *after* the policy decision has been made to navigate and right before we start provisional load. So something like a beforeunload event handler with a setTimeout(0), then navigate the main frame.
Created attachment 335646 [details] Patch for review
> I think we may be able to rely on the beforeunload event of the subframe. We > only fire this event *after* the policy decision has been made to navigate > and right before we start provisional load. So something like a beforeunload > event handler with a setTimeout(0), then navigate the main frame. Thanks for the idea! Added new test. PTAL.
Attachment 335646 [details] did not pass style-queue: ERROR: LayoutTests/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:16: Line contains tab character. [whitespace/tab] [5] ERROR: LayoutTests/ChangeLog:17: Line contains tab character. [whitespace/tab] [5] Total errors found: 7 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 335647 [details] Patch for review (fixed style errors)
Comment on attachment 335646 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=335646&action=review > LayoutTests/ChangeLog:8 > + This test relies on the happenstance that with sync poicy decision, the subframe transitions Indentation issue. Please only use spaces, not tabs. > LayoutTests/ChangeLog:9 > + to FrameLoadProvisional before main frame navigation is committed. With async delegate, Only 1 space after the '.' > LayoutTests/ChangeLog:15 > + * http/tests/navigation/page-cache-iframe-provisional-load-async-delegates.html: Added. Please update existing test too. Otherwise, it will start failing when we enable async policy delegates by default. Also, it will help me see what has changed :) > LayoutTests/ChangeLog:16 > + Add a new version of the test that forces the main frame navigation to start after the indentation issue.
Comment on attachment 335647 [details] Patch for review (fixed style errors) View in context: https://bugs.webkit.org/attachment.cgi?id=335647&action=review > LayoutTests/ChangeLog:15 > + * http/tests/navigation/page-cache-iframe-provisional-load-async-delegates.html: Added. cp LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load-async-delegates.html LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load.html Then fix LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load.html to not call testRunner.setShouldDecideNavigationPolicyAfterDelay(). This way, the tests won't fail once we turn on async policy delegates by default.
I think this may be the last failure with async policy delegates. When will you be able to reupload?
Created attachment 335692 [details] Patch
Created attachment 335693 [details] Patch for review (fixed style and old test)
(In reply to Chris Dumez from comment #13) > Comment on attachment 335647 [details] > Patch for review (fixed style errors) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335647&action=review > > > LayoutTests/ChangeLog:15 > > + * http/tests/navigation/page-cache-iframe-provisional-load-async-delegates.html: Added. > > cp > LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load-async- > delegates.html > LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load.html > Then fix > LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load.html to > not call testRunner.setShouldDecideNavigationPolicyAfterDelay(). > > This way, the tests won't fail once we turn on async policy delegates by > default. This makes total sense. Sorry for not realizing it earlier. Fixed. Also fixed the tabs and whitespace issues. PTAL.
Comment on attachment 335693 [details] Patch for review (fixed style and old test) Clearing flags on attachment: 335693 Committed r229577: <https://trac.webkit.org/changeset/229577>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38418622>