REGRESSION(r252185): NetworkSessionCocoa cancels downloads that receive authentication challenges

• Component: WebKit2
• Status: RESOLVED FIXED

[David Quesada]

rdar://problem/58999654

[David Quesada]

Created attachment 389228 [details]
Patch

[youenn fablet]

Comment on attachment 389228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389228&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:618
> +        auto download = _session->networkProcess().downloadManager().download(_sessionWrapper->downloadMap.get(task.taskIdentifier));

There does not seem to be any guarantee that downloadMap will contain task.taskIdentifier.
If not, this would return 0 which could corrupt the DownloadManager map.
Can we add a check there, like done in URLSession:downloadTask:?

Also, I am wondering whether is it possible to create a test for it.
API tests can spin dedicated servers (look for ServiceWorkerTCPServer), not sure layout tests can pause/resume downloads though we could add test runner APIs.

[Brady Eidson]

(In reply to youenn fablet from comment #2)
> Comment on attachment 389228 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389228&action=review
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:618
> > +        auto download = _session->networkProcess().downloadManager().download(_sessionWrapper->downloadMap.get(task.taskIdentifier));
> 
> There does not seem to be any guarantee that downloadMap will contain
> task.taskIdentifier.
> If not, this would return 0 which could corrupt the DownloadManager map.
> Can we add a check there, like done in URLSession:downloadTask:?
> 
> Also, I am wondering whether is it possible to create a test for it.
> API tests can spin dedicated servers (look for ServiceWorkerTCPServer), not
> sure layout tests can pause/resume downloads though we could add test runner
> APIs.

We have all the tools we need to API test this.

[David Quesada]

Created attachment 389316 [details]
Patch v2

- Took Youenn's suggestion to zero-check the download ID before looking it up in the DownloadMap.
- Added an API test that resumes a download and ensures the delegate can handle the authentication challenge and the download can succeed.

[David Quesada]

Created attachment 389318 [details]
Patch v3

Rebased.

[Alex Christensen]

Comment on attachment 389318 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=389318&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:616
> +    if ([task isKindOfClass:[NSURLSessionDownloadTask class]]) {

Could this just be if (!networkDataTask)?

[David Quesada]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 389318 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389318&action=review
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:616
> > +    if ([task isKindOfClass:[NSURLSessionDownloadTask class]]) {
> 
> Could this just be if (!networkDataTask)?

Sure.

[David Quesada]

Created attachment 389321 [details]
Patch v4

[Brady Eidson]

Comment on attachment 389321 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=389321&action=review

> Source/WebKit/ChangeLog:9
> +        r252185 changed the early return in WKNetworkSessionDelegate's -â¦task:didReceiveChallenge:⦠method

non-ascii in the changelog

[David Quesada]

Created attachment 389428 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389428 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389428 [details]:

imported/w3c/web-platform-tests/IndexedDB/fire-success-event-exception.html bug 206554 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 389428 [details]
Patch for landing

Clearing flags on attachment: 389428

Committed r255533: <https://trac.webkit.org/changeset/255533>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

Comment on attachment 389428 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=389428&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:1131
> +    auto websiteDataStore = adoptNS([WKWebsiteDataStore defaultDataStore]);

This shouldn't adopt.
Fixing in https://bugs.webkit.org/show_bug.cgi?id=230980