Currently, two unit tests are relying on
ContentBrowserClient::SiteInstanceDeleting() to ensure that
SiteInstance lifetimes are managed properly. However, it is
undesirable to allow //content embedders to run arbitrary code during
SiteInstance destruction, per the linked bug. Hence, this CL converts
SiteInstanceDeleting() usage to a test-specific callback instead and
removes it from ContentBrowserClient.
Bug: 1444438
Change-Id: I2affcb4a40ccfbf3bd715d7b225976a687405616
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4564316
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149171}
This cl adds a boolean to SiteInfo to allow it to track cases where
`SiteInfo::requires_origin_keyed_process_` was set due to default
isolation, and not by an explicit opt-in (e.g. receiving the header
"Origin-Agent-Cluster: ?1"). In these cases we do not want to track
the isolation state of the origin in ChildProcessSecurityPolicyImpl
since we expect the vast majority of origins to be isolated.
The new boolean is `SiteInfo::requires_origin_keyed_process_by_default_`
and will only be set when the feature kOriginKeyedProcessesByDefault
is enabled alongside kOriginAgentClusterDefaultEnable.
Bug: 1421329
Change-Id: I554c1949c2a6e7dbcaff8adb117d2e3549850d3d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4439195
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1143406}
This patch adds simple UnguessableTokens in BrowsingInstance and
CoopRelatedGroup, with accessors in SiteInstanceImpl. These will be used
to verify cross-BrowsingInstance window accesses in the renderer.
Because the usages of the CoopRelatedGroupId can all be replaced by the
token, we update current usages with the token flow and remove
the Id altogether.
This patch makes no behavior change.
Bug: 1221127
Change-Id: I08b07ac8eca540c38f34e001260c2628832a3cf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4224333
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Arthur Hemery <ahemery@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1142728}
This CL adds a new feature, kOriginKeyedProcessesByDefault, which
when enabled together with kOriginAgentClusterDefaultEnable allows
default-isolated origins to be placed in origin-keyed processes.
After this CL (and with the above modes enabled), origins with no
explicit Origin-Agent-Cluster headers (i.e., the majority of cases) will
temporarily trigger global walks and be tracked by
ChildProcessSecurityPolicy, because such tracking currently happens for
all origin-keyed processes.
Restricting the tracking to cases with explicit headers (non-default
isolation cases) will be addressed in
https://chromium-review.googlesource.com/c/chromium/src/+/4439195.
Bug: 1421329
Change-Id: Ic7341865279182c722644872b391735abeba4125
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4429100
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1141628}
This CL renames kOriginAgentCluster, kRequiresOriginKeyedProcess and
related accessor functions on UrlInfo to clarify that they are specific
to the case where origin isolation requests derive from the explicit
presence of the Origin-Agent-Cluster header.
Bug: 1421329
Change-Id: I10ea6ec97cddd58edec3a164bb1a2890f4a332f2
Low-Coverage-Reason: refactor, no behavior change.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4518660
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1141585}
This patch implements proxy management that supports cross-
BrowsingContext group communication. Proxy creation takes into account
the properties restrictions that will be applied, and only provide the
minimal number of proxies to support the valid use cases:
- Only the opener and openee are exposed across the BrowsingContextGroup
boundary.
- Proxies may be created on demand to support postMessage's
event.source.
- A minimal chain of ancestors for subframe proxies, required by the
way Chrome implements proxies.
This prevents XSLeaks by scanning proxies existence. Details are
available here:
https://docs.google.com/document/d/1TaC1gk_54B_hpBeIopqG4wgkTk1kZOha7I_P2nMRKSs/edit?usp=sharing&resourcekey=0-TiQk3dQsHngZMIsQOueDug
Proxy creation implies modifications mostly to RenderFrameHostManager,
while deletion is a simple change in BrowsingContextGroupSwap.
The rest of this patch makes sure that we are actually considering
proxies that might now exist across BrowsingInstances. In practice,
this is all the call sites of SiteInstance::IsRelatedSiteInstance().
In particular:
- postMessage should be permitted across BrowsingInstances in the same
CoopRelatedGroup.
- Navigation should not be permitted across BrowsingInstances.
- Closing windows should not be permitted across BrowsingInstances.
- Proxy can exist across BrowsingInstances, and the sanity check
planned in BrowsingContextState now needs to be about
CoopRelatedGroup.
New browser tests are added to verify the behaviors listed above. WPTs
are updated to reflect the current state of development: openers now
provide access to cross-origin properties (apart from some that are
validated by the browser, like window.close() and navigations). They
will be restricted to only window.closed and window.postMessage() in a
follow-up patch, by renderer-side checks.
Low-Coverage-Reason: BrowsingContextState new mode is unfinished
and untestable in conjunction with COOP. A disabled test was added with
a TODO to cover this code once the feature is implemented.
Bug: 1221127, 1370346
Change-Id: I681daa22583e05cce8051349ed38658248185e0c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4200260
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Arthur Hemery <ahemery@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1136447}
There are corner cases where about:blank can't stay in the source
SiteInstance, such as when the navigation to about:blank occurs from
an error page SiteInstance or if it requires a new BrowsingInstance.
Currently, in such cases, we place about:blank in an unassigned
SiteInstance and an unlocked process. However, this is problematic if
the navigation is renderer-initiated and carries an initiator origin,
since it might result in committing about:blank in an unlocked process
but with an initiator origin for a site that requires a dedicated
process. This fails Citadel enforcements that we're hoping to enable
for desktop platforms in issue 764958.
This situation happens in a couple of existing tests:
- UnassignedSiteInstanceBrowserTest.RendererInitiatedNavigationTo uses
a renderer-initiated navigation to about:blank where the old page
may get bfcached, and with bfcache the about:blank document must
commit in a new BrowsingInstance and new SiteInstance, which is
currently unassigned, yet about:blank still carries the initiator's
origin which might require a dedicated process.
- ChromeNavigationBrowserTest.RedirectErrorPageReloadToAboutBlank uses
an extension that redirects navigations to about:blank via
webRequest, starting from an error page which makes the source
SiteInstance incompatible. This CL adds a test for a similar
scenario that also involves navigating to about:blank from an error
page, but without an extension redirect. See the linked bug for
more details.
This CL modifies UrlInfo's origin to carry the initiator origin for
renderer-initiated about:blank navigations. This origin is not going
to be used in the common case where about:blank is left in the source
SiteInstance and an existing process. But if we end up needing to
create a new SiteInstance and compute its SiteInfo and ProcessLock for
an about:blank navigation, this will ensure that we would honor the
initiator origin, if it was present. As a result, about:blank in such
a case will now load in a SiteInstance corresponding to the
initiator's site, and its process will also be locked to that site (if
the site requires a dedicated process).
Bug: 1426928
Change-Id: I19d0265a4f0e6aae7ad732fe92b28c58bcb17c1e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4362801
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124841}
This function has expanded in its use and is no longer used solely for
reusing the current SiteInstance's process. In particular, it can now
also be used for reusing other kinds of processes, including a
speculative RFH's process (for COOP) and an embedder's process (for
fenced frames). So rename the function to be more generic. This is
a followup to CL:4205941.
Bug: 1411606
Change-Id: I12e09e3e311f34155b46ecd2a68a75253c82b9a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4383216
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124278}
Now that site isolation for <webview> has launched, it is time to
remove support for the old path. This CL removes
features::kSiteIsolationForGuests and adjusts all code paths using it
to assume that site isolation for <webview> is always enabled. This
implies removing various paths for keeping guests in a single
SiteInstance, as well as removing translation between chrome-guest:
URLs and StoragePartitions. The chrome-guest: scheme itself will be
removed in a separate CL.
Bug: 1267977
Change-Id: I437d4c2b44caf98c0aa98217704ece8812e5b8b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4370981
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1123220}
Previously, ShouldAssignSiteForURL() allowed any URL to leave a
SiteInstance without a site. This is dangerous, as it is effectively
a site isolation bypass, since processes for SiteInstances without
sites can't be properly locked: see crbug.com/1324407 for an example
and more details. Presently, this capability is also getting in the
way of enforcing citadel protections in crbug.com/764958, since sites
with unassigned SiteInstances are becoming eligible for citadel
checks, which trigger site isolation violations at commit time in a
few tests using those SiteInstances.
This CL ensures that only empty document schemes can use unassigned
SiteInstances. It also removes the ShouldAssignSiteForURL override
from ContentBrowserClient. To use an unassigned SiteInstance now, the
content/ embedder can add schemes to empty_document_schemes in
ChromeContentClient::AddAdditionalSchemes().
This also requires updating a bunch of tests that previously relied on
a custom ContentBrowserClient to instead register an empty scheme for
URLs they use with unassigned SiteInstances. A couple of tests that
explicitly exercised the ability to have a non-empty URL with an
unassigned sites are removed.
Bug: 1296173
Change-Id: I9eb84a04b440e397ded2db7a50a6a8cbab3df79b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4321546
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1119421}
Previously, SiteInstanceGroup held a BrowsingInstanceId. By having
SiteInstanceGroup hold a scoped_refptr to BrowsingInstance, it ensures
the BrowsingInstance outlives all of the SiteInstanceGroups that exist
in it. This prevents lifetime issues like use-after-frees of
BrowsingInstance.
Test: SiteInstanceGroupTest.BrowsingInstanceLifetime
Bug: 1195535
Change-Id: I2d636ceaa970717439ec9949a6782ad25a630d6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4240375
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1116707}
This is the core patch of the COOP: restrict-properties
implementation. It introduces an important abstraction, the
CoopRelatedGroup. It represents a grouping of BrowsingContext groups
(BrowsingInstance) that are capable of a limited set of asynchronous
interactions, namely postMessage and closed.
It is built as closely as possible to how BrowsingInstance is built
on SiteInstance:
- It is fully private, and interacted with through SiteInstance and
BrowsingInstance. SiteInstance exposes a single new method,
GetCoopRelatedSiteInstance(), that works similarly to the
GetRelatedSiteInstance() method, but at the CoopRelatedGroup level.
- It is collectively owned by the BrowsingInstances that are part of
it. A register/unregister mechanism is used to know about the
BrowsingInstances that are still part of the group.
- It might reuse an existing BrowsingInstance when necessary, similar
to how SiteInstance's are reused within a BrowsingInstance.
To support BrowsingInstance reuse, some information needs to be
available to the CoopRelatedGroup when doing the selection. In
particular it needs to know whether the BrowsingInstance
hosts COOP: restrict-properties pages, and from which origin. We
record that information from the NavigationRequest into UrlInfo.
The SiteInstance picking algorithm is updated to output a SiteInstance
created via GetCoopRelatedSiteInstance() when a
BrowsingContextGroupSwap::kCoopRelatedSwap is requested.
Added browser tests and unit tests cover the newly introduced code.
Bug: 1221127, 1385827
Change-Id: Ief8436b5d532b6aadc5d9a29d94346e3fae2641c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3948564
Commit-Queue: Arthur Hemery <ahemery@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1112861}
This patch is the second implementation step for COOP:
restrict-properties. It passes the value produced by CoopStatus into
the SiteInstance picking process to decide what
BrowsingContextGroupSwap to produce.
Before this patch, CoopStatus knows about the different type of swaps, but does not pass the information into the SiteInstance picking process.
A related swap is considered as a no swap.
After this patch, the SiteInstance picking process gets the
information and produces correct internal representation. It enforces
it in the same way as other COOP swaps, however. Implementing the COOP
group to produce visible differences between COOP swaps is the next
step.
Bug: 1221127
Change-Id: If74c44a73c0f1dd7bce2f8125556c636c08feb81
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3945190
Commit-Queue: Arthur Hemery <ahemery@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1099288}
This removes 22 static_cast<SiteInstanceImpl*> instances and makes
RenderFrameHostManager consistent about using SiteInstanceImpl.
This also cleans up several related files that required similar
changes in order to compile.
No behavior change is expected.
Bug: 1394513
Change-Id: Ib10d9fec454fdfbd77ce41a8e90bafa2173e9df5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4129800
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1090871}
This CL moves (temporarily) the kIsolateSandboxedIframes feature into
Blink. This is necessary to facilitate linking its use to a new feature,
tentatively named kNewBaseUrlInheritanceBehavior, to be introduced to
Blink in
https://chromium-review.googlesource.com/c/chromium/src/+/3938111.
We want to ensure that use of kIsolateSandboxedIframes will also force
enabling of kNewBaseUrlInheritanceBehavior, but at present there is no
mechanism in base::Feature to enforce this type of linking between
features. Instead, the follow-on CL will introduce a helper function
that looks at the value of both flags whenever a decision based on
kNewBaseUrlInheritanceBehavior needs to be made.
It is hoped that kNewBaseUrlInheritanceBehavior will be relatively
short-lived, and when it is removed this CL will be reverted in order
to move kIsolateSandboxedIframes back to content/.
Bug: 1356658
Change-Id: Icb063e65eb9cdec56b486a7a679117f4a1fcbcb3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3956476
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1059406}
This CL focuses on the process model for fenced frames that are
embedded inside Guests. We currently have site isolation for Guests and
process isolation for Fenced Frames behind flags, so we have 4
different states to consider. This CL addresses all 4 states, and tries
to make behaviour consistent. (SIG = SiteIsolationForGuests, FFPI =
FencedFrameProcessIsolation)
1) No SIG, No FFPI: FFs inside guests get the same SiteInfo as their
embedder (which was created using SiteInfo::CreateForGuest), and are
put into the same process. The FF will keep the same
SiteInstance throughout its lifetime.
2) No SIG, FFPI: With SIG, we only have one SiteInstance for a guest
BrowsingInstance, and it is never changed. We rely on the initial
SiteInstance being swapped for a new one to implement FF process
isolation, and a result, we cannot implement FFPI when Site Isolation
for Guests isn't enabled, and behaviour will be exactly the same as
the scenario above.
3) SIG, No FFPI: FFs will initially inherit the same SiteInfo as their
embedder and be placed in the same process initially. On navigation,
the FF may get a new SiteInstance (for cross-site navigations), but
is_fenced is never set, so same-site FFs will be placed in their
embedder's process. If the embedder has a default SiteInstance
initially, we cannot make the FF use the same SiteInfo. We special
case this scenario, and create a new SiteInfo using CreateForGuest.
4) SIG, FFPI: Similar to the above scenario initially, except now
we set is_fenced in the IsolationContext. Initially the fenced frame
will share a process with its embedder, but after the first navigation
it will be placed in a different process. We don't have to deal with
the scenario where the embedder has a default SiteInstance since
process isolation for fenced frames is currently only supported when
strict SiteIsolation is enabled.
Bug: 1340662
Change-Id: I880e0d62fcca5d523a8f199ba99ec05cf914a612
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3862308
Reviewed-by: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1048147}
In per-origin mode for process-isolated sandboxed iframes, at present
a sandboxed srcdoc iframe will attempt to clone its parent's
SiteInstance, with the only difference being that the sandbox flag is
set. But the parent's SiteInstance will be using a site-based site-url,
and this will cause the child to attempt to load itself in a process
with a site-based, and not origin-based, site_url.
This CL revises the cloning process to take into account the parent's
origin (which the srcdoc frame will use), and creating a consistent
SiteInstance by recreating the SiteInfo via SiteInfo::Create. This will
use the same pathways for creation of the SiteInfo as if it had been
created for a navigation.
Bug: 1345491
Change-Id: I2870d01f3e916f28784b3d5adb749d9c58edacc2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3854030
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1047598}
With kIsolateFencedFrames enabled, ServiceWorkers created by a fenced
frame are not allowed to be put in the same process as the fenced
frame due to is_fenced not being set for their SiteInstances. This
CL sets is_fenced based on the ancestor type of the ServiceWorker.
Bug: 1340662
Change-Id: I8a6309d3d62594933a5dac928dc98c934aee8f07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3854989
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1044033}
This CL implements process isolation for fenced frames (isolating
fenced frames from their embedders) behind a flag. It adds an
'is_fenced' attribute to SiteInfo and IsolationContext to help enforce
this isolation. Having the is_fenced bit in IsolationContext is used to
maintain the same value of is_fenced for all related SiteInstances
created in a particular BrowsingInstance (e.g., for subframes of a
fenced frame).
The changes here currently only have an effect when
the flag is enabled and strict site isolation is enabled. Changes for
other site isolation modes will be in future CLs. See attached bug for
more details on design (design doc, explainer).
Bug: 1340662
Change-Id: I821943158286c9fa69639ad1156112bd74a4410e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3821704
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1041755}
In crrev.com/c/3645368, we made the initial fenced frame SiteInstance
reuse its embedder's process - and then relied on
REUSE_PENDING_OR_COMMITTED_SITE to force process reuse for future
navigations in the frame. However, this doesn't work when strict site
isolation isn't enabled. Setting the initial fenced frame SiteInstance
would set the process correctly for the default SiteInstance
initially, but if we navigated the frame to a site that required a
dedicated process, and then back to a site that didn't, we would
create a new default SiteInstance and allocate it a brand new
process instead of reusing the embedder's process.
This change adds logic in RFHM::GetSiteInstanceForNavigation that
tries to make BrowsingInstances of a non-fenced FrameTree and any
embedded fenced FrameTrees share the same default process. It does so
by searching through these frame trees and proactively reusing their
default process when returning a SiteInstance in a BrowsingInstance
that doesn't have a default process yet.
Change-Id: Icbb4d29f581f56bc0bc0d495e134a5e4bd42092f
Bug: 1326594
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3749074
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1029354}
This CL adds a "per-document" isolation grouping for isolation of
sandboxed iframes using the kIsolatedSandboxedIframes flag. This CL
parameterizes the flag to add "per-document", and when this new
parameter is present it creates a unique document identifier on every
SiteInfo associated with a sandboxed iframe in order to force each
document into its own process. This grouping is expected to be the most
granular option for isolated sandboxed iframes.
Bug: 510122
Change-Id: I217bf5c57f8e9badaa84c0fab9261c432e766fcb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3727308
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1029273}
With the advent of OriginAgentCluster(OAC)-by-default, the number of
navigations getting OAC isolation is expected to increase dramatically. Prior to this CL, each time a navigation got OAC (either via header or
OAC-by-default) we had to check if the navigation's origin had ever
been isolated before, and if not do a potentially expensive search of
the frame tree and session history to verify that we hadn't previously
encountered the origin in the current BrowsingInstance.
To avoid OAC-by-default causing a performance regression due to many
additional global walks, this CL refactors the global walk logic so
that it is only invoked when the OAC header is explicitly present,
either for opt-in or opt-out, and not invoked when an origin gets
OAC-by-default. Since the number of sites explicitly opting-out is expected to be small, this should help keep the total number of global walks small.
When OAC-by-default is enabled, we will explicitly track all origins
that have sent the OAC header, along with the requested opt-in or
opt-out state. All untracked origins will be assumed to have the
default OAC-enabled state.
When OAC-by-default is not enabled, we continue to track only origins
that have explicitly requested opt-in via the header.
This CL also renames a number of functions used in the global walk
to make their behavior more easily understood.
More details about the global walk and this refactor/redesign can
be found at
https://docs.google.com/document/d/1zrMXDOXDhp4-qFZvkZkDVSuqsT718Y-kRVRPqestiMY/edit?usp=sharing
Bug: 1259920
Change-Id: I0a31641eebe187f3961ddfcb2a2d1977a7a3f1f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3763843
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1027872}
This CL adds a "per-origin" isolation mode for isolation of sandboxed
iframes using the kIsolatedSandboxedIframes flag. Prior to this CL,
all isolation of sandboxed iframes was achieved by placing all the
sandboxed iframes for a given site into the same SiteInstance/process.
This CL parameterizes the flag to add "per-site" and "per-origin"
options, where the latter causes sandboxed iframes to be isolated into
processes according to their URL's origin instead of its site. This gives a more granular mechanism for isolating the sandboxed
iframes.
Enabling per-origin mode is accomplished by re-using the strict-origin-isolation pathway.
Bug: 510122
Change-Id: I3f02c8eaa8edb37ed341cd64302fc7a4e68d2ebe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3703318
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1017666}
This CL fixes an issue when the <webview> site isolation feature is
combined with a flag to turn off strict site isolation. This results
in <webview> using the default SiteInstance paths, where we currently
assume that a default SiteInstance is never created for guests,
leading to a failed CHECK. This configuration can happen if someone
turns off strict site isolation via chrome://flags or
--disable-site-isolation-trials while also enabling
features::kSiteIsolationForGuests (e.g., via field trials).
While disabling strict site isolation is not a supported
configuration, we do not want kSiteIsolationForGuests to take a
dependency on strict site isolation, since we eventually want to
remove the old chrome-guest:// path completely. So, this CL makes
this configuration work properly by teaching default SiteInstances
about whether they are created for a guest. The proper
StoragePartition information is already passed in for that case. This
means that without strict site isolation, guest navigations will stay
in a default SiteInstance that carries the proper guest bit and
StoragePartition info.
Bug: 1337478
Change-Id: I5255545a15dbb02940fb072d1d46758b69bfbbb5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715759
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1017356}
This is a reland of commit 81175d85f6
Original change's description:
> Do not allow ProcessLocks to be refined if process has been used.
>
> If ShouldAssignSiteForURL were to return false for URLs that commit
> actual content, then other sites that require dedicated processes
> might incorrectly share a process with them. Instead, prevent an
> allows_any_site() ProcessLock from being locked to a single site if
> that process has been used.
>
> Some of the test changes are adapted from ahemery's
> https://chromium-review.googlesource.com/c/chromium/src/+/3483667.
>
> Bug: 1324407
> Change-Id: I778d31c5e8b740179f0b6c9a40b010508ce7cfa2
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3640283
> Commit-Queue: Charlie Reis <creis@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1004469}
Bug: 1324407
Change-Id: I9db2f24f66a112005664164ff19f33f1ebc098b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3654240
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004979}
If ShouldAssignSiteForURL were to return false for URLs that commit
actual content, then other sites that require dedicated processes
might incorrectly share a process with them. Instead, prevent an
allows_any_site() ProcessLock from being locked to a single site if
that process has been used.
Some of the test changes are adapted from ahemery's
https://chromium-review.googlesource.com/c/chromium/src/+/3483667.
Bug: 1324407
Change-Id: I778d31c5e8b740179f0b6c9a40b010508ce7cfa2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3640283
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004469}
Before, a SiteInstanceGroup had a RenderProcessHost
(and AgentSchedulingGroup) that could go away and be set again. This
change makes it so that the SiteInstanceGroup goes away when its
RenderProcessHost gets destroyed. This only happens after all
RenderFrameHosts, RenderFrameProxyHosts and workers in a
RenderProcessHost go away. This usually only happens when a SiteInstance
is solely kept alive in session history (i.e. NavigationEntry).
SiteInstances belonging to this group will get a new SiteInstanceGroup
with a new RenderProcessHost on the next call to
SiteInstanceImpl::GetProcess.
Bug: 1294045
Change-Id: Ic372fe33d3b968a9ee9919d1c03e207b5df6db85
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3440449
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002957}
With MPArch, we can have nested frame trees and so FTN::IsMainFrame
cannot be used to determine if the frame is outermost. This CL changes
a number of simpler cases where semantics match outermost. This should
have no effect outside the use of MPArch-based nested frame trees (only
fenced frames at the moment).
A number of usages do not have tests included in this CL, but I have
added TODOs for these, similar to the approach taken in
crrev.com/c/2988476.
This is a follow on to crrev.com/c/3577677
This CL also removes a number of expired histograms:
- Navigation.CommitTimeout.ErrorCode
- Navigation.CommitTimeout.IsMainFrame
- Navigation.CommitTimeout.IsRendererProcessReady
Bug: 1314749,1053061
Change-Id: I09507b1dce358507999e706b9ea81b087b16fc9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3584183
Reviewed-by: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Ian Vollick <vollick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#993707}
The OriginAgentCluster (OAC) metrics for process overhead,
Memory.RenderProcessHost.Count.OriginAgentClusterOverhead and
Memory.RenderProcessHost.Percent.OriginAgentClusterOverhead
do not allow us to distinguish between the cases where a user just
hasn't encountered any origins using OAC, and those where an origin
is process isolated, but doesn't incur any process overhead.
This CL adds logic to activate a synthetic trial group the first time
a browsing session process-isolates an origin. This will allow us to
distinguish between the two no-overhead cases.
Bug: 1171269
Change-Id: I4cd7e6d83106ed7d821079c6a22af250688d0bee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3542930
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#985514}
Use the new ability to write typed messages into the untyped and untyped
into typed to eliminate WriteIntoTrace(TracedValue) /
WriteIntoTrace(TracedProto) duplicated methods in //content.
After this patch, all key //content types listed below now have
corresponding proto messages (with DebugAnnotation support) and
WriteIntoTrace(TracedValue) methods have been removed:
- BrowserContext
- BrowsingContextState
- FrameTreeNode
- NavigationRequest
- RenderFrameHost
- RenderFrameProxyHost
- RenderViewHost
- SiteInstance
- SiteInstanceGroup
- GlobalRenderFrameHostId
This patch also adds a few `const` qualifiers to the appropriate methods
in //content to ensure that all WriteIntoTrace methods can be marked as
const.
R=rakina@chromium.org
Bug: 1137154
Change-Id: I306e7619f594e2a02ca24183720714f234661ff4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3540169
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Commit-Queue: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984436}
This CL implements process isolation for origin-restricted sandboxed
iframes, i.e. those with the 'sandbox' attribute but no 'allow-origin'
modifier.
At present this CL implements a model where all the sandboxed iframes
for a site are placed in the same process, though this could be modified
to be (i) origin-specific, and/or (ii) place each sandbox into its own
process regardless of site-/origin-keying of the sandboxes.
Bug: 510122
Change-Id: If02a02d05a02552ef4b1a0a01145e7c26d9dc98e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3416475
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#969578}
This change moves functionality on SiteInstanceImpl to
SiteInstanceGroup, as it will be group-based. This includes the Observer
interface, and the AgentSchedulingGroupHost and RenderProcessHost
members.
A SiteInstanceGroup always starts with a RPH and ASGH, but it can lose
them and later gain new ones. A SiteInstance starts without a
SiteInstanceGroup, but once it gets one it never changes. A SiteInstance
relies on SiteInstanceGroup for its RPH and ASGH. In a followup, we
intend to allow SiteInstances to change their SiteInstanceGroup after
the RPH goes away. See https://crbug.com/1294045.
When RFH is created, we can assume SiteInstance exists, and has a
SiteInstanceGroup, a RPH and an ASGH. Similarly, when RFPH is created,
SiteInstanceGroup, RPH, and ASGH all exist.
Test: No behaviour change
Bug: 1261963
Change-Id: I9dee2331c359c833b3df1748cf9d9a21781a0b59
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3286326
Reviewed-by: Charles Reis <creis@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#969118}
SiteInstances still support an initial state with no SiteInfo, and will
likely continue to do so. This allows creating renderer processes and
blank RenderFrames that are ready to go for a future navigation in most
cases (in scenarios like the Android NTP). The alternative would be
delaying having a SiteInstance until the SiteInfo is known, but that
would be more problematic for RenderFrameHost and for process creation.
We now have sufficient safeguards to prevent using an unsuitable
process, and the need to discard this blank process should be rare.
Bug: 1015882
Change-Id: I56f2c881fcd4f6ef1e02e7c176488b028cc4c7bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3433275
Reviewed-by: Charles Reis <creis@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#966963}
This CL adds initial support for enabling site isolation inside
<webview> guests. This mode is behind a new feature called
SiteIsolationForGuests.
The overall approach is to stop using special chrome-guest: site URLs
when creating SiteInfos for guests. Instead, site-isolated guests use
real site URLs that are computed with the existing process model code,
and rely on SiteInfo::storage_partition_config to keep navigations
within guests in the same StoragePartition and in guest SiteInstances.
This CL supports cross-process navigations in guests, OOPIFs, and
cross-BrowsingInstance navigations in guests. A particular guest will
no longer be restricted to a single SiteInstance and BrowsingInstance,
though it will always stay in the same StoragePartition, and for all
SiteInstances in a guest, SiteInstance::IsGuest() will be true.
In the current SiteInstance assignment paths, there are various early
returns that make guests always in the same SiteInstance. Those are
now only used when the new feature is off. Since guests will now
descend much deeper into the SiteInstance assignment paths, we needed
an additional signal as to whether the new SiteInstance/SiteInfo would
be for a guest. To do this, instead of plumbing an additional flag to
places like SiteInfo::CreateInternal, is_guest is added to
IsolationContext which is already plumbed into all the needed code
paths (see SiteInfo::CreateInternal for how it's used). Conceptually,
IsolationContext::is_guest just says whether a particular
BrowsingInstance is for a guest or not, so that any related
SiteInstances within it stay in the guest.
To handle cross-BrowsingInstance navigations in guests,
SiteInstanceImpl::CreateForUrlInfo (which is normally used to create
a new SiteInstance in a new BrowsingInstance) is updated to be able to
handle guests, in which case the StoragePartition info needs to be
transferred into the new SiteInstance/BrowsingInstance.
The following are some things that are not part of this CL and will be
addressed in future CLs:
- converting <webview> tests to run in both the legacy and site
isolation modes. (This CL does add standalone tests to validate
site isolation properties.)
- embedder navigating <webview> to about:blank and siteless
SiteInstances.
- error page navigations, WebViewRendererState, loadDataWithBaseURL.
Bug: 1267977
Change-Id: I3b747640c083a302dc07ee4106af4f6d33928165
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3380343
Reviewed-by: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charles Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#965525}
(original CL by acolwell@, rebased and submitted by alexmos@)
This is the final patch in the series to migrate the
NavigationControllerImpl::session_storage_namespace_map_ to use a
StoragePartitionConfig instead of a partition ID string. It removes the
StoragePartitionId class and updates all the existing usage to
StoragePartitionConfig. This should not introduce user visible behavior
changes because the current StoragePartitionId implementation is
essentially a wrapper around a StoragePartitionConfig. That change was
made in https://crrev.com/c/2808670, which has reached stable in M93
and raised no issues.
- Removes the StoragePartitionId object and replace usage with
StoragePartitionConfig.
- Removed NavigationControllerImpl::partition_config_to_id_map_ since
it is no longer needed.
- Removed SiteInfo usage from RenderViewHostImpl now that we only
need a StoragePartitionConfig to get the SessionStorageNamespace.
- Added GetStoragePartitionConfig() method to SiteInstance to make
it easier to get the StoragePartitionConfig for a SiteInstance.
This will also make it easier to remove GetSiteURL() usage related
to storage partitions in followup CLs.
- Removed SiteInstanceImpl::GetSiteInfoForRenderViewHost() and
move code that enabled storage partition verification to the new
GetStoragePartitionConfig() method.
Bug: 1166021
Change-Id: Iefc00108e2d1274dc69bd31a7e4d8f667ffea960
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2811120
Reviewed-by: Charles Reis <creis@chromium.org>
Reviewed-by: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#965388}
UrlInfos are converted to SiteInfos to create and compare security
principals. In certain cases it is even converted to a ProcessLock such
as in RenderProcessHostImpl::IsSuitableHost(). When a navigation is
at the speculative stage (and some other niche cases) we won't have a
WebExposedIsolationInfo yet, because we haven't received security
headers or are outside of the standard security model (about:blank for
example). UrlInfo carries an optional WebExposedIsolationInfo but
SiteInfo represents a real object and must have a non optional value.
What do we put in the SiteInfos when converting if it's optional?
That's what this patch is trying to address, by doing the following:
- UrlInfo() and UrlInfoInit() are created with a nullopt value instead
of a CreateNonIsolated() value. If people converting use explicit
UrlInfo constructor, generally to make clear what attributes they're
trying to compare, we will get nullopt in places where we do the
conversions.
- When we're converting using half a UrlInfo, half a
SiteInstance/BrowsingInstance, for example in DeriveSiteInfo(), we
DCHECK that the WebExposedIsolationInfos are compatible. This way we
avoid complicated semantics and possible interpretations.
- We override any optional value with a value that makes sense (usually
the WebExposedIsolationInfo of the class we're half-using.)
- We add one single conversion point from optional to
CreateNonIsolated() at the heart of the SiteInfo constructor, to
accommodate for the cases where a new SiteInstance/BrowsingInstance
must be created at a speculative stage for example.
Bug: 1243449
Change-Id: Ibd7a815c162a789403b1ac9dbd2efdc6f7475315
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3377183
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Arthur Hemery <ahemery@chromium.org>
Cr-Commit-Position: refs/heads/main@{#958524}
When we take decisions in the process model about about isolation, we're
not only considering the raw values, but things like, "Are we doing a
speculative computation?", "Is this a navigation to about:blank?", etc.
These are grouped in the IsSiteInstanceCompatibleWithWebExposedIsolation
function. When we reach the lower levels of SiteInstances,
BrowsingInstances, etc, we don't have these information anymore, and
that leads us to doing weird WebExposedIsolationInfo (WEII) overrides.
See code with reference to https://crbug.com/1243449 for some examples.
Imagine we go from a COI page to another COI page. When we do the
speculative computation for a RenderFrameHost, since it's only
speculative, we've not yet received the headers that can determine
a final isolation state. It can live in any SiteInstance regarding
isolation. Later, down the line we check that we only put COI with COI,
and CHECK because we don't have the initial information anymore.
The alternative we propose is to make WEII an optional value. More
precisely, three different places hold WEII: UrlInfo, SiteInfo and
BrowsingInstance. We propose to make UrlInfo's WEII optional. The WEII
for a navigation is computed in the NavigationRequest and set in an
UrlInfo. If UrlInfo has an optional WEII, we can convey the information
that this is one of the special cases above, and that we shouldn't pay
attention to it.
The goal of this patch is to introduce the optional member itself, and
as well as the first use for it, a replacement for "is_speculative".
First we verified in this document that we have to keep this mechanism:
https://docs.google.com/document/d/135l25QN2MVjUAJCrvha2UQKqAq7QFp1sRRtDguHtEmA/edit#heading=h.ybelxngq8lbn
The current way of doing that is to infer a "is_speculative" boolean
based on the NavigationRequest state and carry it over in the
RenderFrameHostManager call sites. Instead, we use a nullopt value.
In terms of implementation:
- We introduce a set of static AreCompatible() funcctions that operate
between optional WEII and WEII, that returns true if comparing any WEII
with an empty WEII optional. The underlying meaning is that WEII should
not be a criteria for rejecting a SiteInstance if WEII is nullopt.
- In places that take an UrlInfo WEII and convert it to a SiteInfo /
BrowsingInstance WEII, we convert an empty WEII into a non isolated one.
While this is still technically incorrect, this replaces the previous
behavior that depended on the current frame isolation state. This has
the benefit of being consistent, but does not lead yet to the removal of
overrides mentioned above. This will come in following patches.
- We convert state >= REDIRECT to state >= RESPONSE. We do not need to
enforce WEII at redirect time, there was some confusion with COOP that
is applied even on redirects.
Bug: 1266819
Change-Id: I0176e2283f02929122c2da199737bccf2aea4c09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3306588
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Arthur Hemery <ahemery@chromium.org>
Cr-Commit-Position: refs/heads/main@{#953493}
Currently, calling GetRelatedSiteInstance() on a guest SiteInstance
results in creating a related SiteInstance that's not actually a guest
(its SiteInfo::is_guest() is false, and StoragePartition information
may be wrong). In practice, with the current implementation, content
within a guest should always stay in a single SiteInstance and
BrowsingInstance, and even different guests created in the same app
and partition need to stay in that SiteInstance [1]. We usually avoid
calling GetRelatedSiteInstance() for guests, but while working on
crbug.com/1267977 and trying to enforce that a BrowsingInstance can't
mix guest and non-guest SiteInstances, I discovered that one way this
can happen is through NavigationRequest's
SetSourceSiteInstanceToInitiatorIfNeeded(), which calls
GetRelatedSiteInstance() [2]. So far, I saw this happen in a narrow
case where a guest opens a new about:blank window with the opener
suppressed (i.e., with noopener/noreferrer), which satisfies
RequiresInitiatorBasedSourceSiteInstance() requirements, and, unlike
typical noopener cases, also (currently) stays in the same
BrowsingInstance due to lack of site isolation in guests (see above)
[3].
To fix this, this CL tweaks GetRelatedSiteInstance() to return back
the same SiteInstance when called from a guest SiteInstance. This
obeys the current expected semantics and guards against other
potential calls to GetRelatedSiteInstance() from guest SiteInstances.
It also modifies the test which exercises this scenario to verify that
the source SiteInstance is a guest SiteInstance in this case.
Once site isolation for guests is implemented, this will be changed to
actually return a related SiteInstance that's still marked as a guest
and preserves the same StoragePartition. I also expect that with site
isolation for guests, we will be able to actually swap
BrowsingInstances when opening new noopener windows from guests (while
again staying in the same StoragePartition and a guest SiteInstance).
[1] https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/guest_view/web_view/web_view_guest.cc;l=350-352;drc=3906812a532650fb5f8185caf372f5c5f5c319d0
[2] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;l=6656;drc=0c486499f16137c84cec56d8e711991875d81230
[3] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=3830-3837;drc=0c486499f16137c84cec56d8e711991875d81230
Bug: 1267977
Change-Id: I469fef493154df15577624c3dc9f246ed4ae0001
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3339776
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#952181}
As a followup to introducing RenderProcessHost::GetProcessLock, update
call sites that use ChildProcessSecurityPolicy.
There are also some sites that update SiteInstanceImpl::GetProcessLock
to RenderProcessHost::GetProcessLock, where it is more accurate to get
the ProcessLock for the process.
Bug: 1261963
Test: Updated browsertests
Change-Id: I27823e5c584cde9dcd03e661d4f6f272d483cbd0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3307505
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#946768}
ProcessLocks are used to represent what level of isolation a given
SiteInfo principal requires (e.g., whether it needs a dedicated process
for its site or origin), as well as what sites or origins are allowed
into a given process.
Both SiteInstance and SiteInstanceGroup simply use their SiteInfo to
represent the required lock, and do not need dedicated APIs for it.
Instead, callers should use ProcessLock::FromSiteInfo to do ProcessLock
comparisons. A ProcessLock constructor that takes a SiteInfo exists,
but has been made private in favour of using the static function.
Internal doc with more background
https://docs.google.com/document/d/19H19czBu8_FjOT8Cy8KdEVXxN-gZ93uNVM5rx1Qu9kw/edit?usp=sharing&resourcekey=0-bf4Im7qJWJDYnImt6OsAzw .
Test: CQ (no behaviour change)
Bug: 1261963
Change-Id: Idd0aefea850a795450b62bfc5dcff0b770739e5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3284127
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#946273}
As part of transitioning to SiteInstanceGroup, AgentSchedulingGroupHost
will check that SiteInstanceGroups (rather than SiteInstances) can
request an AgentSchedulingGroup only once from the same
RenderProcessHost.
Test: CQ (no behaviour change)
Bug: 1261963
Change-Id: I5d4c5bb3e9040701ba116517aaefaf983ba52108
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3292287
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/main@{#945139}
Currently, <webview> guests use specially constructed chrome-guest:
site URLs that reflect a guest's StoragePartition configuration. The
translation between StoragePartitionConfigs and guest site URLs
currently happens in the //extensions layer, inside WebViewGuest.
As part of preparing to support site isolation in <webview>, this CL
moves the specifics of guest site URLs into //content and hides guest
site URL translations in SiteInfo. //content already knows about
guests and their special behavior (content::kGuestScheme is considered
in process model decisions, SiteInstance already exposes IsGuest(),
etc), so this wouldn't be any worse layering-wise. The goal is to
force all code that wants to interact with guests to do so via
SiteInstance::IsGuest() and directly manipulating guest
StoragePartitionConfigs (e.g., via
SiteInstance::GetStoragePartitionConfig()), rather than via
chrome-guest: site URLs; details about how a guest's
StoragePartitionConfig is encoded in a site URL should be completely
opaque to outside code. This will make it much easier to eventually
convert guests to use real site URLs, which will be necessary for site
isolation.
More specifically, this CL makes the following changes:
- Move GetSiteForGuestPartitionConfig and
GetGuestPartitionConfigForSite from WebViewGuest into SiteInfo, and
updates site <-> StoragePartitionConfig translations to happen
within //content rather than outside it. Note that WebViewGuest is
still responsible for defining what a guest's StoragePartitionConfig
is (i.e., assigning partition_domain to the embedder app's extension
ID, and computing partition_name and in_memory based on <webview>'s
partition attribute). This still happens in
WebViewGuest::CreateWebContents(), and //content doesn't need to
know about these semantics.
- Updates SiteInstance::CreateForGuest to take a
StoragePartitionConfig instead of a guest site URL, and lets
SiteInfo::CreateForGuest generate the guest site URL internally.
- Updates ExtensionNavigationThrottle and extensions ProcessManager to
directly use StoragePartitionConfigs instead of guest site URLs.
- The CreateForGuest() change required several updates to
ServiceWorker and shared worker tests to be able to pass in an
appropriate StoragePartition when workers are created in guests. In
particular, StoragePartitions for guests no longer track full guest
site URLs and instead just store a bit that indicates that they're
for guests. These tests are also updated to take advantage of
MockRenderProcessHost's support for StoragePartitions, which was
recently added in https://crrev.com/c/3049856.
This CL is based on acolwell@'s draft CL at
https://chromium-review.googlesource.com/c/chromium/src/+/2750284.
This is intended as a pure refactor, with no expected behavioral
changes.
Bug: 1267977, 1030932
Change-Id: Ib1b8dcd7765f09cc9bc196640bd60a3fbe8563a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3283604
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Reviewed-by: W. James MacLean <wjmaclean@chromium.org>
Commit-Queue: W. James MacLean <wjmaclean@chromium.org>
Auto-Submit: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#943621}
