Bug 175872 – REGRESSION (r221059): NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9.

[Chris Dumez]

Created attachment 318850 [details]
Patch

[Alexey Proskuryakov]

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?

[Chris Dumez]

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

[Chris Dumez]

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

[Daniel Bates]

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.

[Chris Dumez]

Created attachment 318875 [details]
Patch

[Daniel Bates]

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?

[Daniel Bates]

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

[Chris Dumez]

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

[Chris Dumez]

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

[WebKit Commit Bot]

Comment on attachment 318875 [details]
Patch

Clearing flags on attachment: 318875

Committed r221081: <http://trac.webkit.org/changeset/221081>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34037374>