Bug 188701 – NetworkLoad::didReceiveResponse should pass its completion handler to its client

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

NetworkLoad::didReceiveResponse should pass its completion handler to its client

[Alex Christensen]

Created attachment 347369 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 347385 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alex Christensen]

Comment on attachment 347443 [details]
Archive of layout-test-results from ews206 for win-future

Windows failure unrelated.

[Michael Catanzaro]

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.

[Alex Christensen]

http://trac.webkit.org/r235413

[Radar WebKit Bug Importer]

<rdar://problem/43778059>

[Ryan Haddad]

(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

[Alex Christensen]

http://trac.webkit.org/r235444

[WebKit Commit Bot]

Re-opened since this is blocked by bug 189163

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 350701 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r236463