Bug 182720

Created attachment 333712 [details] Failing API test Patch relies on the one from Bug 182697.

Created attachment 333730 [details] WIP Patch

Created attachment 333731 [details] WIP Patch

<rdar://problem/37515204>

Created attachment 333733 [details] WIP Patch

Created attachment 333737 [details] WIP Patch

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

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

Created attachment 333807 [details] WIP Patch

Created attachment 334086 [details] Patch

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 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 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 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.

Created attachment 334230 [details] Patch

Created attachment 334231 [details] Patch

Created attachment 334233 [details] Patch

Created attachment 334238 [details] Patch

Created attachment 334239 [details] Patch

Comment on attachment 334239 [details] Patch Clearing flags on attachment: 334239 Committed r228852: <https://trac.webkit.org/changeset/228852>

All reviewed patches have been landed. Closing bug.