Bug 206984

Summary: REGRESSION(r252185): NetworkSessionCocoa cancels downloads that receive authentication challenges
Product: WebKit Reporter: David Quesada <david_quesada>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, commit-queue, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 208029    
Bug Blocks:    
Attachments:
Description Flags
Patch
youennf: review+
Patch v2
none
Patch v3
none
Patch v4
beidson: review+
Patch for landing none

David Quesada
Reported 2020-01-29 20:19:35 PST
rdar://problem/58999654
Attachments
Patch (6.53 KB, patch)
2020-01-29 21:29 PST, David Quesada 		youennf: review+
DetailsFormatted DiffDiff
Patch v2 (13.36 KB, patch)
2020-01-30 17:34 PST, David Quesada 		no flags
DetailsFormatted DiffDiff
Patch v3 (13.38 KB, patch)
2020-01-30 17:44 PST, David Quesada 		no flags
DetailsFormatted DiffDiff
Patch v4 (13.34 KB, patch)
2020-01-30 18:10 PST, David Quesada 		beidson: review+
DetailsFormatted DiffDiff
Patch for landing (13.34 KB, patch)
2020-01-31 15:32 PST, David Quesada 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
David Quesada
Comment 1 2020-01-29 21:29:56 PST
Created attachment 389228 [details] Patch
youenn fablet
Comment 2 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.
Brady Eidson
Comment 3 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.
David Quesada
Comment 4 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.
David Quesada
Comment 5 2020-01-30 17:44:41 PST
Created attachment 389318 [details] Patch v3 Rebased.
Alex Christensen
Comment 6 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)?
David Quesada
Comment 7 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.
David Quesada
Comment 8 2020-01-30 18:10:20 PST
Created attachment 389321 [details] Patch v4
Brady Eidson
Comment 9 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
David Quesada
Comment 10 2020-01-31 15:32:13 PST
Created attachment 389428 [details] Patch for landing
WebKit Commit Bot
Comment 11 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.
WebKit Commit Bot
Comment 12 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.
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2020-01-31 16:48:04 PST
All reviewed patches have been landed. Closing bug.
Alex Christensen
Comment 15 2021-09-29 13:45:11 PDT
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
Note You need to log in before you can comment on or make changes to this bug.