Bug 175872

Created attachment 318850 [details] Patch

Comment on attachment 318850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318850&action=review > Source/WebKit/ChangeLog:3 > + Regression(r221059): NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9 Can this be tested?

(In reply to Alexey Proskuryakov from comment #2) > Comment on attachment 318850 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318850&action=review > > > Source/WebKit/ChangeLog:3 > > + Regression(r221059): NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9 > > Can this be tested? I will look into more detail tomorrow but I doubt it since we cancel right after the completion handler call. The completion handler is mostly so that CFNetwork does not leak stuff. There are 4 tests exercising this code path, all of which are still passing.

(In reply to Chris Dumez from comment #3) > (In reply to Alexey Proskuryakov from comment #2) > > Comment on attachment 318850 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=318850&action=review > > > > > Source/WebKit/ChangeLog:3 > > > + Regression(r221059): NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9 > > > > Can this be tested? > > I will look into more detail tomorrow but I doubt it since we cancel right > after the completion handler call. The completion handler is mostly so that > CFNetwork does not leak stuff. There are 4 tests exercising this code path, > all of which are still passing. To be clear, I do not believe this effectively changes anything from WebKit point of view. However, it feels cleaner to use ignore policy here and it may avoid some unnecessary work at CFNetwork level.

Comment on attachment 318850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318850&action=review > Source/WebKit/ChangeLog:8 > + Use PolicyIgnore instead. Please explain why.

Created attachment 318875 [details] Patch

Comment on attachment 318875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318875&action=review OK. > Source/WebKit/NetworkProcess/NetworkDataTask.cpp:106 > - completionHandler({ }); > + completionHandler(PolicyIgnore); Notice that PolicyIgnore is translated to NSURLSessionResponseCancel. I would have hoped that NSURLSession and URLSession:dataTask:didReceiveResponse:completionHandler: would have been coded such that calling -[NSURLSessionDataTask cancel] (what cancel() does for the non-Blob case) would be equivalent to invoking the completion handler with NSURLSessionResponseCancel and hence not necessitate explicitly calling the completion handler. Is it necessary to also call cancel()? is it good practice?

Out of curiosity, did you see a leak or bad behavior because we were not calling the completion handler in NetworkDataTask::didReceiveResponse()?

(In reply to Daniel Bates from comment #8) > Out of curiosity, did you see a leak or bad behavior because we were not > calling the completion handler in NetworkDataTask::didReceiveResponse()? I have not measured a leak for this particular handler. However, we have been caught by this several times before with other networking completion handlers. For e.g. Bug 175179 which was a bad leak case. We've also had cases of network hangs due to us not calling the completion handlers (supposedly because of leaking open connections and reaching a limit for open connections). Because this has happened several times and because the issues are hard to diagnose, we have introduced this new CompletionHandler type that helps catch such bugs.

(In reply to Chris Dumez from comment #9) > (In reply to Daniel Bates from comment #8) > > Out of curiosity, did you see a leak or bad behavior because we were not > > calling the completion handler in NetworkDataTask::didReceiveResponse()? > > I have not measured a leak for this particular handler. > > However, we have been caught by this several times before with other > networking completion handlers. For e.g. Bug 175179 which was a bad leak > case. We've also had cases of network hangs due to us not calling the > completion handlers (supposedly because of leaking open connections and > reaching a limit for open connections). > > Because this has happened several times and because the issues are hard to > diagnose, we have introduced this new CompletionHandler type that helps > catch such bugs. Forgot to mention, that 4 tests crash in debug builds now that we use CompletionHandler if we fail to call completion handler here.

Comment on attachment 318875 [details] Patch Clearing flags on attachment: 318875 Committed r221081: <http://trac.webkit.org/changeset/221081>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34037374>