This times out because data for the iframe load is received before the response policy decision is received. The problem is that we try to commit the load, which cancels the in-progress policy check, which cancels the load. So the iframe never finishes loading, and the test hangs waiting for the iframe onload event. More details: ServiceWorkerClientFetch::didReceiveData happens after FrameLoader::checkContentPolicy but before we get a response to that check. So then we get to DocumentLoader::commitLoad, which leads to FrameLoader::transitionToCommitted, which leads to PolicyChecker::stopCheck in FrameLoader::stopLoading, which causes DocumentLoader::continueAfterContentPolicy to get called with decision Policy::Ignore. This leads to the frame loader getting destroyed without ever getting to dispatch a load event. We need to wait for a policy decision before trying to call DocumentLoader::commitLoad. Is there an obvious place for DocumentLoader::dataReceived to stash away the data (rather than immediately call DocumentLoader::commitLoad) so that it can be used later when we receive the policy response? Or would it make more sense to somehow defer handling the ServiceWorkerClientFetch::didReceiveData IPC until we're ready for it (though I don't think there's anything SW-specific about this bug, other than maybe the load happens to complete fast enough to beat the policy response).
(In reply to Ali Juma from comment #0) > (though I don't think there's anything > SW-specific about this bug, other than maybe the load happens to complete > fast enough to beat the policy response). Actually, this *does* seem to be SW-specific. The non-SW version of this bug was fixed in bug 182720, where the NetworkResourceLoader::ContinueWillSendRequest message is now delayed until a policy response is received. So it looks like we need an equivalent way to delay receiving data from ServiceWorkers.
It might be a dupe of bug 183110.
Created attachment 335460 [details] Patch
(In reply to youenn fablet from comment #2) > It might be a dupe of bug 183110. Thanks for the pointer! It's the same underlying issue, but looks like there's a race in the setting/reading of m_isCheckingResponse from that bug.
(In reply to Ali Juma from comment #4) > (In reply to youenn fablet from comment #2) > > It might be a dupe of bug 183110. > > Thanks for the pointer! It's the same underlying issue, but looks like > there's a race in the setting/reading of m_isCheckingResponse from that bug. There should be no race since all that code is run on the main thread. I agree this code is misleading, see bug 181392 for more information on why we have these calls. Ideally, in a not too far future, we will be able to remove them. We could document this in the code or maybe add some ASSERT(isMainThread()) in case we want to start receiving data out of the main thread. Test looks good.
(In reply to youenn fablet from comment #5) > (In reply to Ali Juma from comment #4) > > (In reply to youenn fablet from comment #2) > > > It might be a dupe of bug 183110. > > > > Thanks for the pointer! It's the same underlying issue, but looks like > > there's a race in the setting/reading of m_isCheckingResponse from that bug. > > There should be no race since all that code is run on the main thread. > I agree this code is misleading, see bug 181392 for more information on why > we have these calls. Ah, so it's not a race between threads, but rather than the didReceiveResponse and didReceiveData IPCs are sent almost one after the other in ServiceWorkerFetch::processRepsonse. So when ServiceWorkerClientFetch::didReceiveResponse is executing, it's possible that the task for handling the didReceiveData message has already been added to the runloop, so the code that gets scheduled on the runloop (via callOnMainThread) in didReceiveResponse will execute *after* didReceiveData executes, and that's why m_isCheckingResponse is still false in didReceiveData. Moving the check of m_isCheckingResponse into the code that's inside callOnMainThread in didReceiveData gives us the correct ordering, since that code will run *after* the code that's inside callOnMainThread in didReceiveResponse.
Created attachment 335478 [details] Patch Update changelog description of the race
Oh I see, it might be better to set m_isCheckingResponse to true in ServiceWorkerClientFetch::didReceiveResponse before calling callOnMainThread then.
Created attachment 335593 [details] Patch
Comment on attachment 335593 [details] Patch Need to figure out why this version makes imported/w3c/web-platform-tests/service-workers/service-worker/redirected-response.https.html timeout
Created attachment 335597 [details] Patch
(In reply to Ali Juma from comment #10) > Comment on attachment 335593 [details] > Patch > > Need to figure out why this version makes > imported/w3c/web-platform-tests/service-workers/service-worker/redirected- > response.https.html timeout Figured it out -- when clearing m_isCheckingResponse in the code posted to the runloop by didReceiveResponse, we also need to call continueLoadingAfterCheckingResponse to deal with any calls that earlied-out while m_isCheckingResponse was true.
Comment on attachment 335597 [details] Patch r=me. To handle the early exit, you can probably use makeScopeExit and release the scope in case it uses the regular path. See for instance RegistrationDatabase::openSQLiteDatabase.
Comment on attachment 335597 [details] Patch Attachment 335597 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6916049 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/redirected-response.https.html
Created attachment 335598 [details] Archive of layout-test-results from ews105 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 335597 [details] Patch Attachment 335597 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6916071 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/redirected-response.https.html
Created attachment 335602 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 335597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335597&action=review > Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:107 > + continueLoadingAfterCheckingResponse(); continueLoadingAfterCheckingResponse is a no-op in that case. > Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:113 > + continueLoadingAfterCheckingResponse(); We probably do not want to call continueLoadingAfterCheckingResponse() since we will fail the loader just after. > Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:124 > + continueLoadingAfterCheckingResponse(); In that case, we probably want to set m_isCheckingResponse back to false but we do not need to call continueLoadingAfterCheckingResponse(). That will ensure that when following the redirection and getting the error, we will report the error.
Created attachment 335621 [details] Patch
Comment on attachment 335597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335597&action=review >> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:107 >> + continueLoadingAfterCheckingResponse(); > > continueLoadingAfterCheckingResponse is a no-op in that case. Removed. >> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:113 >> + continueLoadingAfterCheckingResponse(); > > We probably do not want to call continueLoadingAfterCheckingResponse() since we will fail the loader just after. Removed. >> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:124 >> + continueLoadingAfterCheckingResponse(); > > In that case, we probably want to set m_isCheckingResponse back to false but we do not need to call continueLoadingAfterCheckingResponse(). > That will ensure that when following the redirection and getting the error, we will report the error. The reason we need continueLoadAfterCheckingResponse here is that m_redirectionStatus is set to RedirectionStatus::Receiving below, and only a call to didFinish can advance that to Received, so if we already earlied-out a call to didFinish we need to make sure it gets called again. There's another related issue with redirects: we need to reset the value of m_didFinish in start(), so that a didFinish that happens before the redirection doesn't confuse a future call to continueLoadingAfterCheckingResponse following the redirection.
Is this ready to land?
Comment on attachment 335621 [details] Patch Clearing flags on attachment: 335621 Committed r229558: <https://trac.webkit.org/changeset/229558>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38397893>