| Summary: | NetworkLoad::didReceiveResponse should pass its completion handler to its client | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | cdumez, cgarcia, commit-queue, ews-watchlist, mcatanzaro, rniwa, ryanhaddad, webkit-bug-importer, youennf | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | 189163, 190011 | ||||||||||||||
| Bug Blocks: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Alex Christensen
2018-08-17 10:56:14 PDT
Created attachment 347369 [details]
Patch
Comment on attachment 347369 [details] Patch Attachment 347369 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8893390 New failing tests: fast/dom/HTMLAnchorElement/anchor-file-blob-convert-to-download.html fast/dom/HTMLAnchorElement/anchor-file-blob-convert-to-download-async-delegate.html Created attachment 347379 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 347385 [details]
Patch
Comment on attachment 347385 [details] Patch Attachment 347385 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8901361 New failing tests: imported/blink/transitions/unprefixed-transform.html Created attachment 347443 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 347443 [details]
Archive of layout-test-results from ews206 for win-future
Windows failure unrelated.
Comment on attachment 347385 [details] Patch Good. View in context: https://bugs.webkit.org/attachment.cgi?id=347385&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:462 > - return ShouldContinueDidReceiveResponse::Yes; > + return completionHandler(PolicyAction::Use); You're relying on the return type of the completion handler being void. Was this an accident? Or return completionHandler() a sort of way to indicate finality? I don't like it myself... I would write: completionHandler(PolicyAction::Use); return; > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:466 > - return ShouldContinueDidReceiveResponse::No; > + return completionHandler(PolicyAction::Ignore); Ditto. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:476 > - return ShouldContinueDidReceiveResponse::No; > + return completionHandler(PolicyAction::Ignore); Ditto. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:483 > - return ShouldContinueDidReceiveResponse::Yes; > + return completionHandler(PolicyAction::Use); Ditto. (In reply to Alex Christensen from comment #9) > http://trac.webkit.org/r235413 Two service worker tests are failing one of the assertions added in this change: ASSERTION FAILED: action == PolicyAction::Use /Volumes/Data/slave/highsierra-debug/build/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp(645) : auto WebKit::NetworkResourceLoader::didFinishWithRedirectResponse(WebCore::ResourceResponse &&)::(anonymous class)::operator()(type-parameter-0-0) const 1 0x1165e8269 WTFCrash 2 0x10f3805cb WTFCrashWithInfo(int, char const*, char const*, int) 3 0x10f6b011f auto WebKit::NetworkResourceLoader::didFinishWithRedirectResponse(WebCore::ResourceResponse&&)::$_22::operator()<WebCore::PolicyAction>(WebCore::PolicyAction) const 4 0x10f6b009a WTF::Function<void (WebCore::PolicyAction)>::CallableWrapper<WebKit::NetworkResourceLoader::didFinishWithRedirectResponse(WebCore::ResourceResponse&&)::$_22>::call(WebCore::PolicyAction) 5 0x10f635973 WTF::Function<void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) const 6 0x10f616b65 WTF::CompletionHandler<void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) 7 0x10f6639cc WebKit::NetworkResourceLoader::~NetworkResourceLoader() https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r235420%20(4647)/results.html Re-opened since this is blocked by bug 189163 Comment on attachment 347385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347385&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:718 > - if (m_cacheEntryWaitingForContinueDidReceiveResponse) { > - continueProcessingCachedEntryAfterDidReceiveResponse(WTFMove(m_cacheEntryWaitingForContinueDidReceiveResponse)); > - return; > - } > + if (m_responseCompletionHandler) > + m_responseCompletionHandler(PolicyAction::Use); I had switched the order of these callbacks. Calling m_responseCompletionHandler (previously NetworkLoad::continueDidReceiveResponse) sometimes deletes the NetworkResourceLoader, so we can't read from its member variables later in the function. Created attachment 350701 [details]
Patch
|