Summary: | Add missing call to completionHandler | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Rollin <krollin> | ||||
Component: | WebCore Misc. | Assignee: | Keith Rollin <krollin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, cdumez, commit-queue, darin, dbates, ews-watchlist, japhet, webkit-bug-importer, youennf | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=179641 | ||||||
Attachments: |
|
Description
Keith Rollin
2020-02-13 14:06:47 PST
Created attachment 390686 [details]
Patch
Comment on attachment 390686 [details] 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? Comment on attachment 390686 [details] Patch Clearing flags on attachment: 390686 Committed r256569: <https://trac.webkit.org/changeset/256569> All reviewed patches have been landed. Closing bug. Comment on attachment 390686 [details] 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. Comment on attachment 390686 [details] 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? Comment on attachment 390686 [details] 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. |