Bug 217420 - Use sendWithAsyncReply for NetworkProcess::CancelDownload
Summary: Use sendWithAsyncReply for NetworkProcess::CancelDownload
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-06 20:27 PDT by Alex Christensen
Modified: 2020-10-09 09:25 PDT (History)
3 users (show)

See Also:


Attachments
Patch (24.44 KB, patch)
2020-10-06 20:29 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (25.17 KB, patch)
2020-10-06 21:44 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (29.55 KB, patch)
2020-10-08 16:22 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (29.21 KB, patch)
2020-10-08 20:58 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (29.31 KB, patch)
2020-10-08 21:07 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (29.67 KB, patch)
2020-10-09 09:04 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2020-10-06 20:27:15 PDT
Use sendWithAsyncReply for NetworkProcess::CancelDownload
Comment 1 Alex Christensen 2020-10-06 20:29:14 PDT
Created attachment 410728 [details]
Patch
Comment 2 Alex Christensen 2020-10-06 21:44:49 PDT
Created attachment 410734 [details]
Patch
Comment 3 Peng Liu 2020-10-07 10:05:39 PDT
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 4 Alex Christensen 2020-10-07 10:06:24 PDT
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 5 Peng Liu 2020-10-07 10:19:38 PDT
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 6 youenn fablet 2020-10-08 01:08:47 PDT
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 7 Alex Christensen 2020-10-08 16:20:25 PDT
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.
Comment 8 Alex Christensen 2020-10-08 16:22:41 PDT
Created attachment 410894 [details]
Patch
Comment 9 Alex Christensen 2020-10-08 17:10:16 PDT
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.
Comment 10 Alex Christensen 2020-10-08 20:58:45 PDT
Created attachment 410906 [details]
Patch
Comment 11 Alex Christensen 2020-10-08 21:07:31 PDT
Created attachment 410907 [details]
Patch
Comment 12 youenn fablet 2020-10-09 08:42:29 PDT
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!
Comment 13 Alex Christensen 2020-10-09 08:51:19 PDT
(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.
Comment 14 Alex Christensen 2020-10-09 08:52:18 PDT
(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 15 youenn fablet 2020-10-09 09:02:49 PDT
Comment on attachment 410907 [details]
Patch

ok, let's go
Comment 16 Alex Christensen 2020-10-09 09:04:49 PDT
Created attachment 410940 [details]
Patch
Comment 17 Alex Christensen 2020-10-09 09:24:38 PDT
http://trac.webkit.org/r268261
Comment 18 Radar WebKit Bug Importer 2020-10-09 09:25:17 PDT
<rdar://problem/70141283>