This CL adds support to log UrlInfo in trace events. This also
includes WebExposedIsolationInfo which is contained within
UrlInfo. This will be handy for identifying non-standard navigations
in tracing, such as those involving sandboxed frames, IWAs, COOP/COEP,
PDF, etc.
This is then used to add trace events for
SiteInstanceImpl::IsSuitableForUrlInfo() and
SiteInstanceImpl::CreateForUrlInfo(), both core parts of
GetFrameHostForNavigation(). A couple of existing trace events are
converted to use UrlInfo as well.
Bug: 382303496
Change-Id: I85fe16c6b3400356e2d471cc43be3f2a98bfedbb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6073022
Reviewed-by: Sharon Yang <yangsharon@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1394016}
In most cases NOTREACHED() is now a better option. Also performs
dead-code removal.
Bug: 40122554
Low-Coverage-Reason: OTHER Should-be-unreachable code
Change-Id: I3d9054619242c472feadab98d9de4024c74d4992
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6013928
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Auto-Submit: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1385678}
This CL introduces multiple SiteInstances per SiteInstanceGroup for
subframe data: URLs. This feature is behind the feature flag
kSiteInstanceGroupsForDataUrls, which is disabled by default, so there
is no behaviour change in this CL.
Subframe data: URLs will now have their own
SiteInstance that goes in the same SiteInstanceGroup as its initiator.
Currently, sandboxed data: subframes are excluded, as they require
computing a variation of the initiator SiteInstance, which is out of
scope for this CL and will be added in a followup.
Because the new data: subframe shares a SiteInstanceGroup with its
initiator, the number of processes remains the same as before.
Test: Added SiteInstanceGroup browsertests
Change-Id: If784b21ceccd440e35c0020053823ae287cf931d
Bug: 40269084
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4675093
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Elly FJ <ellyjones@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1382080}
The usage of last_succesful_url in IsNavigationSameSite should not be
necessary anymore as the SiteInstance selection logic should not need
to rely on the last_successful_url at this time. This CL removes the
conditional check and updates a test which captures the desired behavior
of a very specific navigation case.
Change-Id: I63368f91f768f62101d18244b0c806f4cdae7780
Bug: 40248630
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4073945
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1362037}
Resource thresholds may soon be added for other process reuse policies,
so this CL starts to generalize the code added for the ProcessPerSite
main frame reuse threshold.
Specifically, the policy in use for the current case is now passed into
the resource check function, and the main frame threshold is only
referenced there and not passed through other functions anymore.
SiteIsolationImpl::ProcessReusePolicy has also moved into its own file,
to make it easier to use in more places.
Low-Coverage-Reason: TRIVIAL_CHANGE Refactor with no behavior change.
Bug: 40259123
Change-Id: Ic1448b68b8e5a6a2b33081849e67ee1c83019d3f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5837830
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1351731}
Currently, the hosted app process model allows same-site subframes to
stay in the app's SiteInstance process, even if they are not covered
by the app's extent. This exception is implemented by skipping a call
to IsSuitableForUrlInfo(), and it inadvertently also allowed same-site
PDF documents to stay in the app's process, which is incorrect and led
to crashes, since PDF documents need to be in PDF-specific processes
with special command-line flags, etc. This CL fixes this by modifying
SiteInstanceImpl::IsNavigationSameSite() to never consider non-PDF and
PDF URLs to be same-site, similar to how this is already done for
unsandboxed vs sandboxed documents. This is a short-term fix for the
crashes to unblock the PDF OOPIF rollout; longer-term,
IsNavigationSameSite() should consider the other bits in SiteInfo as
well when determining if a navigation is same-site, and the function
should also be renamed to convey that it's performing a check on
security principals, not sites (see https://crbug.com/349777779).
Bug: 359345045
Change-Id: Id9820f7bca6a578b10cbf2150f54632b23a8146e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5839205
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1351722}
In preparation for implementing a subframe process reuse memory
threshold, this CL refactors to split REUSE_PENDING_OR_COMMITTED_SITE
into two values, one for subframes and one for service workers.
The memory threshold will only apply to process reuse by subframes, at
least initially. If/when we consider the case of process reuse for
workers, it's likely that the threshold/algorithms will be different,
so having a separate policy identifier for subframes and workers will
still be necessary.
This CL introduces no behavioral changes.
Bug: 40600119, 40259123
Change-Id: I219c7f0e5ef68e2236d95972a5b680874a7faf8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5813084
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1347440}
This change implements support for:
* DefaultJavaScriptOptimizerSetting
* JavaScriptOptimizerAllowedForSites
* JavaScriptOptimizerBlockedForSites
By:
* Adding a method to ContentBrowserClient called
AreV8OptimizationsDisabledForSite(), and implementing it in
ChromeContentBrowserClient by checking the associated content setting
* Storing whether v8 optimizations are disabled for a site in SiteInfo
(sadly by adding a new constructor argument to it)
* Storing whether v8 optimizations should be disabled as part of
RenderProcessHost
* Starting render processes with --disable-optimizing-compilers if a
given RenderProcessHost should have optimizations disabled
* Adding a browser test to ensure this logic works
Bug: 338434846
Change-Id: Idb9a85d4eb73a7cc5bfc60d303809c714959a666
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5689960
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Yann Dago <ydago@chromium.org>
Commit-Queue: Elly FJ <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1343526}
This CL implements process isolation of documents that set
Document-Isolation-Policy. It introduces an AgentClusterKey passed to SiteInfo,
which is used to isolate pages with DIP from pages without DIP. In this CL, the
AgentClusterKey is only computed for pages with DIP. Pages without DIP only get
an AgentClusterKey with an empty URL. Follow-up work will properly compute the
AgentClusterKey for all navigations.
Bug: 333047378
Change-Id: I86f1fa637f68dfe0932be7b2373323472c19ac7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5588626
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1323878}
There are comparisons that currently use the SiteInstance ID from the
content public API that should be instead using the SiteInstanceGroup
ID. Introduce and use the SiteInstanceGroup ID.
http/tests/loading/simple-subframe.html fails when comparing
SiteInstance IDs
Test: With data: URLs in SiteInstanceGroup enabled, the blink_web_test
Bug: 40269084
Change-Id: If24045558420a16a067e34b60f7a1eef0222815d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5625177
Reviewed-by: Joe Mason <joenotcharles@google.com>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1316059}
At present, all sandboxed extension frames (regardless of whether they
are listed as sandboxed in the manifest) are treated the same: when
IsolateSandboxedIframes is enabled, they are placed in a separate
process for the extension with all the other sandboxed frames for the
extension. This process is listed in the extension process map, so
any attempt to limit API access could be thwarted by a compromised
renderer.
The goals for this CL are:
1) Non-manifest sandboxed extension URLs are changed so that they stay
in the main extension process, and continue to have API access. This
reduces the process count in cases where the extra process wasn't
providing much security benefit. Continued API access for these sandboxed
frames is important, since these documents may not be able to control
their sandbox status (e.g. they might be embedded in a web page that
is itself sandboxed, in which case the extension document inherits the
sandbox, possibly unexpectedly).
2) Manifest-sandboxed extension URLs, sandboxed about:srcdoc and data
urls remain isolated. They continue to not have API privileges, but
their process is not included in the extensions ProcessMap, which
improves security and fixes the linked bug.
Summary:
This CL makes sure that, when IsolateSandboxedIframes is enabled,
sandboxed extension frames are not placed into the ProcessMap if they
are (i) manifest sandboxed, (ii) sandboxed about:srcdoc or
(iii) sandboxed data urls; but as of this CL their process is not
included in the extensions ProcessMap, which improves security and
fixes the linked bug. Any other sandboxed extension frame is
placed in the main extension process which is in the ProcessMap.
Bug: 40243274
Change-Id: Ia3d2c855f86b84d46e01a0bc7812af00b50de7bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5535370
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1315183}
The canonical bug format is TODO(crbug.com/<id>). TODOs of the
following forms will all be migrated to the new format:
- TODO(crbug.com/<old id>)
- TODO(https://crbug.com/<old id>)
- TODO(crbug/<old id>)
- TODO(crbug/monorail/<old id>)
- TODO(<old id>)
- TODO(issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/u/1/issues/<old id>)
- TODO(bugs.chromium.org/<old id>)
Bug id mapping is sourced from go/chrome-on-buganizer-prod-issues.
See go/crbug-todo-migration for details.
#crbug-todo-migration
Bug: b/321899722
Change-Id: Ibc66b8c440e4bcdef414e77fef4d9874d2ea9951
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5493800
Auto-Submit: Alison Gale <agale@chromium.org>
Commit-Queue: Alison Gale <agale@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Owners-Override: Alison Gale <agale@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293330}
In the linked bug, we saw crashes where the calculated origin for
a navigation doesn't match the SiteInstance chosen for it. After
investigation, we found it might be caused by a history navigation
that got redirected to a data: URL. In the current code, we would
try to reuse the SiteInstance saved in the FrameNavigationEntry (not
related to the data URL) but use a newly calculated opaque origin for
the data URL, causing the mismatch and crash.
This CL fixes the crash by making sure we won't use the destination
SiteInstance from the FrameNavigationEntry in those cases. Note that
this actually leads to a slightly different crashing case for the
about:blank redirect case: since we won't reuse the saved
SiteInstace, the about:blank redirect will reuse the previous page's SiteInstance (see SiteInstanceImpl::IsSameSite()). If that
SiteInstance doesn't match the final origin, we'll crash. This case
is not fixed by this CL, but since the crash will only trigger for a
smaller subset than before, it's slightly in a better state
Bug: 1454273, 1440543
Change-Id: Ibcaf9de5608600199db17c347f7a50039a853fb4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5238193
Auto-Submit: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1292003}
The canonical bug format is TODO(crbug.com/<id>). TODOs of the
following forms will all be migrated to the new format:
- TODO(crbug.com/<old id>)
- TODO(https://crbug.com/<old id>)
- TODO(crbug/<old id>)
- TODO(crbug/monorail/<old id>)
- TODO(<old id>)
- TODO(issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/u/1/issues/<old id>)
- TODO(bugs.chromium.org/<old id>)
Bug id mapping is sourced from go/chrome-on-buganizer-prod-issues.
See go/crbug-todo-migration for details.
#crbug-todo-migration
Bug: b/321899722
Change-Id: Iee14d10d544e9f0ec046117cc4ec8a55c427adc0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5469947
Reviewed-by: Darryl James <dljames@chromium.org>
Owners-Override: Alison Gale <agale@chromium.org>
Commit-Queue: Alison Gale <agale@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1290838}
All uses of ContentBrowserClient::SiteInstanceGotProcess() reference
the SiteInstance's site URL, but in certain cases the site URL may be
set *after* a SiteInstance's process is set. This happens when
creating a SiteInstance that's not for any specific URL, such as for a
blank tab; if the SiteInstance ends up being used for a subsequent
navigation, its site will be assigned at response time, but the
process will be created earlier. This behavior is undesirable since
the callers will not get another signal that both the site URL and
process are available, and the current logic in
SiteInstanceGotProcess() (such as adding extensions to ProcessMap)
won't work correctly with an empty site URL. This isn't an issue for
extensions today, since we'll end up throwing away the unassigned
SiteInstance and its process and create a new SiteInstance and process
at response time due to a mismatch in RenderProcessHostPrivilege, but
longer term we'd like to both remove those privilege buckets and allow
the unassigned SiteInstance and its initial process to be reused by
navigations to extensions, so this CL is a prerequisite for that.
This CL fixes the issue by dispatching SiteInstanceGotProcess() only
when the process and the site have both been assigned for a particular
SiteInstance, which is what its use cases really want. It also
renames the function to SiteInstanceGotProcessAndSite() to better
convey the semantics and removes an unused override in Cast code.
Bug: 1506082, 1485586
Change-Id: Id13593d4f1056d31f9a626e7ffbac30e59e45f28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5142354
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Reviewed-by: Sean Topping <seantopping@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1246267}
This new bit will make the storage partition config to preserved
across navigation (similar to the "is_guest" bit).
Also switch the isolated PWA to use this instead of guest SiteInstance.
Bug: b/304851565
Change-Id: I43755d6bce622749dbe6cca2ae5ac170e381bbba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5012192
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Jason Lin <lxj@google.com>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1227266}
When a navigation to a COOP:RP page requires a new BrowsingInstance in
a new CoopRelatedGroup (such as when navigating from a WebUI page), a
new SiteInstance in a fresh BrowsingInstance with no
common_coop_origin is created at request start time. When we receive
the response and recompute the target SiteInstance, that original
SiteInstance is passed in as the candidate_instance in
RFHM::GetFrameHostForNavigation() and ConvertToSiteInstance(). This
CL fixes a bug where candidate_instance is allowed to be reused even
if the response specifies a COOP: RP header which requires a
BrowsingInstance with a specific common_coop_origin, even though
candidate_instance's common_coop_origin is null. This later caused a
crash because the same BrowsingInstance was unexpectedly reused for
non-COOP:RP navigations, when a swap was expected.
Bug: 1491282
Change-Id: I14763513869f11ee335f19b2da2643519e7280a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4951513
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1211803}
There are two headers to declare features in content.
- the public one: `content/public/common/content_features.h`
- the private one: `content/common/features.h`.
Unfortunately, most are declared in the public one, despite being used
privately exclusively. This violate the `content/public/` rules. This
patches provides a fix.
Parts of this patch was made programmatically using this script:
https://paste.googleplex.com/6699322946093056, with the following
output: https://paste.googleplex.com/5591288895242240
This patch:
1. Update `docs/how_to_add_your_feature_flag.md` to incentive
developers to the non public versions.
2. Move ~70 features back into the private version.
3. Programmatically update the includes to include the correct
#include header(s).
4. For consistency and minimizing the amount of files modified,
the two headers to use the `features::` namespace.
AX-Relnotes: n/a.
Change-Id: Id9126a95dfbc533d4778b188b659b5acc9b3d9e3
Bug: None
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4836057
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1194718}
IWA-gated features check the WebExposedIsolationLevel of their frame or
process, and enable themselves if the isolation level is
kMaybeIsolatedApplication. Currently WebExposedIsolationLevel (WEIL)
maps directly to WebExposedIsolationInfo (WEII), which results in
cross-origin IWA child frames claiming to have the
kMaybeIsolatedApplication level, which is incorrect.
This direct mapping is incorrect because WEII and WEIL are meant to
represent two different concepts: WEII represents the cross-origin
isolation (COI) mode of a browsing context group, while WEIL
represents the COI capability of a frame or worker, and should take
into account the 'cross-origin-isolated' permissions policy and the
frame/worker's origin.
All agents in a browsing context group will share a WEII, but the
WEIL of a kIsolated frame's child frame (same or cross-origin) could
be kNotIsolated if permissions policy did not delegate the
'cross-origin-isolated' permission. kMaybeIsolatedApplication
can only be delegated to same-origin pages in addition to being
controllable with permissions policy. This CL adds WEIL to
ProcessLock and lowers the isolation level based on origin if needed.
It also adds permissions policy checks to
RenderFrameHost::GetWebExposedIsolationLevel so that it will return
the correct isolation level if the permission policy wasn't
delegated to a child frame.
See go/content-isolation-levels for more background information.
Bug: 1467673, 1159832, b/280497768
Change-Id: I2a328185c9379a96d56f77178ba5e283148524b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4568370
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Daniel d'Andrada <dandrader@google.com>
Cr-Commit-Position: refs/heads/main@{#1182214}
The SiteInstanceGroupManager class currently is mainly used for
handling the default SiteInstance. As we move forward, we don't
necessarily need it and would like to possibly remove it.
Right now, SiteInstanceGroups are constructed through the
GetOrCreateGroupForNewSiteInstance function, but the
SiteInstanceGroup constructor can be called directly instead.
Bug: 1291351
Change-Id: I4a16d6400aedc97cb2291ecc8f3a218ce5949c3a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4656805
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1173267}
In crrev.com/c/4445209 we added some metrics detailing related active
contents to investigate the conditions of a page that is not bfcached
because of related active contents existence, but since it uses the
count of documents in each SiteInstance, it can't differentiate when
the documents are actually cross-site and can never be same origin
even after document.domain modification (i.e. they can never access
each other synchronously) but share the same SiteInstance (the default SiteInstance), which is possible on Android.
This CL make us use per-url-derived-SiteInfo document counts instead,
which will handle the default SiteInstance case, because it uses the
navigation's real UrlInfo instead of the SiteInstance's SiteInfo
which would be set to unisolated.invalid. This means documents that
can never be same origin (because their 'real' site URLs are
completely different from each other) won't be counted.
Bug: 1444759
Change-Id: I6bc460083256bbcede6d6177c08edee5fad2cb0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4603703
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1159349}
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}
