Bug 237071 - Call WKNavigationDelegate.didFailProvisionalNavigation even after a cross-origin navigation with COOP
Summary: Call WKNavigationDelegate.didFailProvisionalNavigation even after a cross-ori...
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: Chris Dumez
URL:
Keywords: InRadar
: 237105 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-02-22 18:53 PST by Alex Christensen
Modified: 2022-02-27 12:30 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.42 KB, patch)
2022-02-22 18:55 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.93 KB, patch)
2022-02-25 09:19 PST, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.40 KB, patch)
2022-02-25 13:22 PST, Chris Dumez
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 2022-02-22 18:53:19 PST
Call WKNavigationDelegate.didFailProvisionalNavigation even after a cross-origin navigation with COOP
Comment 1 Alex Christensen 2022-02-22 18:55:26 PST
Created attachment 452928 [details]
Patch
Comment 2 Chris Dumez 2022-02-22 19:59:51 PST
I will be a little surprised if the ProcessSwap.ResponsePolicyDownloadAfterCOOPProcessSwap API test still passes.

I added this check not long to fix a bug and added that API test to cover the change (see Bug 233798)
Comment 3 Chris Dumez 2022-02-23 07:15:57 PST
Comment on attachment 452928 [details]
Patch

No API test failures, things must have changed since this check was added.
Comment 4 EWS 2022-02-23 07:21:42 PST
Committed r290371 (247687@main): <https://commits.webkit.org/247687@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452928 [details].
Comment 5 Radar WebKit Bug Importer 2022-02-23 07:22:17 PST
<rdar://problem/89354367>
Comment 6 Alex Christensen 2022-02-24 21:10:31 PST
Reverted in r290493
Comment 7 Alex Christensen 2022-02-24 21:10:58 PST
*** Bug 237105 has been marked as a duplicate of this bug. ***
Comment 8 Chris Dumez 2022-02-25 08:06:02 PST
So here is what's happening:
1. We navigate to https://webkit.org/path1 in Process A
2. We request a navigation to https://example.com/path3.
3. Because the navigation is cross-site, PSON triggers and starts a provisional load in Process B AND tells Process A to stop all loads (no more provisional load going on in Process A).
4. When we receive the response for https://example.com/path3, the COOP header is present is triggers yet another process swap to process C.
5. We start a provisional load in process C and Process B actually goes away because it is associated with a ProvisionalPageProxy and the WebPageProxy can only have one ProvisionalPageProxy at any point, and we are constructing a new ProvisionalPageProxy for process C.
6. So at this point, we only have committed Process A and provisional Process C. There is no longer a provisional load going on in Process A (it was going on in Process B which was canceled before it got committed).
6. Then the client blocks the navigation via DecidePolicyForNavigationResponse which causes the ProvisionalPageProxy to go away. Normally, destroying the ProvisionalPageProxy would stop the provisional load in the committed process and cause a didFailProvisionalLoad to get sent to the client. However, in this case, it is not happening since there is no longer a provisional load going on in Process A (because PSON and the provisional load to process B messed with that).

I am looking into how to best fix this.
Comment 9 Chris Dumez 2022-02-25 09:19:01 PST
Created attachment 453223 [details]
Patch
Comment 10 Chris Dumez 2022-02-25 12:03:13 PST
Comment on attachment 453223 [details]
Patch

Looks like I messed up something. Will investigate.
Comment 11 Chris Dumez 2022-02-25 13:22:47 PST
Created attachment 453252 [details]
Patch
Comment 12 Darin Adler 2022-02-27 08:17:16 PST
Comment on attachment 453252 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1839
> +    SetForScope<bool> isStoppingLoadingDueToProcessSwap(m_isStoppingLoadingDueToProcessSwap, true);

Do we still need to write <bool>? If so, maybe we should add deduction guides so we don’t.
Comment 13 EWS 2022-02-27 08:34:22 PST
Committed r290563 (247841@main): <https://commits.webkit.org/247841@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453252 [details].
Comment 14 Chris Dumez 2022-02-27 12:30:11 PST
(In reply to Darin Adler from comment #12)
> Comment on attachment 453252 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453252&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1839
> > +    SetForScope<bool> isStoppingLoadingDueToProcessSwap(m_isStoppingLoadingDueToProcessSwap, true);
> 
> Do we still need to write <bool>? If so, maybe we should add deduction
> guides so we don’t.

No we don't. Following up in Bug 237258.