Bug 238036 – Website policies are not respected when doing COOP based process swap

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Process Model

[youenn fablet]

Website policies are not respected when doing COOP based process swap

[youenn fablet]

<rdar://89616625>

[youenn fablet]

Created attachment 455004 [details]
Patch

[youenn fablet]

Created attachment 455071 [details]
Patch

[youenn fablet]

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.

[Chris Dumez]

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.

[youenn fablet]

Created attachment 455228 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

> > 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.

[youenn fablet]

> 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.

[Chris Dumez]

(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.

[youenn fablet]

Created attachment 455253 [details]
Patch

[Chris Dumez]

Comment on attachment 455253 [details]
Patch

r=me

[EWS]

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].