Disable origin isolation if OAC disabled via enterprise policy. [chromium/src : main]
Liam Brady (Gerrit)
Liam Brady voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alex Moshchuk
- Charlie Reis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 1 comment![]( "Open in Gerrit")
if (content::SiteIsolationPolicy::IsSitePerProcessOrStricter(nullptr)) {I'm not 100% convinced this is the right thing to do here. Presumably we would also care about enterprise policy here, but if I try to grab an actual browser context to pass in, we hit DCHECKs because the wrong thread is trying to access a RPH ([example test failure](https://ci.chromium.org/ui/p/chromium/builders/try/mac-rel/b8691313081671647089/test-results?q=ExactID%3A%3A%2F%2F%5C%3Ablink_web_tests%21webtest%3A%3Avirtual%2Fnot-site-per-process%2Fhttp%2Ftests%2Fdom%23EventListener-incumbent-global-1.html+VHash%3Ae5ceb6ad8779f6ec&clean=))
Related details
- Alex Moshchuk
- Charlie Reis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 6 comments![]( "Open in Gerrit")
if (content::SiteIsolationPolicy::IsSitePerProcessOrStricter(nullptr)) {I'm not 100% convinced this is the right thing to do here. Presumably we would also care about enterprise policy here, but if I try to grab an actual browser context to pass in, we hit DCHECKs because the wrong thread is trying to access a RPH ([example test failure](https://ci.chromium.org/ui/p/chromium/builders/try/mac-rel/b8691313081671647089/test-results?q=ExactID%3A%3A%2F%2F%5C%3Ablink_web_tests%21webtest%3A%3Avirtual%2Fnot-site-per-process%2Fhttp%2Ftests%2Fdom%23EventListener-incumbent-global-1.html+VHash%3Ae5ceb6ad8779f6ec&clean=))
Yes, indeed this can be accessed on the IO thread... which is really unfortunate. But rereading the implementation of this function, there might be an easy fix - I left a separate comment about it.
// will ensure that the correct MockContentBrowserClient is installed.nit: maybe something like "the test's actual navigation swaps to a fresh BrowsingInstance, and that the correct MockContentBrowserClient is installed before that BrowsingInstance is created and looks up OAC isolation policy."
// Attempt to script the popup page from its opener.I think it's the reverse, scripting the opener from its popup?
EvalJs(contents, "window.location.href").ExtractString();Nice, this should be much more robust in ensuring that document.domain is actually working.
AreOriginKeyedProcessesEnabledByDefault(browser_context);Hmm. I wonder if checking AreOriginKeyedProcessesEnabledByDefault is really needed here. Looking at its implementation, it will always [return false](https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/site_isolation_policy.cc;l=244;drc=72eb090be2c9a401d7f03a5b5f91c9e216e8bba3) if UseDedicatedProcessesForAllSites() is false, which makes sense - we don't want to use origin-keyed processes if site isolation is off. And since we also check this on line 114, we're basically guaranteed that this will always return false, because at this point UseDedicatedProcessesForAllSites() is false. So I wonder if this can just be removed? That would allow us to keep IsSitePerProcessOrStricter (and GetSiteIsolationDisabledReason) free from a BrowserContext dependency. (We can run this by Charlie as well, who's been involved in the other uses of IsSitePerProcessOrStricter() for metrics in browser_main_loop.)
Related details
- Charlie Reis
- Liam Brady
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |