Bug 194823 - Always call CompletionHandlers after r240909
Summary: Always call CompletionHandlers after r240909
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-19 11:40 PST by Alex Christensen
Modified: 2019-02-20 14:59 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.73 KB, patch)
2019-02-19 11:40 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (1.73 KB, patch)
2019-02-19 12:57 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-02-19 11:40:39 PST
Always call CompletionHandlers after r240909
Comment 1 Alex Christensen 2019-02-19 11:40:59 PST
Created attachment 362404 [details]
Patch
Comment 2 Ryosuke Niwa 2019-02-19 12:23:27 PST
Looks like the patch isn't building??
Comment 3 Alex Christensen 2019-02-19 12:57:20 PST
Created attachment 362417 [details]
Patch
Comment 4 Ryosuke Niwa 2019-02-19 13:48:12 PST
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.
Comment 5 Alex Christensen 2019-02-19 14:18:23 PST
We definitely need to do something with the CompletionHandlers.  If we don't call them we need to store them somewhere.
Comment 6 Ryosuke Niwa 2019-02-19 19:01:50 PST
(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.
Comment 7 Ryosuke Niwa 2019-02-19 19:02:55 PST
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?
Comment 8 Alex Christensen 2019-02-20 10:17:38 PST
No
Comment 9 Ryosuke Niwa 2019-02-20 13:35:38 PST
(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?
Comment 10 Alex Christensen 2019-02-20 14:05:04 PST
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.
Comment 11 Ryosuke Niwa 2019-02-20 14:07:29 PST
(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.
Comment 12 WebKit Commit Bot 2019-02-20 14:52:24 PST
Comment on attachment 362417 [details]
Patch

Clearing flags on attachment: 362417

Committed r241842: <https://trac.webkit.org/changeset/241842>
Comment 13 WebKit Commit Bot 2019-02-20 14:52:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-02-20 14:59:53 PST
<rdar://problem/48253351>