Bug 238036 - Website policies are not respected when doing COOP based process swap
Summary: Website policies are not respected when doing COOP based process swap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Process Model (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-17 12:24 PDT by youenn fablet
Modified: 2022-03-22 01:02 PDT (History)
3 users (show)

See Also:


Attachments
Patch (9.24 KB, patch)
2022-03-17 12:28 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.48 KB, patch)
2022-03-18 00:07 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (16.70 KB, patch)
2022-03-21 03:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (27.98 KB, patch)
2022-03-21 09:43 PDT, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2022-03-17 12:24:38 PDT
Website policies are not respected when doing COOP based process swap
Comment 1 youenn fablet 2022-03-17 12:24:55 PDT
<rdar://89616625>
Comment 2 youenn fablet 2022-03-17 12:28:59 PDT
Created attachment 455004 [details]
Patch
Comment 3 youenn fablet 2022-03-18 00:07:53 PDT
Created attachment 455071 [details]
Patch
Comment 4 youenn fablet 2022-03-18 00:09:45 PDT
Comment on attachment 455071 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:3478
> +    m_lastWebsitePolicies = policies;

I am not clearing m_lastWebsitePolicies proactively in didFinish/didFail or earlier as I am not sure this is actually worth it.
Comment 5 Chris Dumez 2022-03-18 07:25:29 PDT
Comment on attachment 455071 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3478
>> +    m_lastWebsitePolicies = policies;
> 
> I am not clearing m_lastWebsitePolicies proactively in didFinish/didFail or earlier as I am not sure this is actually worth it.

I think it would look better if we stored the WebsitePolicies on the APINavigation object. This is the object associated with the navigation from the decidePolicyForNavigationAction to the COOP process-swap later on.

It would also greatly reduce the risk of using websitesPolicies from an unrelated navigation.
Comment 6 youenn fablet 2022-03-21 03:57:20 PDT
Created attachment 455228 [details]
Patch
Comment 7 Chris Dumez 2022-03-21 07:23:29 PDT
Comment on attachment 455228 [details]
Patch

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

> Source/WebKit/UIProcess/API/APINavigation.h:175
> +    void setWebsitePoliciesForProcessSwap(RefPtr<API::WebsitePolicies>&& policies) { m_websitePoliciesForProcessSwap = WTFMove(policies); }

I am not sure the "ForProcessSwap" part of the name is super helpful.

> Source/WebKit/UIProcess/API/APINavigation.h:176
> +    RefPtr<API::WebsitePolicies> takeWebsitePoliciesForProcessSwap() { return std::exchange(m_websitePoliciesForProcessSwap, { }); }

Why the need to "take" them? Policies are associated with the navigation (and passed during policy decision). I don't think they really need to be consumed?
Navigation objects are not reused across different navigations after all.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3516
> +            receivedPolicyDecision(policyAction, navigation.ptr(), nullptr, WTFMove(navigationAction), WTFMove(sender), WillContinueLoadInNewProcess::Yes);

It is not clear to me why this new factory with 2 separate calls to receivedPolicyDecision() and 2 separate calls to setWebsitePoliciesForProcessSwap() is better.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3640
> +    auto websitePolicies = navigation.takeWebsitePoliciesForProcessSwap();

This doesn't seem right. I think this means that if we first do a PSON swap, and then do a COOP swap on response, then the COOP swap still won't have website policies.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:8312
> +TEST(ProcessSwap, ContentModeInCaseOfCoopProcessSwap)

May be nice to add a test case where we PSON swap before with COOP swap given the bug in the patch.
Comment 8 youenn fablet 2022-03-21 08:05:28 PDT
> > Source/WebKit/UIProcess/WebPageProxy.cpp:3516
> > +            receivedPolicyDecision(policyAction, navigation.ptr(), nullptr, WTFMove(navigationAction), WTFMove(sender), WillContinueLoadInNewProcess::Yes);
> 
> It is not clear to me why this new factory with 2 separate calls to
> receivedPolicyDecision() and 2 separate calls to
> setWebsitePoliciesForProcessSwap() is better.

It seems much more readable to me.
The call to receivedPolicyDecision was not clear to me given it had 3 parameters that were fully different based on shouldProcessSwap.
Now we have a branch for PSON, and another for not PSON.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:3640
> > +    auto websitePolicies = navigation.takeWebsitePoliciesForProcessSwap();
> 
> This doesn't seem right. I think this means that if we first do a PSON swap,
> and then do a COOP swap on response, then the COOP swap still won't have
> website policies.

AIUI, this cannot happen given PSON will give you a well deserved isolated process anyway that should be good enough for all COOP values.
Maybe I misread COOP though. Would be good to add a test if that can indeed happen in practice.
Comment 9 youenn fablet 2022-03-21 08:07:53 PDT
> AIUI, this cannot happen given PSON will give you a well deserved isolated
> process anyway that should be good enough for all COOP values.
> Maybe I misread COOP though. Would be good to add a test if that can indeed
> happen in practice.

Looking at all COOP policy values, I see how this could be theoretically possible.
That said, I would hope that if we are doing PSON, we never end up doing COOP-based process swap, that would seem wasteful to do so.

I'll update the patch to make it work no matter what.
Comment 10 Chris Dumez 2022-03-21 08:09:35 PDT
(In reply to youenn fablet from comment #9)
> > AIUI, this cannot happen given PSON will give you a well deserved isolated
> > process anyway that should be good enough for all COOP values.
> > Maybe I misread COOP though. Would be good to add a test if that can indeed
> > happen in practice.
> 
> Looking at all COOP policy values, I see how this could be theoretically
> possible.
> That said, I would hope that if we are doing PSON, we never end up doing
> COOP-based process swap, that would seem wasteful to do so.
> 
> I'll update the patch to make it work no matter what.

Oh, we can definitely PSON swap and then COOP swap. A PSON swap does not give you a truly isolated process in which you can use SharedArrayBuffer for e.g.
Comment 11 youenn fablet 2022-03-21 09:43:15 PDT
Created attachment 455253 [details]
Patch
Comment 12 Chris Dumez 2022-03-21 13:03:41 PDT
Comment on attachment 455253 [details]
Patch

r=me
Comment 13 EWS 2022-03-22 00:45:21 PDT
Committed r291606 (248699@main): <https://commits.webkit.org/248699@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455253 [details].