175872 2017-08-22 21:07:07 -0700 REGRESSION (r221059): NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9 2017-08-23 10:15:15 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build All All RESOLVED FIXED InRadar, Regression P2 Normal --- 175832 1 cdumez cdumez achristensen ap commit-queue dbates webkit-bug-importer oldest_to_newest 1341444 0 cdumez 2017-08-22 21:07:07 -0700 NetworkDataTask::didReceiveResponse() should not use PolicyUse for HTTP/0.9. 1341445 1 318850 cdumez 2017-08-22 21:08:20 -0700 Created attachment 318850 Patch 1341457 2 318850 ap 2017-08-22 22:32:46 -0700 Comment on attachment 318850 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? 1341458 3 cdumez 2017-08-22 22:41:27 -0700 (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. 1341459 4 cdumez 2017-08-22 22:47:14 -0700 (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. 1341476 5 318850 dbates 2017-08-23 01:06:12 -0700 Comment on attachment 318850 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318850&action=review > Source/WebKit/ChangeLog:8 > + Use PolicyIgnore instead. Please explain why. 1341552 6 318875 cdumez 2017-08-23 09:04:09 -0700 Created attachment 318875 Patch 1341567 7 318875 dbates 2017-08-23 09:39:41 -0700 Comment on attachment 318875 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? 1341568 8 dbates 2017-08-23 09:41:14 -0700 Out of curiosity, did you see a leak or bad behavior because we were not calling the completion handler in NetworkDataTask::didReceiveResponse()? 1341570 9 cdumez 2017-08-23 09:45:28 -0700 (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. 1341571 10 cdumez 2017-08-23 09:46:31 -0700 (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. 1341584 11 318875 commit-queue 2017-08-23 10:14:12 -0700 Comment on attachment 318875 Patch Clearing flags on attachment: 318875 Committed r221081: <http://trac.webkit.org/changeset/221081> 1341585 12 commit-queue 2017-08-23 10:14:14 -0700 All reviewed patches have been landed. Closing bug. 1341586 13 webkit-bug-importer 2017-08-23 10:15:15 -0700 <rdar://problem/34037374> 318850 2017-08-22 21:08:20 -0700 2017-08-23 09:04:07 -0700 Patch bug-175872-20170822210819.patch text/plain 1700 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjIxMDY1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDIzMzhjNTgxYTI1YTY3N2Mx YWI3ZTNlY2VhZjYwMGNmNjg4YmFiMzEuLjg5ZjlhYTJkMGQyZWEyNTg2YTI0NDk1N2U0OGExNTM1 NDQ5YjBmNGEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTctMDgtMjIgIENocmlzIER1 bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4KKworICAgICAgICBSZWdyZXNzaW9uKHIyMjEwNTkpOiBO ZXR3b3JrRGF0YVRhc2s6OmRpZFJlY2VpdmVSZXNwb25zZSgpIHNob3VsZCBub3QgdXNlIFBvbGlj eVVzZSBmb3IgSFRUUC8wLjkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTE3NTg3MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIFVzZSBQb2xpY3lJZ25vcmUgaW5zdGVhZC4KKworICAgICAgICAqIE5ldHdvcmtQ cm9jZXNzL05ldHdvcmtEYXRhVGFzay5jcHA6CisgICAgICAgIChXZWJLaXQ6Ok5ldHdvcmtEYXRh VGFzazo6ZGlkUmVjZWl2ZVJlc3BvbnNlKToKKwogMjAxNy0wOC0yMCAgV2Vuc29uIEhzaWVoICA8 d2Vuc29uX2hzaWVoQGFwcGxlLmNvbT4KIAogICAgICAgICBbaU9TIFdLMl0gV0tXZWJWaWV3IHNj aGVkdWxlcyBub25zdG9wIGxheW91dCBhZnRlciBwcmVzc2luZyBjbWIrYixpLHUgaW5zaWRlIGEg Y29udGVudGVkaXRhYmxlIGRpdgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9OZXR3b3JrUHJv Y2Vzcy9OZXR3b3JrRGF0YVRhc2suY3BwIGIvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9O ZXR3b3JrRGF0YVRhc2suY3BwCmluZGV4IGEwMzNkYzRhNDhlY2RkYzRlMjNlZjQ3NmE4YWUzMTlh ZGE2NzYyNTUuLmVhYWUzZGE4ZThjM2I5OWU0ZTU2YWFkYTRhNGViNDU4YjBmMzIxMGQgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvTmV0d29ya0RhdGFUYXNrLmNwcAor KysgYi9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL05ldHdvcmtEYXRhVGFzay5jcHAKQEAg LTEwMyw3ICsxMDMsNyBAQCB2b2lkIE5ldHdvcmtEYXRhVGFzazo6ZGlkUmVjZWl2ZVJlc3BvbnNl KFJlc291cmNlUmVzcG9uc2UmJiByZXNwb25zZSwgUmVzcG9uc2VDbwogICAgICAgICBhdXRvIHVy bCA9IHJlc3BvbnNlLnVybCgpOwogICAgICAgICBzdGQ6Om9wdGlvbmFsPHVpbnQxNl90PiBwb3J0 ID0gdXJsLnBvcnQoKTsKICAgICAgICAgaWYgKHBvcnQgJiYgIWlzRGVmYXVsdFBvcnRGb3JQcm90 b2NvbChwb3J0LnZhbHVlKCksIHVybC5wcm90b2NvbCgpKSkgewotICAgICAgICAgICAgY29tcGxl dGlvbkhhbmRsZXIoeyB9KTsKKyAgICAgICAgICAgIGNvbXBsZXRpb25IYW5kbGVyKFBvbGljeUln bm9yZSk7CiAgICAgICAgICAgICBjYW5jZWwoKTsKICAgICAgICAgICAgIG1fY2xpZW50LT5kaWRD b21wbGV0ZVdpdGhFcnJvcih7IFN0cmluZygpLCAwLCB1cmwsICJDYW5jZWxsZWQgbG9hZCBmcm9t ICciICsgdXJsLnN0cmluZ0NlbnRlckVsbGlwc2l6ZWRUb0xlbmd0aCgpICsgIicgYmVjYXVzZSBp dCBpcyB1c2luZyBIVFRQLzAuOS4iIH0pOwogICAgICAgICAgICAgcmV0dXJuOwo= 318875 2017-08-23 09:04:09 -0700 2017-08-23 10:14:12 -0700 Patch bug-175872-20170823090408.patch text/plain 2098 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjIxMDc1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDRiMGUxNWUyNjEwZjEwZDE3 NTU2MTk1NWM5YzNlMzY2MTUyMTRmZWYuLjUzNmI3ZDVmYmQzYjNkMzQ1NDRhNTdhZDE4NTIzNjQw NmVkMTY3OWEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEgQEAKKzIwMTctMDgtMjMgIENocmlzIER1 bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4KKworICAgICAgICBSZWdyZXNzaW9uKHIyMjEwNTkpOiBO ZXR3b3JrRGF0YVRhc2s6OmRpZFJlY2VpdmVSZXNwb25zZSgpIHNob3VsZCBub3QgdXNlIFBvbGlj eVVzZSBmb3IgSFRUUC8wLjkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTE3NTg3MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIHIyMjEwNTkgd2FzIGNhbGxpbmcgdGhlIFJlc3BvbnNlQ29tcGxldGlvbkhhbmRs ZXIgd2l0aCB7fSB3aGljaCBlbmRlZCB1cCBiZWluZworICAgICAgICBQb2xpY3lVc2UuIFNpbmNl IHRoaXMgaXMgYW4gZXJyb3IgY2FzZSBhbmQgZG8gbm90IHdhbnQgdG8gcmVjZWl2ZSB0aGUgZGF0 YSwgaXQKKyAgICAgICAgbWFrZXMgbW9yZSBzZW5zZSB0byB1c2UgUG9saWN5SWdub3JlIGluc3Rl YWQuIFRoZXJlIHNob3VsZCBub3QgYmUgYSBiZWhhdmlvcgorICAgICAgICBjaGFuZ2Ugb24gV2Vi S2l0IHNpZGUgdGhvdWdoIHNpbmNlIHdlIGFyZSBjYW5jZWxsaW5nIHRoZSBsb2FkIHJpZ2h0IGFm dGVyCisgICAgICAgIGNhbGxpbmcgdGhlIGNvbXBsZXRpb24gaGFuZGxlciBhbnl3YXkuCisKKyAg ICAgICAgVGVzdHMgdW5kZXIgaHR0cC90ZXN0cy9zZWN1cml0eS9odHRwLTAuOS8gYXJlIHN0aWxs IHBhc3NpbmcuCisKKyAgICAgICAgKiBOZXR3b3JrUHJvY2Vzcy9OZXR3b3JrRGF0YVRhc2suY3Bw OgorICAgICAgICAoV2ViS2l0OjpOZXR3b3JrRGF0YVRhc2s6OmRpZFJlY2VpdmVSZXNwb25zZSk6 CisKIDIwMTctMDgtMjIgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CiAK ICAgICAgICAgX1dLVGh1bWJuYWlsVmlldyBzaG91bGQgdXNlIHRoZSBzY3JlZW4gY29sb3Igc3Bh Y2UgaW5zdGVhZCBvZiBzUkdCCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9j ZXNzL05ldHdvcmtEYXRhVGFzay5jcHAgYi9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL05l dHdvcmtEYXRhVGFzay5jcHAKaW5kZXggYTAzM2RjNGE0OGVjZGRjNGUyM2VmNDc2YThhZTMxOWFk YTY3NjI1NS4uZWFhZTNkYThlOGMzYjk5ZTRlNTZhYWRhNGE0ZWI0NThiMGYzMjEwZCAxMDA2NDQK LS0tIGEvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9OZXR3b3JrRGF0YVRhc2suY3BwCisr KyBiL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvTmV0d29ya0RhdGFUYXNrLmNwcApAQCAt MTAzLDcgKzEwMyw3IEBAIHZvaWQgTmV0d29ya0RhdGFUYXNrOjpkaWRSZWNlaXZlUmVzcG9uc2Uo UmVzb3VyY2VSZXNwb25zZSYmIHJlc3BvbnNlLCBSZXNwb25zZUNvCiAgICAgICAgIGF1dG8gdXJs ID0gcmVzcG9uc2UudXJsKCk7CiAgICAgICAgIHN0ZDo6b3B0aW9uYWw8dWludDE2X3Q+IHBvcnQg PSB1cmwucG9ydCgpOwogICAgICAgICBpZiAocG9ydCAmJiAhaXNEZWZhdWx0UG9ydEZvclByb3Rv Y29sKHBvcnQudmFsdWUoKSwgdXJsLnByb3RvY29sKCkpKSB7Ci0gICAgICAgICAgICBjb21wbGV0 aW9uSGFuZGxlcih7IH0pOworICAgICAgICAgICAgY29tcGxldGlvbkhhbmRsZXIoUG9saWN5SWdu b3JlKTsKICAgICAgICAgICAgIGNhbmNlbCgpOwogICAgICAgICAgICAgbV9jbGllbnQtPmRpZENv bXBsZXRlV2l0aEVycm9yKHsgU3RyaW5nKCksIDAsIHVybCwgIkNhbmNlbGxlZCBsb2FkIGZyb20g JyIgKyB1cmwuc3RyaW5nQ2VudGVyRWxsaXBzaXplZFRvTGVuZ3RoKCkgKyAiJyBiZWNhdXNlIGl0 IGlzIHVzaW5nIEhUVFAvMC45LiIgfSk7CiAgICAgICAgICAgICByZXR1cm47Cg==