207720 2020-02-13 14:06:47 -0800 Add missing call to completionHandler 2020-02-13 16:54:29 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=179641 InRadar P2 Normal --- 1 krollin krollin achristensen cdumez commit-queue darin dbates ews-watchlist japhet webkit-bug-importer youennf oldest_to_newest 1618704 0 krollin 2020-02-13 14:06:47 -0800 Bug 179641 (r225702) updated SubresourceLoader::willSendRequestInternal with a return call without first calling the completionHandler. Address this by adding the call to the completionHandler. 1618705 1 webkit-bug-importer 2020-02-13 14:07:02 -0800 <rdar://problem/59436915> 1618708 2 390686 krollin 2020-02-13 14:20:19 -0800 Created attachment 390686 Patch 1618751 3 390686 achristensen 2020-02-13 15:33:11 -0800 Comment on attachment 390686 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390686&action=review > Source/WebCore/loader/SubresourceLoader.cpp:242 > + return completionHandler(WTFMove(newRequest)); If we reached terminal state, shouldn't we return completionHandler({ }); to not go through with the redirection? 1618763 4 390686 commit-queue 2020-02-13 15:53:42 -0800 Comment on attachment 390686 Patch Clearing flags on attachment: 390686 Committed r256569: <https://trac.webkit.org/changeset/256569> 1618764 5 commit-queue 2020-02-13 15:53:43 -0800 All reviewed patches have been landed. Closing bug. 1618779 6 390686 cdumez 2020-02-13 16:38:17 -0800 Comment on attachment 390686 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390686&action=review >> Source/WebCore/loader/SubresourceLoader.cpp:242 >> + return completionHandler(WTFMove(newRequest)); > > If we reached terminal state, shouldn't we return completionHandler({ }); to not go through with the redirection? What Keith did seems consistent with other early return cases for error cases in this method. 1618782 7 390686 darin 2020-02-13 16:43:09 -0800 Comment on attachment 390686 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390686&action=review > Source/WebCore/ChangeLog:15 > + No new tests -- it's not possible to control execution into the > + affected code path. Seems surprising. Why not? A limitation in our test machinery? 1618786 8 390686 cdumez 2020-02-13 16:54:29 -0800 Comment on attachment 390686 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390686&action=review >> Source/WebCore/ChangeLog:15 >> + affected code path. > > Seems surprising. Why not? A limitation in our test machinery? If I understand the code correctly, you'd have to cancel the load just at the right time. Then since the load would be cancelled, I don't think there would be anything to observe from the web point-of-view. Since we fail to call the completion handler, the load simply will be hung on network process side I believe (likely until some timeout is reached), which again, would not be easily observable I believe. 390686 2020-02-13 14:20:19 -0800 2020-02-13 15:53:42 -0800 Patch bug-207720-20200213142018.patch text/plain 2032 krollin U3VidmVyc2lvbiBSZXZpc2lvbjogMjU2MTk0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNGY0NTc3YzA0MTUwZDBm M2E3NDIxNDFhNzdkNjRhZDkxZDcxNDY4MS4uODQwNGI2MjZkOGQzOGE1NTFjZmM4OGM4ZjFjZjlh YTYzYjQ5ZWY2NSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDIwLTAyLTEzICBLZWl0 aCBSb2xsaW4gIDxrcm9sbGluQGFwcGxlLmNvbT4KKworICAgICAgICBBZGQgbWlzc2luZyBjYWxs IHRvIGNvbXBsZXRpb25IYW5kbGVyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0yMDc3MjAKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzU5NDM2OTE1Pgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEJ1ZyAxNzk2 NDEgKHIyMjU3MDIpIHVwZGF0ZWQKKyAgICAgICAgU3VicmVzb3VyY2VMb2FkZXI6OndpbGxTZW5k UmVxdWVzdEludGVybmFsIHdpdGggYSByZXR1cm4gY2FsbCB3aXRob3V0CisgICAgICAgIGZpcnN0 IGNhbGxpbmcgdGhlIGNvbXBsZXRpb25IYW5kbGVyLiBBZGRyZXNzIHRoaXMgYnkgYWRkaW5nIHRo ZSBjYWxsCisgICAgICAgIHRvIHRoZSBjb21wbGV0aW9uSGFuZGxlci4KKworICAgICAgICBObyBu ZXcgdGVzdHMgLS0gaXQncyBub3QgcG9zc2libGUgdG8gY29udHJvbCBleGVjdXRpb24gaW50byB0 aGUKKyAgICAgICAgYWZmZWN0ZWQgY29kZSBwYXRoLgorCisgICAgICAgICogbG9hZGVyL1N1YnJl c291cmNlTG9hZGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlN1YnJlc291cmNlTG9hZGVyOjp3 aWxsU2VuZFJlcXVlc3RJbnRlcm5hbCk6CisKIDIwMjAtMDItMTAgIFNhaWQgQWJvdS1IYWxsYXdh ICA8c2Fib3VoYWxsYXdhQGFwcGxlLmNvbT4KIAogICAgICAgICBVbnJlYWNoYWJsZSBjb2RlIGhp dCBpbiBXZWJDb3JlOjpTaGFwZTo6Y3JlYXRlU2hhcGUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL2xvYWRlci9TdWJyZXNvdXJjZUxvYWRlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIv U3VicmVzb3VyY2VMb2FkZXIuY3BwCmluZGV4IGI3NDgyYWE0YWFlOWZlMGJiYThkZjk3YjZlNGM3 NWFkNDQ3YzYyYWQuLjJiODEwYzY2ODJiYjMyYjU5YWYzYjgxYzM3YTRmZDliYTllY2FiMGQgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9TdWJyZXNvdXJjZUxvYWRlci5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL1N1YnJlc291cmNlTG9hZGVyLmNwcApAQCAtMjM5LDcg KzIzOSw3IEBAIHZvaWQgU3VicmVzb3VyY2VMb2FkZXI6OndpbGxTZW5kUmVxdWVzdEludGVybmFs KFJlc291cmNlUmVxdWVzdCYmIG5ld1JlcXVlc3QsIGNvCiAgICAgICAgICAgICBtX3Jlc291cmNl LT5yZXNwb25zZVJlY2VpdmVkKG9wYXF1ZVJlZGlyZWN0ZWRSZXNwb25zZSk7CiAgICAgICAgICAg ICBpZiAocmVhY2hlZFRlcm1pbmFsU3RhdGUoKSkgewogICAgICAgICAgICAgICAgIFJFTEVBU0Vf TE9HX0lGX0FMTE9XRUQoIndpbGxTZW5kUmVxdWVzdGludGVybmFsOiByZWFjaGVkIHRlcm1pbmFs IHN0YXRlIChmcmFtZSA9ICVwLCBmcmFtZUxvYWRlciA9ICVwLCByZXNvdXJjZUlEID0gJWx1KSIs IGZyYW1lKCksIGZyYW1lTG9hZGVyKCksIGlkZW50aWZpZXIoKSk7Ci0gICAgICAgICAgICAgICAg cmV0dXJuOworICAgICAgICAgICAgICAgIHJldHVybiBjb21wbGV0aW9uSGFuZGxlcihXVEZNb3Zl KG5ld1JlcXVlc3QpKTsKICAgICAgICAgICAgIH0KIAogICAgICAgICAgICAgUkVMRUFTRV9MT0df SUZfQUxMT1dFRCgid2lsbFNlbmRSZXF1ZXN0aW50ZXJuYWw6IHJlc291cmNlIGxvYWQgY29tcGxl dGVkIChmcmFtZSA9ICVwLCBmcmFtZUxvYWRlciA9ICVwLCByZXNvdXJjZUlEID0gJWx1KSIsIGZy YW1lKCksIGZyYW1lTG9hZGVyKCksIGlkZW50aWZpZXIoKSk7Cg==