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.
We should do this for a lot of things.
Created attachment 318768 [details] Patch
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
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.
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.
(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.
Created attachment 318809 [details] Patch
Comment on attachment 318809 [details] Patch Clearing flags on attachment: 318809 Committed r221059: <http://trac.webkit.org/changeset/221059>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34025346>
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?
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.
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?