Bug 206984 - REGRESSION(r252185): NetworkSessionCocoa cancels downloads that receive authentication challenges
Summary: REGRESSION(r252185): NetworkSessionCocoa cancels downloads that receive authe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 208029
Blocks:
  Show dependency treegraph
 
Reported: 2020-01-29 20:19 PST by David Quesada
Modified: 2020-02-20 13:17 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.53 KB, patch)
2020-01-29 21:29 PST, David Quesada
youennf: review+
Details | Formatted Diff | Diff
Patch v2 (13.36 KB, patch)
2020-01-30 17:34 PST, David Quesada
no flags Details | Formatted Diff | Diff
Patch v3 (13.38 KB, patch)
2020-01-30 17:44 PST, David Quesada
no flags Details | Formatted Diff | Diff
Patch v4 (13.34 KB, patch)
2020-01-30 18:10 PST, David Quesada
beidson: review+
Details | Formatted Diff | Diff
Patch for landing (13.34 KB, patch)
2020-01-31 15:32 PST, David Quesada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Quesada 2020-01-29 20:19:35 PST
rdar://problem/58999654
Comment 1 David Quesada 2020-01-29 21:29:56 PST
Created attachment 389228 [details]
Patch
Comment 2 youenn fablet 2020-01-30 05:49:57 PST
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.
Comment 3 Brady Eidson 2020-01-30 09:44:01 PST
(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.
Comment 4 David Quesada 2020-01-30 17:34:03 PST
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.
Comment 5 David Quesada 2020-01-30 17:44:41 PST
Created attachment 389318 [details]
Patch v3

Rebased.
Comment 6 Alex Christensen 2020-01-30 17:47:09 PST
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)?
Comment 7 David Quesada 2020-01-30 17:52:49 PST
(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.
Comment 8 David Quesada 2020-01-30 18:10:20 PST
Created attachment 389321 [details]
Patch v4
Comment 9 Brady Eidson 2020-01-31 15:25:50 PST
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
Comment 10 David Quesada 2020-01-31 15:32:13 PST
Created attachment 389428 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2020-01-31 16:30:28 PST
The commit-queue encountered the following flaky tests while processing attachment 389428 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2020-01-31 16:30:36 PST
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: shvaikalesh@gmail.com and youennf@gmail.com)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2020-01-31 16:48:02 PST
Comment on attachment 389428 [details]
Patch for landing

Clearing flags on attachment: 389428

Committed r255533: <https://trac.webkit.org/changeset/255533>
Comment 14 WebKit Commit Bot 2020-01-31 16:48:04 PST
All reviewed patches have been landed.  Closing bug.