Website policies are not respected when doing COOP based process swap
<rdar://89616625>
Created attachment 455004 [details] Patch
Created attachment 455071 [details] Patch
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 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.
Created attachment 455228 [details] Patch
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.
> > 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.
> 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.
(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.
Created attachment 455253 [details] Patch
Comment on attachment 455253 [details] Patch r=me
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].