[Webkit-unassigned] [Bug 183386] LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load.html fails with async policy delegate

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=183386

--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180307/8ba265a6/attachment-0001.html>