<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load.html fails with async policy delegate"
   href="https://bugs.webkit.org/show_bug.cgi?id=183386#c6">Comment # 6</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load.html fails with async policy delegate"
   href="https://bugs.webkit.org/show_bug.cgi?id=183386">bug 183386</a>
              from <span class="vcard"><a class="email" href="mailto:cdumez@apple.com" title="Chris Dumez <cdumez@apple.com>"> <span class="fn">Chris Dumez</span></a>
</span></b>
        <pre>(In reply to Chris Dumez from <a href="show_bug.cgi?id=183386#c5">comment #5</a>)
<span class="quote">> (In reply to Danyao Wang from <a href="show_bug.cgi?id=183386#c4">comment #4</a>)
> > 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;
>     }</span >

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.</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>