Bug 183386

Summary: LayoutTests/http/tests/navigation/page-cache-iframe-provisional-load.html fails with async policy delegate
Product: WebKit Reporter: Danyao Wang <danyao>
Component: Page LoadingAssignee: Danyao Wang <danyao>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, cdumez, commit-queue, dbates, ews, japhet, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 180568    
Attachments:
Description Flags
WIP
none
Patch for review
none
Patch for review (fixed style errors)
none
Patch
none
Patch for review (fixed style and old test) none

Description Danyao Wang 2018-03-06 15:16:31 PST
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.
Comment 1 Danyao Wang 2018-03-06 15:33:14 PST
Created attachment 335152 [details]
WIP
Comment 2 Build Bot 2018-03-06 15:35:01 PST
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 3 Chris Dumez 2018-03-06 15:38:11 PST
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.
Comment 4 Danyao Wang 2018-03-07 12:33:07 PST
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?
Comment 5 Chris Dumez 2018-03-07 12:41:11 PST
(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;
    }
Comment 6 Chris Dumez 2018-03-07 12:45:27 PST
(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.
Comment 7 Chris Dumez 2018-03-07 12:51:22 PST
(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.
Comment 8 Danyao Wang 2018-03-12 14:49:51 PDT
Created attachment 335646 [details]
Patch for review
Comment 9 Danyao Wang 2018-03-12 14:51:17 PDT
> 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.
Comment 10 Build Bot 2018-03-12 14:52:14 PDT
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.
Comment 11 Danyao Wang 2018-03-12 14:58:53 PDT
Created attachment 335647 [details]
Patch for review (fixed style errors)
Comment 12 Chris Dumez 2018-03-12 15:06:22 PDT
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 13 Chris Dumez 2018-03-12 15:31:11 PDT
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.
Comment 14 Chris Dumez 2018-03-12 16:40:06 PDT
I think this may be the last failure with async policy delegates. When will you be able to reupload?
Comment 15 Danyao Wang 2018-03-13 07:54:51 PDT
Created attachment 335692 [details]
Patch
Comment 16 Danyao Wang 2018-03-13 07:58:39 PDT
Created attachment 335693 [details]
Patch for review (fixed style and old test)
Comment 17 Danyao Wang 2018-03-13 08:07:03 PDT
(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 18 WebKit Commit Bot 2018-03-13 09:12:45 PDT
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>
Comment 19 WebKit Commit Bot 2018-03-13 09:12:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2018-03-13 09:13:28 PDT
<rdar://problem/38418622>