Bug 188701 - NetworkLoad::didReceiveResponse should pass its completion handler to its client
Summary: NetworkLoad::didReceiveResponse should pass its completion handler to its client
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on: 189163 190011
Blocks:
  Show dependency treegraph
 
Reported: 2018-08-17 10:56 PDT by Alex Christensen
Modified: 2018-09-26 14:49 PDT (History)
9 users (show)

See Also:


Attachments
Patch (26.77 KB, patch)
2018-08-17 11:00 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.98 MB, application/zip)
2018-08-17 12:21 PDT, EWS Watchlist
no flags Details
Patch (32.61 KB, patch)
2018-08-17 13:24 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.90 MB, application/zip)
2018-08-18 06:50 PDT, EWS Watchlist
no flags Details
Patch (32.39 KB, patch)
2018-09-24 15:33 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2018-08-17 10:56:14 PDT
NetworkLoad::didReceiveResponse should pass its completion handler to its client
Comment 1 Alex Christensen 2018-08-17 11:00:29 PDT
Created attachment 347369 [details]
Patch
Comment 2 EWS Watchlist 2018-08-17 12:21:10 PDT
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
Comment 3 EWS Watchlist 2018-08-17 12:21:11 PDT
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
Comment 4 Alex Christensen 2018-08-17 13:24:07 PDT
Created attachment 347385 [details]
Patch
Comment 5 EWS Watchlist 2018-08-18 06:50:36 PDT
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
Comment 6 EWS Watchlist 2018-08-18 06:50:48 PDT
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 7 Alex Christensen 2018-08-20 16:28:17 PDT
Comment on attachment 347443 [details]
Archive of layout-test-results from ews206 for win-future

Windows failure unrelated.
Comment 8 Michael Catanzaro 2018-08-20 18:57:30 PDT
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.
Comment 9 Alex Christensen 2018-08-27 17:26:55 PDT
http://trac.webkit.org/r235413
Comment 10 Radar WebKit Bug Importer 2018-08-27 17:27:22 PDT
<rdar://problem/43778059>
Comment 11 Ryan Haddad 2018-08-28 09:02:59 PDT
(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
Comment 12 Alex Christensen 2018-08-28 15:25:49 PDT
http://trac.webkit.org/r235444
Comment 13 WebKit Commit Bot 2018-08-30 11:13:17 PDT
Re-opened since this is blocked by bug 189163
Comment 14 Alex Christensen 2018-09-24 15:33:31 PDT
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.
Comment 15 Alex Christensen 2018-09-24 15:33:53 PDT
Created attachment 350701 [details]
Patch
Comment 16 Alex Christensen 2018-09-25 10:36:39 PDT
http://trac.webkit.org/r236463