Bug 175832 – Introduce a new CompletionHandler type and use it for NetworkDataTaskClient's completion handlers to help catch bugs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Make NetworkDataTaskClient completion handlers less error prone by introducing a wrapper for them. This wrapper makes sure that:
1. The completion handler is always called
2. The completion handler is only called once

We've been caught by this several times in the past and it causes hard to diagnose issues such as memory leaks and network load hangs.

[Alex Christensen]

We should do this for a lot of things.

[Chris Dumez]

Created attachment 318768 [details]
Patch

[Alex Christensen]

Comment on attachment 318768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318768&action=review

I think we should solve this problem instead by just making a wrapper of WTF::Function that counts how many times it's called in a debug build and asserts that it is called exactly once before it is destroyed

> Source/WebKit/NetworkProcess/NetworkDataTask.h:66
> +    NetworkCompletionHandler(HandlerType&& handler, WTF::Function<void(HandlerType&)>&& defaultHandler)

This is strange.

> Source/WebKit/NetworkProcess/PingLoad.cpp:-108
> -        completionHandler({ });

I think we should keep these in.

> Source/WebKit/NetworkProcess/capture/NetworkDataTaskReplay.cpp:206
> +    RedirectCompletionHandler handler = makeRedirectCompletionHandler([this, protectedThis = makeRef(*this)] (const WebCore::ResourceRequest& updatedRequest) {

auto redirectCompletionHandler

[Chris Dumez]

Comment on attachment 318768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318768&action=review

> Source/WebKit/NetworkProcess/NetworkLoad.cpp:-138
> -    if (m_redirectCompletionHandler)

You really think we should keep these? This is super ugly. Adding assertion will not necessarily help in edge / rare cases because they will not be hit and we will not find the bugs. I'd rather make this less error prone.

[Chris Dumez]

Comment on attachment 318768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318768&action=review

>> Source/WebKit/NetworkProcess/NetworkDataTask.h:66
>> +    NetworkCompletionHandler(HandlerType&& handler, WTF::Function<void(HandlerType&)>&& defaultHandler)
> 
> This is strange.

Maybe we can clarify the naming but I think it makes sense to have a completion handler with default handling if the client does not call it.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 318768 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318768&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkLoad.cpp:-138
> > -    if (m_redirectCompletionHandler)
> 
> You really think we should keep these? This is super ugly. Adding assertion
> will not necessarily help in edge / rare cases because they will not be hit
> and we will not find the bugs. I'd rather make this less error prone.

To be clear, these completion handlers happen in case of redirects and authentication which are not common cases. It is very easy to introduce a bug where the redirect / authentication handlers do not call the completion handler, especially in specific error handling code path. Those do not have good test coverage and are not easy to test for. Therefore, I think merely adding assertion is not going to be sufficient to find bugs. Also, this leads to some ugly code where you have to make sure you call completion handler in class destructors.

[Chris Dumez]

Created attachment 318809 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 318809 [details]
Patch

Clearing flags on attachment: 318809

Committed r221059: <http://trac.webkit.org/changeset/221059>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34025346>

[Daniel Bates]

Comment on attachment 318809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318809&action=review

> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:106
> +            completionHandler({ });

This will pass PolicyAction::PolicyUse to the completion handler. Is this intentional?

[Chris Dumez]

Comment on attachment 318809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318809&action=review

>> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:106
>> +            completionHandler({ });
> 
> This will pass PolicyAction::PolicyUse to the completion handler. Is this intentional?

Good catch. This is not intentional. I'll follow-up.

[Daniel Bates]

Comment on attachment 318809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318809&action=review

>>> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:106
>>> +            completionHandler({ });
>> 
>> This will pass PolicyAction::PolicyUse to the completion handler. Is this intentional?
> 
> Good catch. This is not intentional. I'll follow-up.

What about the ordering of calling the completion handler with respect to calling cancel()? Is it acceptable to call the completion handler first?