Summary: | Use sendWithAsyncReply for NetworkProcess::CancelDownload | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | peng.liu6, webkit-bug-importer, youennf | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2020-10-06 20:27:15 PDT
Created attachment 410728 [details]
Patch
Created attachment 410734 [details]
Patch
Comment on attachment 410734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410734&action=review > Source/WebKit/NetworkProcess/Downloads/Download.cpp:170 > + ASSERT_NOT_REACHED(); Will this be an issue in the case when we cancel a download without a completion handler? Comment on attachment 410734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410734&action=review >> Source/WebKit/NetworkProcess/Downloads/Download.cpp:170 >> + ASSERT_NOT_REACHED(); > > Will this be an issue in the case when we cancel a download without a completion handler? No. Those now call didFail. Comment on attachment 410734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410734&action=review >>> Source/WebKit/NetworkProcess/Downloads/Download.cpp:170 >>> + ASSERT_NOT_REACHED(); >> >> Will this be an issue in the case when we cancel a download without a completion handler? > > No. Those now call didFail. I see, thanks! Comment on attachment 410734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410734&action=review > Source/WebKit/NetworkProcess/Downloads/Download.cpp:95 > + if (completionHandler) { Why do we need a check for completionHandler. Can we just write: if (m_wasCanceled) { completionHandler({ }); return; } m_wasCanceled = true; m_cancelCompletionHandler = WTFMove(completionHandler); > Source/WebKit/NetworkProcess/Downloads/Download.cpp:171 > I would write it as: ASSERT(m_cancelCompletionHandler) if (m_cancelCompletionHandler) m_cancelCompletionHandler(resumeData); Why do we need the ASSERT though. Can we just try to ship without? > Source/WebKit/NetworkProcess/Downloads/DownloadMonitor.cpp:121 > + m_download.cancel(nullptr); Can we write it as m_download.cancel([] { }) instead? We could add a default value in cancel declaration if needed. > Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm:49 > + m_download->cancel(nullptr); I would go with a default value instead of nullptr. > Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:69 > + return API::Data::create(data.data(), data.size()); One liner? > Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:772 > +- (void)_download:(_WKDownload *)download didFailWithError:(NSError *)error We do not check for errors in the test. It would be nice to validate this is a 'cancel' error. Comment on attachment 410734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410734&action=review >> Source/WebKit/NetworkProcess/Downloads/DownloadMonitor.cpp:121 >> + m_download.cancel(nullptr); > > Can we write it as m_download.cancel([] { }) instead? > We could add a default value in cancel declaration if needed. I was trying to make a difference between cancelling from the API which will want a resume data response and cancelling from here or NSProgress, which will call didFailWithError. I'm doing this a cleaner way in the next patch. >> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:69 >> + return API::Data::create(data.data(), data.size()); > > One liner? That would require an explicit call to the RefPtr<API::Data> constructor because API::Data::create returns a Ref<API::Data> and the ?: operator requires the same type for the last two arguments and the first one would be nullptr. I think this looks better, and it's identical to what we had. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:772 >> +- (void)_download:(_WKDownload *)download didFailWithError:(NSError *)error > > We do not check for errors in the test. It would be nice to validate this is a 'cancel' error. Good idea. I'm adding that check. Created attachment 410894 [details]
Patch
Comment on attachment 410894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410894&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:730 > + WTFLogAlways("didCompleteWithError %@", error); Will remove these logs before landing. Created attachment 410906 [details]
Patch
Created attachment 410907 [details]
Patch
Comment on attachment 410907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410907&action=review > Source/WebKit/ChangeLog:11 > + cause any problems in practice. That's only used by Safari and behaves similarly. This might need more explanation > Source/WebKit/NetworkProcess/Downloads/Download.cpp:96 > + m_ignoreDidFail = ignoreDidFail; There could be two calls with cancel({}, true) and cancel({}, false), the latter would override m_ignoreDidFail. Can we capture ignoreDidFail and add a different check in didFail>? For instance, if there is a guarantee that completionHandlerWrapper is called before Download::didFail, we could add a check in didFail to see whether downLoadManager knows that the download is finished. > Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm:49 > + m_download->cancel([](auto&) { }, false); It is not easy to understand why we use false here. Maybe an enum would help, maybe add more info in ChangeLog. > Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:822 > + EXPECT_EQ(error.code, NSURLErrorCancelled); Looks good! (In reply to youenn fablet from comment #12) > Comment on attachment 410907 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410907&action=review > > > Source/WebKit/ChangeLog:11 > > + cause any problems in practice. That's only used by Safari and behaves similarly. > > This might need more explanation > > > Source/WebKit/NetworkProcess/Downloads/Download.cpp:96 > > + m_ignoreDidFail = ignoreDidFail; > > There could be two calls with cancel({}, true) and cancel({}, false), the > latter would override m_ignoreDidFail. That hypothetical future code would be a bug, and you would know because the first one would never receive didFail or didCancel. Since there are only two calls here, I've made sure both of them are correct, and tests cover that. > Can we capture ignoreDidFail and add a different check in didFail>? > For instance, if there is a guarantee that completionHandlerWrapper is > called before Download::didFail, we could add a check in didFail to see > whether downLoadManager knows that the download is finished. The problem is that when you tell CFNetwork to cancel a download, it tells you twice that it's done, once in the completionHandler of cancelByProducingResumeData and once in your NSURLSessionDelegate's didCompleteWithError. Our API should inform the UI process once that the download ended. I've implemented two different ways of making sure the didCompleteWithError is ignored if you cancelled the download, and I think this is the better one. We still need didCompleteWithError because sometimes downloads just fail without you cancelling them, like if the network connection is lost. > > > Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm:49 > > + m_download->cancel([](auto&) { }, false); > > It is not easy to understand why we use false here. > Maybe an enum would help, maybe add more info in ChangeLog. I'll use an enum class IgnoreDidFail : bool { No, Yes } instead of this bool. (In reply to Alex Christensen from comment #13) > (In reply to youenn fablet from comment #12) > > Comment on attachment 410907 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=410907&action=review > > > > > Source/WebKit/ChangeLog:11 > > > + cause any problems in practice. That's only used by Safari and behaves similarly. > > > > This might need more explanation Safari is the only API client that uses this, and it behaves similarly in didCancel and didFailWithError. I'll include this in the change log. Comment on attachment 410907 [details]
Patch
ok, let's go
Created attachment 410940 [details]
Patch
|