Bug 182720 - Provisional load may get committed before receiving the decidePolicyForNavigationResponse response
Summary: Provisional load may get committed before receiving the decidePolicyForNaviga...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 180568
  Show dependency treegraph
 
Reported: 2018-02-12 16:42 PST by Chris Dumez
Modified: 2018-03-12 14:47 PDT (History)
11 users (show)

See Also:


Attachments
Failing API test (2.17 KB, patch)
2018-02-13 12:24 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (16.30 KB, patch)
2018-02-13 15:04 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (16.21 KB, patch)
2018-02-13 15:05 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (16.21 KB, patch)
2018-02-13 15:21 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (16.21 KB, patch)
2018-02-13 15:32 PST, Chris Dumez
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.44 MB, application/zip)
2018-02-13 18:58 PST, EWS Watchlist
no flags Details
WIP Patch (17.20 KB, patch)
2018-02-14 09:36 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.66 KB, patch)
2018-02-16 16:15 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.65 KB, patch)
2018-02-19 20:18 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.64 KB, patch)
2018-02-19 20:24 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.66 KB, patch)
2018-02-19 20:39 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.68 KB, patch)
2018-02-19 21:10 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.68 KB, patch)
2018-02-19 21:18 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-02-12 16:42:25 PST
Provisional load may get committed before receiving the decidePolicyForNavigationResponse response from the client.

This is because WebResourceLoader::didReceiveResponse() sends the NetworkResourceLoader::ContinueDidReceiveResponse() IPC back to the network process right after calling ResourceLoader::didReceiveResponse(), allowing CFNetwork to start sending us data for the resource. However, ResourceLoader::didReceiveResponse() triggers the decidePolicyForNavigationResponse delegate which the client may provide an answer to asynchronously.

If the client responds asynchronously and takes too much time to response, we'll receive all the main resource's data and commit the provisional load anyway.
Comment 1 Chris Dumez 2018-02-13 12:24:47 PST
Created attachment 333712 [details]
Failing API test

Patch relies on the one from Bug 182697.
Comment 2 Chris Dumez 2018-02-13 15:04:28 PST
Created attachment 333730 [details]
WIP Patch
Comment 3 Chris Dumez 2018-02-13 15:05:44 PST
Created attachment 333731 [details]
WIP Patch
Comment 4 Radar WebKit Bug Importer 2018-02-13 15:07:49 PST
<rdar://problem/37515204>
Comment 5 Chris Dumez 2018-02-13 15:21:38 PST
Created attachment 333733 [details]
WIP Patch
Comment 6 Chris Dumez 2018-02-13 15:32:12 PST
Created attachment 333737 [details]
WIP Patch
Comment 7 EWS Watchlist 2018-02-13 18:58:37 PST
Comment on attachment 333737 [details]
WIP Patch

Attachment 333737 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6492317

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-content-to-inscope.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-css-cross-origin-mime-check.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-content-to-outscope.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/referrer-policy-header.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-no-freshness-headers.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-redirect.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-header-visibility.https.html
fast/dom/HTMLAnchorElement/anchor-file-blob-convert-to-download.html
imported/w3c/web-platform-tests/service-workers/service-worker/referer.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-csp.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-response-taint.https.html
Comment 8 EWS Watchlist 2018-02-13 18:58:39 PST
Created attachment 333759 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 9 Chris Dumez 2018-02-14 09:36:16 PST
Created attachment 333807 [details]
WIP Patch
Comment 10 Chris Dumez 2018-02-16 16:15:19 PST
Created attachment 334086 [details]
Patch
Comment 11 Alex Christensen 2018-02-19 10:31:48 PST
Comment on attachment 334086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334086&action=review

> Source/WebCore/loader/SubresourceLoader.cpp:365
> +    ResourceLoader::didReceiveResponse(response, nullptr);

Do we not want to pass a completion handler to the parent class here, then do the rest upon completion?

> Source/WebCore/loader/SubresourceLoader.h:65
> +    void didReceiveResponseForDecidePolicyForResponse();

I think this is a bad name.  didReceiveResponsePolicy?

> Source/WebCore/loader/SubresourceLoader.h:131
> +    WTF::CompletionHandler<void()> m_policyForResponseCompletionHandler;

You shouldn't need WTF:: anywhere in this patch.

> Source/WebCore/loader/SubresourceLoader.h:134
> +    bool m_inAsyncPolicyCheckForResponse { false };

I'm not sure marking like this is the best approach.
Comment 12 Chris Dumez 2018-02-19 13:29:41 PST
Comment on attachment 334086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334086&action=review

>> Source/WebCore/loader/SubresourceLoader.cpp:365
>> +    ResourceLoader::didReceiveResponse(response, nullptr);
> 
> Do we not want to pass a completion handler to the parent class here, then do the rest upon completion?

I can look into this. It is not technically needed since the policy check is triggered by the SubResourceLoader code but it may look nicer.

>> Source/WebCore/loader/SubresourceLoader.h:65
>> +    void didReceiveResponseForDecidePolicyForResponse();
> 
> I think this is a bad name.  didReceiveResponsePolicy?

Ok.

>> Source/WebCore/loader/SubresourceLoader.h:131
>> +    WTF::CompletionHandler<void()> m_policyForResponseCompletionHandler;
> 
> You shouldn't need WTF:: anywhere in this patch.

Ok.

>> Source/WebCore/loader/SubresourceLoader.h:134
>> +    bool m_inAsyncPolicyCheckForResponse { false };
> 
> I'm not sure marking like this is the best approach.

Do you have an alternative. I need to make sure:
1. The CompletionHandler always gets called
2. The CompletionHandler gets called after the policy decision is made, when an async policy decision is happening

The issue is that there are a lot of layer abstractions between SubResourceLoader::didReceiveResponse() getting called and the policy check happens (In  DocumentLoader::responseReceived()). It goes through CachedRawResource / CachedRawResourceClient.
I initially trying piping the completion handler all the way through but this ended up being complicated and super intrusive. This is why I ended up storing the completion handler on the subResourceLoader and having a flag on the subResourceLoader that gets set when an async policy decision is happening. This makes it very unlikely I fail to call the completion handler.
Comment 13 Alex Christensen 2018-02-19 14:20:54 PST
Comment on attachment 334086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334086&action=review

>>> Source/WebCore/loader/SubresourceLoader.h:134
>>> +    bool m_inAsyncPolicyCheckForResponse { false };
>> 
>> I'm not sure marking like this is the best approach.
> 
> Do you have an alternative. I need to make sure:
> 1. The CompletionHandler always gets called
> 2. The CompletionHandler gets called after the policy decision is made, when an async policy decision is happening
> 
> The issue is that there are a lot of layer abstractions between SubResourceLoader::didReceiveResponse() getting called and the policy check happens (In  DocumentLoader::responseReceived()). It goes through CachedRawResource / CachedRawResourceClient.
> I initially trying piping the completion handler all the way through but this ended up being complicated and super intrusive. This is why I ended up storing the completion handler on the subResourceLoader and having a flag on the subResourceLoader that gets set when an async policy decision is happening. This makes it very unlikely I fail to call the completion handler.

Can't you just check to see if the completion handler is null instead of having a completion handler and a bool?
Comment 14 Chris Dumez 2018-02-19 16:08:12 PST
Comment on attachment 334086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334086&action=review

> Source/WebCore/loader/DocumentLoader.cpp:787
> +        mainResourceLoader->markInAsyncPolicyCheckForResponse();

The boolean flag gets set here when the DocumentLoader actually starts a policy check

> Source/WebCore/loader/DocumentLoader.cpp:790
> +            mainResourceLoader->didReceiveResponseForDecidePolicyForResponse();

... and unsets the flag here synchronously or asynchronously.

The flag is controlled by the policy checking from the DocumentLoader. The completion handler is in the SubresourceLoader and needs to be called asynchronously if the client responds to the policy check asynchronously.

> Source/WebCore/loader/SubresourceLoader.cpp:396
> +    if (m_inAsyncPolicyCheckForResponse) {

See logic here. The code above (in particular this call: m_resource->responseReceived(response)), we cause a policy check. We a policy check happened AND we did not get a response for this policy check yet, then it is an async policy check and we need to store the completionHandler here in m_policyForResponseCompletionHandler so that we can call the completion handler after the policy check.

>>>> Source/WebCore/loader/SubresourceLoader.h:134
>>>> +    bool m_inAsyncPolicyCheckForResponse { false };
>>> 
>>> I'm not sure marking like this is the best approach.
>> 
>> Do you have an alternative. I need to make sure:
>> 1. The CompletionHandler always gets called
>> 2. The CompletionHandler gets called after the policy decision is made, when an async policy decision is happening
>> 
>> The issue is that there are a lot of layer abstractions between SubResourceLoader::didReceiveResponse() getting called and the policy check happens (In  DocumentLoader::responseReceived()). It goes through CachedRawResource / CachedRawResourceClient.
>> I initially trying piping the completion handler all the way through but this ended up being complicated and super intrusive. This is why I ended up storing the completion handler on the subResourceLoader and having a flag on the subResourceLoader that gets set when an async policy decision is happening. This makes it very unlikely I fail to call the completion handler.
> 
> Can't you just check to see if the completion handler is null instead of having a completion handler and a bool?

I don't understand your comment, this flag is not about having a completionHandler or not. It is about having a pending async policy delegate or not. If this flag is true, we will store the completionHandler on the SubResourceLoader so that we can call it later, when we get a response for the policy.
Comment 15 Chris Dumez 2018-02-19 20:18:36 PST
Created attachment 334230 [details]
Patch
Comment 16 Chris Dumez 2018-02-19 20:24:25 PST
Created attachment 334231 [details]
Patch
Comment 17 Chris Dumez 2018-02-19 20:39:16 PST
Created attachment 334233 [details]
Patch
Comment 18 Chris Dumez 2018-02-19 21:10:24 PST
Created attachment 334238 [details]
Patch
Comment 19 Chris Dumez 2018-02-19 21:18:26 PST
Created attachment 334239 [details]
Patch
Comment 20 WebKit Commit Bot 2018-02-20 17:08:32 PST
Comment on attachment 334239 [details]
Patch

Clearing flags on attachment: 334239

Committed r228852: <https://trac.webkit.org/changeset/228852>
Comment 21 WebKit Commit Bot 2018-02-20 17:08:34 PST
All reviewed patches have been landed.  Closing bug.