Bug 175872 - REGRESSION (r221059): NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9
Summary: REGRESSION (r221059): NetworkDataTask::didReceiveResponse() should not use Po...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar, Regression
Depends on:
Blocks: 175832
  Show dependency treegraph
 
Reported: 2017-08-22 21:07 PDT by Chris Dumez
Modified: 2017-08-23 10:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.66 KB, patch)
2017-08-22 21:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (2.05 KB, patch)
2017-08-23 09:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-08-22 21:07:07 PDT
NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9.
Comment 1 Chris Dumez 2017-08-22 21:08:20 PDT
Created attachment 318850 [details]
Patch
Comment 2 Alexey Proskuryakov 2017-08-22 22:32:46 PDT
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?
Comment 3 Chris Dumez 2017-08-22 22:41:27 PDT
(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.
Comment 4 Chris Dumez 2017-08-22 22:47:14 PDT
(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 5 Daniel Bates 2017-08-23 01:06:12 PDT
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.
Comment 6 Chris Dumez 2017-08-23 09:04:09 PDT
Created attachment 318875 [details]
Patch
Comment 7 Daniel Bates 2017-08-23 09:39:41 PDT
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?
Comment 8 Daniel Bates 2017-08-23 09:41:14 PDT
Out of curiosity, did you see a leak or bad behavior because we were not calling the completion handler in NetworkDataTask::didReceiveResponse()?
Comment 9 Chris Dumez 2017-08-23 09:45:28 PDT
(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.
Comment 10 Chris Dumez 2017-08-23 09:46:31 PDT
(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 11 WebKit Commit Bot 2017-08-23 10:14:12 PDT
Comment on attachment 318875 [details]
Patch

Clearing flags on attachment: 318875

Committed r221081: <http://trac.webkit.org/changeset/221081>
Comment 12 WebKit Commit Bot 2017-08-23 10:14:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2017-08-23 10:15:15 PDT
<rdar://problem/34037374>