Bug 217420 – Use sendWithAsyncReply for NetworkProcess::CancelDownload

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Use sendWithAsyncReply for NetworkProcess::CancelDownload

[Alex Christensen]

Created attachment 410728 [details]
Patch

[Alex Christensen]

Created attachment 410734 [details]
Patch

[Peng Liu]

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?

[Alex Christensen]

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.

[Peng Liu]

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!

[youenn fablet]

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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 410894 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 410906 [details]
Patch

[Alex Christensen]

Created attachment 410907 [details]
Patch

[youenn fablet]

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!

[Alex Christensen]

(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.

[Alex Christensen]

(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.

[youenn fablet]

Comment on attachment 410907 [details]
Patch

ok, let's go

[Alex Christensen]

Created attachment 410940 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r268261

[Radar WebKit Bug Importer]

<rdar://problem/70141283>