Bug 194823 – Always call CompletionHandlers after r240909

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Always call CompletionHandlers after r240909

[Alex Christensen]

Created attachment 362404 [details]
Patch

[Ryosuke Niwa]

Looks like the patch isn't building??

[Alex Christensen]

Created attachment 362417 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 362417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362417&action=review

> Source/WebCore/loader/PolicyChecker.cpp:196
>          if (!responseIdentifier.isValidFor(requestIdentifier))
> -            return;
> +            return function({ }, nullptr, NavigationPolicyDecision::IgnoreLoad);

I don't think this is quite right.

When the response identifier is less than what we have,
it means the response was for some other resource we had requested a policy check in the past.
It’s still possible to receive the right policy check response in the future.

[Alex Christensen]

We definitely need to do something with the CompletionHandlers.  If we don't call them we need to store them somewhere.

[Ryosuke Niwa]

(In reply to Alex Christensen from comment #5)
> We definitely need to do something with the CompletionHandlers.  If we don't
> call them we need to store them somewhere.

Oh, that's a good point. Maybe we DO need to fail in this case because we don't store the completion handler anywhere.

[Ryosuke Niwa]

But hang on, if we did that, wouldn't it mean that WebKit layer's listener map would still contain a stale completion handler which would never get called anyway?

[Alex Christensen]

No

[Ryosuke Niwa]

(In reply to Alex Christensen from comment #8)
> No

Why not? If we've reached this point, it means that we've got listenerID confused, and invoked the wrong listener with a policy check response. That would mean that the other listener for which this policy check response was really meant for would be left in the map. How does the other listener ever get a policy check response?

[Alex Christensen]

listenerID corresponds to WebFrame's m_policyFunction, and if they don't line up then WebFrame::invalidatePolicyListener calls the CompletionHandler owned by the WebFrame in that case.  The CompletionHandlers being fixed by this patch are not owned by anything.  They need to be called or owned.

[Ryosuke Niwa]

(In reply to Alex Christensen from comment #10)
> listenerID corresponds to WebFrame's m_policyFunction, and if they don't
> line up then WebFrame::invalidatePolicyListener calls the CompletionHandler
> owned by the WebFrame in that case.

Oh, that's a good point.

[WebKit Commit Bot]

Comment on attachment 362417 [details]
Patch

Clearing flags on attachment: 362417

Committed r241842: <https://trac.webkit.org/changeset/241842>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48253351>