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

-- 
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/0c45795f/attachment.html>