Bug 207720 – Add missing call to completionHandler

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Keith Rollin]

Bug 179641 (r225702) updated SubresourceLoader::willSendRequestInternal with a return call without first calling the completionHandler. Address this by adding the call to the completionHandler.

[Radar WebKit Bug Importer]

<rdar://problem/59436915>

[Keith Rollin]

Created attachment 390686 [details]
Patch

[Alex Christensen]

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?

[WebKit Commit Bot]

Comment on attachment 390686 [details]
Patch

Clearing flags on attachment: 390686

Committed r256569: <https://trac.webkit.org/changeset/256569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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.

[Darin Adler]

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?

[Chris Dumez]

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.