Pass the AgentClusterKey from the browser process [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 3 comments![]( "Open in Gerrit")
AgentClusterKey* agent_cluster_key);Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter name `agent_cluster_key` can be omitted from the function declaration in the header file as the type `AgentClusterKey*` is self-descriptive.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
WindowAgent(AgentGroupScheduler& agent_group_scheduler,Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter name `agent_cluster_key` can be omitted from the function declaration in the header file as the type `AgentClusterKey*` is self-descriptive.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
WindowAgent* GetAgentForAgentClusterKey(Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. Consider using an enum for `has_potential_universal_access_privilege` to improve clarity at call sites, for example: `enum class UniversalAccessPrivilege { kNo, kYes };`.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 3 comments![]( "Open in Gerrit")
Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter name `agent_cluster_key` can be omitted from the function declaration in the header file as the type `AgentClusterKey*` is self-descriptive.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
WindowAgent(AgentGroupScheduler& agent_group_scheduler,Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter name `agent_cluster_key` can be omitted from the function declaration in the header file as the type `AgentClusterKey*` is self-descriptive.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. Consider using an enum for `has_potential_universal_access_privilege` to improve clarity at call sites, for example: `enum class UniversalAccessPrivilege { kNo, kYes };`.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Acknowledged
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 4 comments![]( "Open in Gerrit")
@Alex: PTAL. This is moving the agent cluster decision in the browser process and passing it to the renderer process. I have taken the option to try to use the AgentClusterKey from SiteInfo as much as possible, as I think we want to move to using it most of the time eventually. However, if we want to keep closer to the current code, we could also just check whether we have an origin-keyed/site-keyed/COI key and always update the origin/site of the key.
// Now build the appropriate AgentClusterKey for commit, starting with theI have been considering moving all the code below to SiteInfo, as it essentially undoes all the special cases of GetAgentClusterKeyForURL, so maybe having the two in the same place to compare would be nice.
bool is_navigation_to_existing_entry =I still have one test failing for this specific case. I think it's a case of a subframe navigation to an existing subframe NavigationEntry. I'm not sure how to write the condition for it.
mojom::CrossOriginIsolationMode cross_origin_isolation_mode)The presubmit checks complain about non-Blink mojom usage, but since this is the public interface that seems to be the correct thing to do to eventually translate into Blink mojom types. I'm not sure where I need to authorize this as I would have imagined that third_party/blink/public/web would not be covered by the check.
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 10 comments![]( "Open in Gerrit")
commit_params->agent_cluster_key = blink::mojom::AgentClusterKey::New();Should `blink::CreateCommitNavigationParams()` also just set this? Seems like it's not really optional anyway.
(Though I guess it becomes messy if we make the agent cluster key a union as I suggested below–does the agent cluster key actually have three distinct states? Site, origin, and neither? Though based off the WebAgentClusterKey class... I'm not actually sure)
commit_params->agent_cluster_key = blink::mojom::AgentClusterKey::New();And that would help with some of the boilerplate elsewhere I think
// Indicates if cross-origin isolation gives access to cross-origin isolatedNit: extra space
// when cross-origin sioaltion is enabled by COOP + COEP. When cross-originNit: isolation.
// Additionally, the AgentClusterKey might have a CrossOriginIsolationKey isif
// The AgentClusterKey has either a site URL or an origin. In the first case,
// the AgentClusterKey is site keyed, in the second case, it is origin keyed.
url.mojom.Url? url;
url.mojom.Origin? origin;
// Additionally, the AgentClusterKey might have a CrossOriginIsolationKey is
// the context is cross-origin isolated. In this case, the AgentClusterKey
// must be origin-keyed.
// https://wicg.github.io/document-isolation-policy/#coi-agent-cluster-key
CrossOriginIsolationKey? isolation_key;It seems like maybe this should be a union consisting of either a `url.mojom.Url` or a `OriginKeyedAgentClusterKey` struct containing a `url.mojom.Origin` and a `CrossOriginIsolationKey` (not sure if the COI key is required in the case of origin-keyed origins)
std::optional<WebCrossOriginIsolationKey> isolation_key;As noted above, can we restructure this so it's a variant of a WebURL and a struct containing a WebSecurityOrigin and an optional key?
mojom::CrossOriginIsolationMode cross_origin_isolation_mode)The presubmit checks complain about non-Blink mojom usage, but since this is the public interface that seems to be the correct thing to do to eventually translate into Blink mojom types. I'm not sure where I need to authorize this as I would have imagined that third_party/blink/public/web would not be covered by the check.
I agree this is fine; can you link me to an example of the presubmit warning?
AgentClusterKey();What does this create by default? Is it site keyed, origin keyed, or neither?
// the custom HashTraits defined at the end of this file.I don't think we should have both--or at the very least, one should delegate to the other.
Can you file a TODO to clean this up? Is there a failing test someone can run when they are trying to figure out what's going wrong here?
Related details
- Alex Moshchuk
- Camille Lamy
Alex Moshchuk (Gerrit)
Alex Moshchuk added 11 comments![]( "Open in Gerrit")
Thanks! I'll defer to Daniel on the blink changes; a few comments on the content side.
class CONTENT_EXPORT AgentClusterKey {I guess I'm a bit worried that we now have 3-4 places that define AgentClusterKeys (here, mojo, WebAgentClusterKey, and blink::AgentClusterKey), and they might get out of sync. Maybe it's intentional that the browser-side and renderer-side keys aren't going to be identical (e.g., the renderer doesn't need oac_status_), though we could at least add comments reminding what else must be updated when someone wants to add something to the AgentClusterKey? And maybe we could use LINT.IfChange to enforce that things stay updated together where it makes sense?
// SiteInfo, as it will only duplicate the information in AgentClusterKey.Might be worth updating this documentation to mention the other definitions of agent cluster key used in the renderer (and for sending this key to the renderer), and how they differ practically from this key.
// Now build the appropriate AgentClusterKey for commit, starting with theI have been considering moving all the code below to SiteInfo, as it essentially undoes all the special cases of GetAgentClusterKeyForURL, so maybe having the two in the same place to compare would be nice.
Yes, it might be worth moving this code into a separate helper, and maybe SiteInfo is a better place for it, as this function is getting long. It might be worth keeping the focus of this function on the OAC stuff as before, and then computing the key_to_commit as a separate step.
One high-level concern I have with the content/browser side of this CL is indeed about introducing another place for handling these special cases, and moreover trying to reconstruct how we computed a SiteInfo based on the AgentClusterKey after the fact, which seems fragile (and potentially hard to maintain for future changes to the process model). Do you think it could be better to compute both the agent cluster key for SiteInfo and the agent cluster key to send to the renderer in the same place in GetAgentClusterKeyForURL? Basically, have all the special cases in one place, but computing two keys? Or at least compute the should_update_origin_or_site bit there, so that we know that the site URL we ended up with in SiteInfo is a Chrome-process-model specific URL and not what the actual agent cluster key should be per spec.
(I'm not sure this is better or even feasible, just curious if it's worth thinking about at all.)
// isolation key. Currently, pages cross-origin isolated through COOP and COEP
// are not given a cross-origin isolated AgentClusterKey. Override theIt seems that this will be addressed by your https://chromium-review.googlesource.com/c/chromium/src/+/6516176? Will you be landing this CL before or after that?
// the document already as a cross-origin isolated AgentClusterKey, whichhas
origin.scheme())) {Is this to undo the case [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/site_info.cc;l=791;drc=7ce25d3420a0eb77c46618536b3434f7b9ceef66)? Does it matter that this only applies to top-chrome WebUIs and not all WebUIs? Should it be more restricted to IsWebUIAndUsesTLDForProcessLockURL cases?
should_update_origin_or_site = true;I guess we don't have this problem with features::kDefaultSiteInstanceGroups?
// redirect causes the response URL to no longer match the original
// SiteInstance. In that case, create a new AgentClusterKey.Is this actually intended behavior and not a bug? I think we try to disqualify the original SiteInstance in the NavigationEntry from being reused if it's no longer suitable for the redirected URL, with things like RenderFrameHostManager::CanUseDestinationInstance(). I'm surprised it would need its own condition here, as it seems like it might be violating our SiteInstance assignment rules.
if (key_to_commit.IsSiteKeyed()) {Can there be some sort of conversion function to populate a mojo AgentClusterKey from a normal one, ideally in agent_cluster_key.*?
process_lock.agent_cluster_key().IsCrossOriginIsolated();Can this be cleaned up independently, or does it depend on something in this CL?
Related details
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy voted and added 21 comments![]( "Open in Gerrit")
Votes added by Camille Lamy
| Commit-Queue | +1 |
21 comments
class CONTENT_EXPORT AgentClusterKey {I guess I'm a bit worried that we now have 3-4 places that define AgentClusterKeys (here, mojo, WebAgentClusterKey, and blink::AgentClusterKey), and they might get out of sync. Maybe it's intentional that the browser-side and renderer-side keys aren't going to be identical (e.g., the renderer doesn't need oac_status_), though we could at least add comments reminding what else must be updated when someone wants to add something to the AgentClusterKey? And maybe we could use LINT.IfChange to enforce that things stay updated together where it makes sense?
I don't think we should pass the oac_status to the renderer process. I have added a LINT.IfChange comment (not sure of the usage, this is my first time using it).
I agree that the duplication is unfortunate, but this is a side effect of the CommitNavigation not being sent to Blink directly. Otherwise, we could have had just the mojo struct, and used a typemap to map this struct to the Mojo struct.
// SiteInfo, as it will only duplicate the information in AgentClusterKey.Might be worth updating this documentation to mention the other definitions of agent cluster key used in the renderer (and for sending this key to the renderer), and how they differ practically from this key.
Done
// Now build the appropriate AgentClusterKey for commit, starting with theAlex MoshchukI have been considering moving all the code below to SiteInfo, as it essentially undoes all the special cases of GetAgentClusterKeyForURL, so maybe having the two in the same place to compare would be nice.
Yes, it might be worth moving this code into a separate helper, and maybe SiteInfo is a better place for it, as this function is getting long. It might be worth keeping the focus of this function on the OAC stuff as before, and then computing the key_to_commit as a separate step.
One high-level concern I have with the content/browser side of this CL is indeed about introducing another place for handling these special cases, and moreover trying to reconstruct how we computed a SiteInfo based on the AgentClusterKey after the fact, which seems fragile (and potentially hard to maintain for future changes to the process model). Do you think it could be better to compute both the agent cluster key for SiteInfo and the agent cluster key to send to the renderer in the same place in GetAgentClusterKeyForURL? Basically, have all the special cases in one place, but computing two keys? Or at least compute the should_update_origin_or_site bit there, so that we know that the site URL we ended up with in SiteInfo is a Chrome-process-model specific URL and not what the actual agent cluster key should be per spec.
(I'm not sure this is better or even feasible, just curious if it's worth thinking about at all.)
I have added a member to AgentClusterKey tracking whether we should update its origin/site URL before sending it. This allows to set it to true in all the edge cases we have in GetAgentClusterKeyForURL. This covers pretty much every edge case, except 4:
- logical OAC (computed after we compute the AgentClusterKey)
- opaque origins that are not data URLs -> we only pass the non opaque URL to SiteInfo, so it doesn't know the origin is actually opaque.
- cross-origin isolated contexts when COI comes from COOP + COEP (temporary, will go away once we create AgentClusterKeys with COIKeys)
- navigation to an existing SiteInstance without SiteIsolation when there is a redirect. See more about this one in the comment about this behavior.
Wdyt of the new version?
// isolation key. Currently, pages cross-origin isolated through COOP and COEP
// are not given a cross-origin isolated AgentClusterKey. Override theIt seems that this will be addressed by your https://chromium-review.googlesource.com/c/chromium/src/+/6516176? Will you be landing this CL before or after that?
Yes the other CL will fix it, but I'd like to land this one first. This CL is needed for DIP on Android while the other isn't. And to land the other, I need to do more investigation on why the preparatory CLs failed, which I have pushed back until after I get DIP on Android.
// the document already as a cross-origin isolated AgentClusterKey, whichCamille Lamyhas
Done
Is this to undo the case [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/site_info.cc;l=791;drc=7ce25d3420a0eb77c46618536b3434f7b9ceef66)? Does it matter that this only applies to top-chrome WebUIs and not all WebUIs? Should it be more restricted to IsWebUIAndUsesTLDForProcessLockURL cases?
Marked as resolved.
I guess we don't have this problem with features::kDefaultSiteInstanceGroups?
No kDefaultSiteInstanceGroups properly creates SiteInstances.
// redirect causes the response URL to no longer match the original
// SiteInstance. In that case, create a new AgentClusterKey.Is this actually intended behavior and not a bug? I think we try to disqualify the original SiteInstance in the NavigationEntry from being reused if it's no longer suitable for the redirected URL, with things like RenderFrameHostManager::CanUseDestinationInstance(). I'm surprised it would need its own condition here, as it seems like it might be violating our SiteInstance assignment rules.
The test NavigationControllerBrowserTest.SiteInstanceChangeOnHistoryNavigation has expectations of the SiteInstance not changing when SiteIsolation is not enabled and the navigation to an existing SiteInstance encounters a cross-site reload.
After further investigation, RenderFrameHostManager::CanUseDestinationInstance() will call SiteInstanceImpl::IsSuitableForUrlInfo. This in turns calls RenderProcessHostImpl::IsSuitableHost. In the non-Site Isolation case, the RPH is unlocked and the destination does not require a locked process, so the SiteInstance is deemed suitable and reused, even if the SiteInstance is not the default SiteInstance and its AgentClusterKey does not match the URL of the navigation.
I still have one test failing for this specific case. I think it's a case of a subframe navigation to an existing subframe NavigationEntry. I'm not sure how to write the condition for it.
Done
Can there be some sort of conversion function to populate a mojo AgentClusterKey from a normal one, ideally in agent_cluster_key.*?
Done
process_lock.agent_cluster_key().IsCrossOriginIsolated();Can this be cleaned up independently, or does it depend on something in this CL?
It could be done beforehand. Basically, I changed the CrossOriginIsolationMode enum to be a blink mojo enum, so the old code does not compile. I could just change the enum to the new version, but since we introduced the helper function in the meantime I thought it'd be cleaner to use the helper function.
commit_params->agent_cluster_key = blink::mojom::AgentClusterKey::New();Should `blink::CreateCommitNavigationParams()` also just set this? Seems like it's not really optional anyway.
(Though I guess it becomes messy if we make the agent cluster key a union as I suggested below–does the agent cluster key actually have three distinct states? Site, origin, and neither? Though based off the WebAgentClusterKey class... I'm not actually sure)
Yes it makes sense to have blink::CreateCommitNavigationParams() create this. The AgentClusterKey has two states, site keyed or origin-keyed, now represented by a union. I think creating a default site keyed to GURL is fine, it can be replaced later down the road by the proper one.
// Indicates if cross-origin isolation gives access to cross-origin isolatedCamille LamyNit: extra space
Done
// when cross-origin sioaltion is enabled by COOP + COEP. When cross-originCamille LamyNit: isolation.
Done
// Additionally, the AgentClusterKey might have a CrossOriginIsolationKey isCamille Lamyif
Done
// The AgentClusterKey has either a site URL or an origin. In the first case,
// the AgentClusterKey is site keyed, in the second case, it is origin keyed.
url.mojom.Url? url;
url.mojom.Origin? origin;
// Additionally, the AgentClusterKey might have a CrossOriginIsolationKey is
// the context is cross-origin isolated. In this case, the AgentClusterKey
// must be origin-keyed.
// https://wicg.github.io/document-isolation-policy/#coi-agent-cluster-key
CrossOriginIsolationKey? isolation_key;It seems like maybe this should be a union consisting of either a `url.mojom.Url` or a `OriginKeyedAgentClusterKey` struct containing a `url.mojom.Origin` and a `CrossOriginIsolationKey` (not sure if the COI key is required in the case of origin-keyed origins)
Done
std::optional<WebCrossOriginIsolationKey> isolation_key;As noted above, can we restructure this so it's a variant of a WebURL and a struct containing a WebSecurityOrigin and an optional key?
Done
mojom::CrossOriginIsolationMode cross_origin_isolation_mode)Daniel ChengThe presubmit checks complain about non-Blink mojom usage, but since this is the public interface that seems to be the correct thing to do to eventually translate into Blink mojom types. I'm not sure where I need to authorize this as I would have imagined that third_party/blink/public/web would not be covered by the check.
I agree this is fine; can you link me to an example of the presubmit warning?
It seems it's no longer trigerring.
AgentClusterKey();What does this create by default? Is it site keyed, origin keyed, or neither?
A site-keyed AgentClusterKey with KURL(). It's only there so that it works with the map. Added comments to clarify.
// the custom HashTraits defined at the end of this file.I don't think we should have both--or at the very least, one should delegate to the other.
Can you file a TODO to clean this up? Is there a failing test someone can run when they are trying to figure out what's going wrong here?
Removed this. Adding an import of third_party/blink/renderer/platform/weborigin/security_origin.h in the file using the HashTraits seems to have fixed the issue.
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 18 comments![]( "Open in Gerrit")
Thanks, and sorry it took a while to find enough focus time for this!
// manner. However, the proper origin or site URL of the navigation shouldMaybe give an example here, and mention that ideally this wouldn't be needed if we always created spec-compliant AgentClusterKeys and used SiteInstanceGroups to handle process allocation?
void set_update_key_before_sending(bool update_key_before_sending) {Do we need to pass a value, or can we assume it's always true? Seems like there'd be no use case for forcing this to be off.
void set_update_key_before_sending(bool update_key_before_sending) {It'd be nice if the name reflected something about this being for the renderer process, or that the renderer version of the key will differ. Some alternatives:
- set_update_key_before_sending_to_renderer()
- set_key_update_required_for_renderer() or set_should_update_key_for_renderer()
- mark_for_renderer_key_correction() or set_needs_renderer_key_correction()
- or maybe even set_key_is_not_spec_compliant() (maybe this isn't accurate though)
// before being sent to the renderer process.Mention how ideally all browser-side AgentClusterKeys would match those that we send to the renderer, but we have some cases where the browser-side AgentClusterKey will be set according to process allocation decisions, rather than what is required per spec, and we want to keep this transparent to the renderer and send it an idealized version of this key.
bool force_update,Let's also describe what this param is for in the comment. Also, is it needed? Could we get the same behavior by calling set_update_key_before_sending() on the key when this is true, before calling this function? (Or do we want to avoid modifying the key for these cases?)
class CONTENT_EXPORT AgentClusterKey {Camille LamyI guess I'm a bit worried that we now have 3-4 places that define AgentClusterKeys (here, mojo, WebAgentClusterKey, and blink::AgentClusterKey), and they might get out of sync. Maybe it's intentional that the browser-side and renderer-side keys aren't going to be identical (e.g., the renderer doesn't need oac_status_), though we could at least add comments reminding what else must be updated when someone wants to add something to the AgentClusterKey? And maybe we could use LINT.IfChange to enforce that things stay updated together where it makes sense?
I don't think we should pass the oac_status to the renderer process. I have added a LINT.IfChange comment (not sure of the usage, this is my first time using it).
I agree that the duplication is unfortunate, but this is a side effect of the CommitNavigation not being sent to Blink directly. Otherwise, we could have had just the mojo struct, and used a typemap to map this struct to the Mojo struct.
Acknowledged
const url::Origin& navigation_origin,origin_to_commit might be clearer?
// Now build the appropriate AgentClusterKey for commit, starting with theAlex MoshchukI have been considering moving all the code below to SiteInfo, as it essentially undoes all the special cases of GetAgentClusterKeyForURL, so maybe having the two in the same place to compare would be nice.
Camille LamyYes, it might be worth moving this code into a separate helper, and maybe SiteInfo is a better place for it, as this function is getting long. It might be worth keeping the focus of this function on the OAC stuff as before, and then computing the key_to_commit as a separate step.
One high-level concern I have with the content/browser side of this CL is indeed about introducing another place for handling these special cases, and moreover trying to reconstruct how we computed a SiteInfo based on the AgentClusterKey after the fact, which seems fragile (and potentially hard to maintain for future changes to the process model). Do you think it could be better to compute both the agent cluster key for SiteInfo and the agent cluster key to send to the renderer in the same place in GetAgentClusterKeyForURL? Basically, have all the special cases in one place, but computing two keys? Or at least compute the should_update_origin_or_site bit there, so that we know that the site URL we ended up with in SiteInfo is a Chrome-process-model specific URL and not what the actual agent cluster key should be per spec.
(I'm not sure this is better or even feasible, just curious if it's worth thinking about at all.)
I have added a member to AgentClusterKey tracking whether we should update its origin/site URL before sending it. This allows to set it to true in all the edge cases we have in GetAgentClusterKeyForURL. This covers pretty much every edge case, except 4:
- logical OAC (computed after we compute the AgentClusterKey)
- opaque origins that are not data URLs -> we only pass the non opaque URL to SiteInfo, so it doesn't know the origin is actually opaque.
- cross-origin isolated contexts when COI comes from COOP + COEP (temporary, will go away once we create AgentClusterKeys with COIKeys)
- navigation to an existing SiteInstance without SiteIsolation when there is a redirect. See more about this one in the comment about this behavior.
Wdyt of the new version?
Thanks, this looks better to me. It's a bit unfortunate that these decisions are still pretty spread out (e.g., the four exception cases you mentioned are here, most cases are special-cased in GetAgentClusterKeyForURL(), some are in special-case SiteInfo constructors (e.g. CreateForErrorPage), and then the last redirect case is in an entirely different place. I don't know if there's fundamentally a better organization, though, given all the constraints and how everything is structured at the moment. E.g., computing the renderer-side key closer to the place where we compute the browser-side key may not be possible without additional context for some cases (e.g., the data URL or the redirect cases you mentioned).
I do have a couple of questions:
- One SiteInstance might get reused by several possibly concurrent navigations (e.g., in several subframes). Is it a problem that we might set the new flag for one navigation and then a different navigation for different URL/origin might look at it at commit time, and incorrectly rely on the value of that flag that was computed for a different navigation? Or do we expect that all navigations in a particular SiteInstance that's marked as having a non-compliant key will need to recompute the AgentClusterKey?
- Could we actually compute the renderer key from scratch at commit time, instead of basing it off of the browser-side key? We already have the origin to commit and at least some of the inputs for when it needs to be origin-keyed and/or cross-origin-isolated. Could that be simpler than trying to decide what pieces of the browser-side key can be reused?
// isolation key. Currently, pages cross-origin isolated through COOP and COEP
// are not given a cross-origin isolated AgentClusterKey. Override theCamille LamyIt seems that this will be addressed by your https://chromium-review.googlesource.com/c/chromium/src/+/6516176? Will you be landing this CL before or after that?
Yes the other CL will fix it, but I'd like to land this one first. This CL is needed for DIP on Android while the other isn't. And to land the other, I need to do more investigation on why the preparatory CLs failed, which I have pushed back until after I get DIP on Android.
Acknowledged
// redirect causes the response URL to no longer match the original
// SiteInstance. In that case, create a new AgentClusterKey.Camille LamyIs this actually intended behavior and not a bug? I think we try to disqualify the original SiteInstance in the NavigationEntry from being reused if it's no longer suitable for the redirected URL, with things like RenderFrameHostManager::CanUseDestinationInstance(). I'm surprised it would need its own condition here, as it seems like it might be violating our SiteInstance assignment rules.
The test NavigationControllerBrowserTest.SiteInstanceChangeOnHistoryNavigation has expectations of the SiteInstance not changing when SiteIsolation is not enabled and the navigation to an existing SiteInstance encounters a cross-site reload.
After further investigation, RenderFrameHostManager::CanUseDestinationInstance() will call SiteInstanceImpl::IsSuitableForUrlInfo. This in turns calls RenderProcessHostImpl::IsSuitableHost. In the non-Site Isolation case, the RPH is unlocked and the destination does not require a locked process, so the SiteInstance is deemed suitable and reused, even if the SiteInstance is not the default SiteInstance and its AgentClusterKey does not match the URL of the navigation.
Thanks, that helps. When site isolation is not enabled, I wonder if this case should instead create a new SiteInstance (with a SiteInfo that reflects the post-redirect URL) in the same SiteInstanceGroup, instead of staying in the old SiteInstance? Would doing that fix this issue, at least when we're using DefaultSiteInstanceGroups (which should be everywhere except Android Webview at this point)? The point of DefaultSiteInstanceGroups is to keep meaningful site URLs in cases like this, which this test case seems to violate, so might be worth filing this as a separate bug in DefaultSiteInstanceGroups.
url::Origin origin = url::Origin::Create(dest_url_info.url);Hmm, this seems like it might not be correct in corner cases like sandboxed frames or about:blank or error pages, or cases where dest_url_info.origin is set. I'm not sure exactly how to fix it, but it feels it should use NavigationRequest::GetOriginToCommit() (or GetTentativeOriginAtRequestTime() if this is called prior to ready-to-commit), which is what we use later to pass to GetAgentClusterKeyForNavigationCommit().
process_lock.agent_cluster_key().IsCrossOriginIsolated();Camille LamyCan this be cleaned up independently, or does it depend on something in this CL?
It could be done beforehand. Basically, I changed the CrossOriginIsolationMode enum to be a blink mojo enum, so the old code does not compile. I could just change the enum to the new version, but since we introduced the helper function in the meantime I thought it'd be cleaner to use the helper function.
Acknowledged
// origin or URL of the AgentClusterKey.Nit: these and similar comments might benefit from a simple concrete example of where they differ (e.g., special-case site URLs like chrome-error or data:nonce). (I know there'll be some duplication and that you do have examples elsewhere...)
/*update_key_before_sending=*/true);This probably needs a quick comment to explain that error pages commit in chrome-error:// SiteInstances, which is Chrome's process model behavior that shouldn't be reflected in the key exposed to the renderer.
Actually, what is the renderer-side AgentClusterKey for error pages? Is it based on an opaque origin?
/*update_key_before_sending=*/true);This probably needs a comment to explain why default SiteInstances set this to true (because by definition their Site URL will be shared among potentially multiple cross-site documents and won't match a particular navigation's site.)
/*update_key_before_sending=*/true);I'm on the fence about passing this new arg when creating every new AgentClusterKey. It seems the cases where we have to set this to true should gradually disappear as we roll out more uses of SiteInstanceGroups, right?
While requiring it as an explicit param is useful to force all callers to reason about it, I also think that we shouldn't add new exceptions going forward, and that all new code should use SiteInstanceGroups rather than add an exception that forces this update. So, maybe we can rely on a separate call to set_update_key_before_sending() for setting this flag, on newly created AgentClusterKeys that need this? That would also provide a more convenient place to add a comment to explain why each particular use case needs this set, and would also make it easier to identify all places that we'd want to eventually update to use SiteInstanceGroups.
If we wanted to keep call sites that return a create key right away, we could also consider making the function that sets this flag internally also return the key it just modified, or creating alternate versions of the constructors, e.g. CreateNonSpecCompliantSiteKeyed().
true);Why does this need to update the key? Is it because we're creating a site-keyed key but the site URL might not actually be a site but rather an (isolated) origin? Let's explain in a comment if so.
Related details
- Camille Lamy
- Daniel Cheng
Camille Lamy (Gerrit)
Camille Lamy added 15 comments![]( "Open in Gerrit")
Thanks for the review! It took me quite a while to answer as I was OOO for three weeks. Following your suggestion, I have modified the CL to always create the AgentClusterKey we pass to the renderer process from scratch. It does make things a lot simpler. It's a bit unfortunate. However, maybe when more edge cases get migrated to using SiteInstanceGroup we might attempt to use the AgentClusterKey from the SiteInstance.
// manner. However, the proper origin or site URL of the navigation shouldMaybe give an example here, and mention that ideally this wouldn't be needed if we always created spec-compliant AgentClusterKeys and used SiteInstanceGroups to handle process allocation?
This parameter no longer exists.
void set_update_key_before_sending(bool update_key_before_sending) {Do we need to pass a value, or can we assume it's always true? Seems like there'd be no use case for forcing this to be off.
This parameter no longer exists.
void set_update_key_before_sending(bool update_key_before_sending) {It'd be nice if the name reflected something about this being for the renderer process, or that the renderer version of the key will differ. Some alternatives:
- set_update_key_before_sending_to_renderer()
- set_key_update_required_for_renderer() or set_should_update_key_for_renderer()
- mark_for_renderer_key_correction() or set_needs_renderer_key_correction()
- or maybe even set_key_is_not_spec_compliant() (maybe this isn't accurate though)
This parameter no longer exists.
Mention how ideally all browser-side AgentClusterKeys would match those that we send to the renderer, but we have some cases where the browser-side AgentClusterKey will be set according to process allocation decisions, rather than what is required per spec, and we want to keep this transparent to the renderer and send it an idealized version of this key.
This parameter no longer exists.
Let's also describe what this param is for in the comment. Also, is it needed? Could we get the same behavior by calling set_update_key_before_sending() on the key when this is true, before calling this function? (Or do we want to avoid modifying the key for these cases?)
This parameter is no longer present in the latest version.
const url::Origin& navigation_origin,Camille Lamyorigin_to_commit might be clearer?
Done
// Now build the appropriate AgentClusterKey for commit, starting with theAlex MoshchukI have been considering moving all the code below to SiteInfo, as it essentially undoes all the special cases of GetAgentClusterKeyForURL, so maybe having the two in the same place to compare would be nice.
Camille LamyYes, it might be worth moving this code into a separate helper, and maybe SiteInfo is a better place for it, as this function is getting long. It might be worth keeping the focus of this function on the OAC stuff as before, and then computing the key_to_commit as a separate step.
One high-level concern I have with the content/browser side of this CL is indeed about introducing another place for handling these special cases, and moreover trying to reconstruct how we computed a SiteInfo based on the AgentClusterKey after the fact, which seems fragile (and potentially hard to maintain for future changes to the process model). Do you think it could be better to compute both the agent cluster key for SiteInfo and the agent cluster key to send to the renderer in the same place in GetAgentClusterKeyForURL? Basically, have all the special cases in one place, but computing two keys? Or at least compute the should_update_origin_or_site bit there, so that we know that the site URL we ended up with in SiteInfo is a Chrome-process-model specific URL and not what the actual agent cluster key should be per spec.
(I'm not sure this is better or even feasible, just curious if it's worth thinking about at all.)
Alex MoshchukI have added a member to AgentClusterKey tracking whether we should update its origin/site URL before sending it. This allows to set it to true in all the edge cases we have in GetAgentClusterKeyForURL. This covers pretty much every edge case, except 4:
- logical OAC (computed after we compute the AgentClusterKey)
- opaque origins that are not data URLs -> we only pass the non opaque URL to SiteInfo, so it doesn't know the origin is actually opaque.
- cross-origin isolated contexts when COI comes from COOP + COEP (temporary, will go away once we create AgentClusterKeys with COIKeys)
- navigation to an existing SiteInstance without SiteIsolation when there is a redirect. See more about this one in the comment about this behavior.
Wdyt of the new version?
Thanks, this looks better to me. It's a bit unfortunate that these decisions are still pretty spread out (e.g., the four exception cases you mentioned are here, most cases are special-cased in GetAgentClusterKeyForURL(), some are in special-case SiteInfo constructors (e.g. CreateForErrorPage), and then the last redirect case is in an entirely different place. I don't know if there's fundamentally a better organization, though, given all the constraints and how everything is structured at the moment. E.g., computing the renderer-side key closer to the place where we compute the browser-side key may not be possible without additional context for some cases (e.g., the data URL or the redirect cases you mentioned).
I do have a couple of questions:
- One SiteInstance might get reused by several possibly concurrent navigations (e.g., in several subframes). Is it a problem that we might set the new flag for one navigation and then a different navigation for different URL/origin might look at it at commit time, and incorrectly rely on the value of that flag that was computed for a different navigation? Or do we expect that all navigations in a particular SiteInstance that's marked as having a non-compliant key will need to recompute the AgentClusterKey?
- Could we actually compute the renderer key from scratch at commit time, instead of basing it off of the browser-side key? We already have the origin to commit and at least some of the inputs for when it needs to be origin-keyed and/or cross-origin-isolated. Could that be simpler than trying to decide what pieces of the browser-side key can be reused?
Following your suggestion, I am now computing the renderer key from scratch. It's a bit unfortunate that we can't use the browser one, but there seems to be too many edge cases right now that introduce complexity. What do you think of the computation from scratch version?
// redirect causes the response URL to no longer match the original
// SiteInstance. In that case, create a new AgentClusterKey.Camille LamyIs this actually intended behavior and not a bug? I think we try to disqualify the original SiteInstance in the NavigationEntry from being reused if it's no longer suitable for the redirected URL, with things like RenderFrameHostManager::CanUseDestinationInstance(). I'm surprised it would need its own condition here, as it seems like it might be violating our SiteInstance assignment rules.
Alex MoshchukThe test NavigationControllerBrowserTest.SiteInstanceChangeOnHistoryNavigation has expectations of the SiteInstance not changing when SiteIsolation is not enabled and the navigation to an existing SiteInstance encounters a cross-site reload.
After further investigation, RenderFrameHostManager::CanUseDestinationInstance() will call SiteInstanceImpl::IsSuitableForUrlInfo. This in turns calls RenderProcessHostImpl::IsSuitableHost. In the non-Site Isolation case, the RPH is unlocked and the destination does not require a locked process, so the SiteInstance is deemed suitable and reused, even if the SiteInstance is not the default SiteInstance and its AgentClusterKey does not match the URL of the navigation.
Thanks, that helps. When site isolation is not enabled, I wonder if this case should instead create a new SiteInstance (with a SiteInfo that reflects the post-redirect URL) in the same SiteInstanceGroup, instead of staying in the old SiteInstance? Would doing that fix this issue, at least when we're using DefaultSiteInstanceGroups (which should be everywhere except Android Webview at this point)? The point of DefaultSiteInstanceGroups is to keep meaningful site URLs in cases like this, which this test case seems to violate, so might be worth filing this as a separate bug in DefaultSiteInstanceGroups.
Yes that would probably be better. I have filed https://g-issues.chromium.org/issues/466026066 to keep track of the issue. In the meantime, the concern no longer applies since we're now always creating the AgentClusterKey for commit from scratch, so I am going to close this.
url::Origin origin = url::Origin::Create(dest_url_info.url);Hmm, this seems like it might not be correct in corner cases like sandboxed frames or about:blank or error pages, or cases where dest_url_info.origin is set. I'm not sure exactly how to fix it, but it feels it should use NavigationRequest::GetOriginToCommit() (or GetTentativeOriginAtRequestTime() if this is called prior to ready-to-commit), which is what we use later to pass to GetAgentClusterKeyForNavigationCommit().
We no longer have to track this since we are re-creating the AgentClusterKey from scratch at commit time.
Nit: these and similar comments might benefit from a simple concrete example of where they differ (e.g., special-case site URLs like chrome-error or data:nonce). (I know there'll be some duplication and that you do have examples elsewhere...)
This function no longer exists.
This probably needs a quick comment to explain that error pages commit in chrome-error:// SiteInstances, which is Chrome's process model behavior that shouldn't be reflected in the key exposed to the renderer.
Actually, what is the renderer-side AgentClusterKey for error pages? Is it based on an opaque origin?
This parameter no longer exists.
The renderer-side AgentClysterKey for error pages is just going to be a regular AgentClusterKey with the origin/site URL of the error page I believe. I don't think there is any code to commit the error page with an opaque origin.
This probably needs a comment to explain why default SiteInstances set this to true (because by definition their Site URL will be shared among potentially multiple cross-site documents and won't match a particular navigation's site.)
This parameter no longer exists.
/*update_key_before_sending=*/true);I'm on the fence about passing this new arg when creating every new AgentClusterKey. It seems the cases where we have to set this to true should gradually disappear as we roll out more uses of SiteInstanceGroups, right?
While requiring it as an explicit param is useful to force all callers to reason about it, I also think that we shouldn't add new exceptions going forward, and that all new code should use SiteInstanceGroups rather than add an exception that forces this update. So, maybe we can rely on a separate call to set_update_key_before_sending() for setting this flag, on newly created AgentClusterKeys that need this? That would also provide a more convenient place to add a comment to explain why each particular use case needs this set, and would also make it easier to identify all places that we'd want to eventually update to use SiteInstanceGroups.
If we wanted to keep call sites that return a create key right away, we could also consider making the function that sets this flag internally also return the key it just modified, or creating alternate versions of the constructors, e.g. CreateNonSpecCompliantSiteKeyed().
This parameter no longer exists.
Why does this need to update the key? Is it because we're creating a site-keyed key but the site URL might not actually be a site but rather an (isolated) origin? Let's explain in a comment if so.
This parameter no longer exists. The reason why it was set to true is indeed because it's a site-keyed key which does not use the regular site URL but the isolated origin.
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 1 comment![]( "Open in Gerrit")
Thanks for the review! It took me quite a while to answer as I was OOO for three weeks. Following your suggestion, I have modified the CL to always create the AgentClusterKey we pass to the renderer process from scratch. It does make things a lot simpler. It's a bit unfortunate. However, maybe when more edge cases get migrated to using SiteInstanceGroup we might attempt to use the AgentClusterKey from the SiteInstance.
Thanks! I'm looking forward to reviewing this simpler version (looks very nice after a quick skim!), but just to set expectations, we've in the middle of our CSA hackathon and I may not be able to get to this review for another few days. I'll get to it when I can, and please let me know if there's any urgency or time constraints here to be aware of.
Camille Lamy (Gerrit)
Camille Lamy added 1 comment![]( "Open in Gerrit")
Alex MoshchukThanks for the review! It took me quite a while to answer as I was OOO for three weeks. Following your suggestion, I have modified the CL to always create the AgentClusterKey we pass to the renderer process from scratch. It does make things a lot simpler. It's a bit unfortunate. However, maybe when more edge cases get migrated to using SiteInstanceGroup we might attempt to use the AgentClusterKey from the SiteInstance.
Thanks! I'm looking forward to reviewing this simpler version (looks very nice after a quick skim!), but just to set expectations, we've in the middle of our CSA hackathon and I may not be able to get to this review for another few days. I'll get to it when I can, and please let me know if there's any urgency or time constraints here to be aware of.
Thanks! It's not urgent, but if possible I'd like to get this CL and its follow-up CL in before the holidays. I don't think you will need to review the follow-up CL if you're in hackathon period, so teh part of interest to you in the next one is very small (removing teh tracking of COI from RenderProcessHost).
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 11 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
11 comments
Thanks, the new version makes content/ much simpler! content/ LGTM with some minor comments below. (Sorry again that it took me a while to get to this, due to the CSA hackathon and being out sick most of the week.)
// LINT.ThenChange(third_party/blink/public/mojom/frame/agent_cluster_key.mojom)Would it make sense to also list the two Blink files here for WebAgentClusterKey and blink::AgentClusterKey? Seems this can be a comma-separate list, per this [example](https://source.chromium.org/chromium/chromium/src/+/main:components/supervised_user/core/browser/proto/families_common.proto;l=40;drc=a96634cfe21812aa33b986ca36fdb5d01666f5be).
bool use_origin_keying,Maybe "is_origin_keyed"?
// |use_orgiin_keying| is false, an origin keyed will still be returned if aorigin
// |use_orgiin_keying| is false, an origin keyed will still be returned if anit: insert key
// done entirely in the browser process. |update_key_before_sending| is also notUpdate this comment, now that this no longer exists.
// Now build the appropriate AgentClusterKey for commit, starting with theAlex MoshchukI have been considering moving all the code below to SiteInfo, as it essentially undoes all the special cases of GetAgentClusterKeyForURL, so maybe having the two in the same place to compare would be nice.
Camille LamyYes, it might be worth moving this code into a separate helper, and maybe SiteInfo is a better place for it, as this function is getting long. It might be worth keeping the focus of this function on the OAC stuff as before, and then computing the key_to_commit as a separate step.
One high-level concern I have with the content/browser side of this CL is indeed about introducing another place for handling these special cases, and moreover trying to reconstruct how we computed a SiteInfo based on the AgentClusterKey after the fact, which seems fragile (and potentially hard to maintain for future changes to the process model). Do you think it could be better to compute both the agent cluster key for SiteInfo and the agent cluster key to send to the renderer in the same place in GetAgentClusterKeyForURL? Basically, have all the special cases in one place, but computing two keys? Or at least compute the should_update_origin_or_site bit there, so that we know that the site URL we ended up with in SiteInfo is a Chrome-process-model specific URL and not what the actual agent cluster key should be per spec.
(I'm not sure this is better or even feasible, just curious if it's worth thinking about at all.)
Alex MoshchukI have added a member to AgentClusterKey tracking whether we should update its origin/site URL before sending it. This allows to set it to true in all the edge cases we have in GetAgentClusterKeyForURL. This covers pretty much every edge case, except 4:
- logical OAC (computed after we compute the AgentClusterKey)
- opaque origins that are not data URLs -> we only pass the non opaque URL to SiteInfo, so it doesn't know the origin is actually opaque.
- cross-origin isolated contexts when COI comes from COOP + COEP (temporary, will go away once we create AgentClusterKeys with COIKeys)
- navigation to an existing SiteInstance without SiteIsolation when there is a redirect. See more about this one in the comment about this behavior.
Wdyt of the new version?
Camille LamyThanks, this looks better to me. It's a bit unfortunate that these decisions are still pretty spread out (e.g., the four exception cases you mentioned are here, most cases are special-cased in GetAgentClusterKeyForURL(), some are in special-case SiteInfo constructors (e.g. CreateForErrorPage), and then the last redirect case is in an entirely different place. I don't know if there's fundamentally a better organization, though, given all the constraints and how everything is structured at the moment. E.g., computing the renderer-side key closer to the place where we compute the browser-side key may not be possible without additional context for some cases (e.g., the data URL or the redirect cases you mentioned).
I do have a couple of questions:
- One SiteInstance might get reused by several possibly concurrent navigations (e.g., in several subframes). Is it a problem that we might set the new flag for one navigation and then a different navigation for different URL/origin might look at it at commit time, and incorrectly rely on the value of that flag that was computed for a different navigation? Or do we expect that all navigations in a particular SiteInstance that's marked as having a non-compliant key will need to recompute the AgentClusterKey?
- Could we actually compute the renderer key from scratch at commit time, instead of basing it off of the browser-side key? We already have the origin to commit and at least some of the inputs for when it needs to be origin-keyed and/or cross-origin-isolated. Could that be simpler than trying to decide what pieces of the browser-side key can be reused?
Following your suggestion, I am now computing the renderer key from scratch. It's a bit unfortunate that we can't use the browser one, but there seems to be too many edge cases right now that introduce complexity. What do you think of the computation from scratch version?
This seems much simpler to me indeed. Glad that this worked out. Let's go with this approach, and maybe in the future if/when we have fewer corner cases, we might be able to revisit using the browser-side key.
bool use_origin_agent_cluster = is_opaque_origin_because_sandbox ||maybe rename to something like is_origin_keyed_for_renderer? (Because it's covering more than OAC?)
GetRenderFrameHost()
->GetSiteInstance()
->GetSiteInfo()
.agent_cluster_key()
.IsOriginKeyed();I forget, would this introduce any new cases for which the renderer would see an origin-keyed key, which it wasn't seeing before?
// are not given a cross-origin isolated AgentClusterKey. Create one to sendadd "in their SiteInstance" or something like that, to be clear about which key this is describing?
if (origin->IsLocal()) {Double-checking the file: URL flow: I'm not sure if I'm following this correctly, but the new code in DocumentLoader::InitializeWindow() seems to [create](https://chromium-review.googlesource.com/c/chromium/src/+/7041044/25/third_party/blink/renderer/core/loader/document_loader.cc#2650) a default blink::AgentClusterKey() for the origin->IsLocal case. IIRC, this default key is site-keyed with an empty URL. So how will we still end up with an origin for which IsLocal() is true here and use file_url_agent_?
Related details
- Camille Lamy
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 9 comments![]( "Open in Gerrit")
Thanks for the review!
// LINT.ThenChange(third_party/blink/public/mojom/frame/agent_cluster_key.mojom)Would it make sense to also list the two Blink files here for WebAgentClusterKey and blink::AgentClusterKey? Seems this can be a comma-separate list, per this [example](https://source.chromium.org/chromium/chromium/src/+/main:components/supervised_user/core/browser/proto/families_common.proto;l=40;drc=a96634cfe21812aa33b986ca36fdb5d01666f5be).
Done
bool use_origin_keying,Camille LamyMaybe "is_origin_keyed"?
Done
// |use_orgiin_keying| is false, an origin keyed will still be returned if aCamille Lamyorigin
Done
// |use_orgiin_keying| is false, an origin keyed will still be returned if aCamille Lamynit: insert key
Done
// done entirely in the browser process. |update_key_before_sending| is also notUpdate this comment, now that this no longer exists.
Done
bool use_origin_agent_cluster = is_opaque_origin_because_sandbox ||maybe rename to something like is_origin_keyed_for_renderer? (Because it's covering more than OAC?)
Done
GetRenderFrameHost()
->GetSiteInstance()
->GetSiteInfo()
.agent_cluster_key()
.IsOriginKeyed();I forget, would this introduce any new cases for which the renderer would see an origin-keyed key, which it wasn't seeing before?
In terms of blink internals yes, but in terms in web visible behavior no.
Basically, even if cross-origin isolated Agents were not marked as origin keyed, we already have code that prevent document.domain from working in cross-origin isolated documents. And we also had code that would expose the agent as origin keyed to JS when the Agent is cross-origin isolated (there's a test for that somewhere that failed at one point when developing this). So this would not be an issue for the web visible behavior, in fact it's aligning more closely Blink's internals with what we actually expose to the page.
// are not given a cross-origin isolated AgentClusterKey. Create one to sendadd "in their SiteInstance" or something like that, to be clear about which key this is describing?
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
3 comments
(content/ still lgtm)
GetRenderFrameHost()
->GetSiteInstance()
->GetSiteInfo()
.agent_cluster_key()
.IsOriginKeyed();Camille LamyI forget, would this introduce any new cases for which the renderer would see an origin-keyed key, which it wasn't seeing before?
In terms of blink internals yes, but in terms in web visible behavior no.
Basically, even if cross-origin isolated Agents were not marked as origin keyed, we already have code that prevent document.domain from working in cross-origin isolated documents. And we also had code that would expose the agent as origin keyed to JS when the Agent is cross-origin isolated (there's a test for that somewhere that failed at one point when developing this). So this would not be an issue for the web visible behavior, in fact it's aligning more closely Blink's internals with what we actually expose to the page.
Thanks, makes sense. This might be worth mentioning in the last sentence of the CL description, since it's a little more nuanced than a pure refactoring CL.
if (origin->IsLocal()) {Double-checking the file: URL flow: I'm not sure if I'm following this correctly, but the new code in DocumentLoader::InitializeWindow() seems to [create](https://chromium-review.googlesource.com/c/chromium/src/+/7041044/25/third_party/blink/renderer/core/loader/document_loader.cc#2650) a default blink::AgentClusterKey() for the origin->IsLocal case. IIRC, this default key is site-keyed with an empty URL. So how will we still end up with an origin for which IsLocal() is true here and use file_url_agent_?
Ping on this question, though I actually defer to Daniel for blink things :)
Related details
- Camille Lamy
- Daniel Cheng
Camille Lamy (Gerrit)
Camille Lamy added 1 comment![]( "Open in Gerrit")
if (origin->IsLocal()) {Alex MoshchukDouble-checking the file: URL flow: I'm not sure if I'm following this correctly, but the new code in DocumentLoader::InitializeWindow() seems to [create](https://chromium-review.googlesource.com/c/chromium/src/+/7041044/25/third_party/blink/renderer/core/loader/document_loader.cc#2650) a default blink::AgentClusterKey() for the origin->IsLocal case. IIRC, this default key is site-keyed with an empty URL. So how will we still end up with an origin for which IsLocal() is true here and use file_url_agent_?
Ping on this question, though I actually defer to Daniel for blink things :)
Right this was indeed an issue and we did not go into this code block. I now create a site-keyed AgentClusterKey in the file case using the file URL, so we do hit that. As explained in the comment below, the AgentCluster is created with the first file AgentClusterKey the WindowAgentFactory encounters. We do need a key with a file URL stored in the AgentCluster so that an about blank document inherits a key that will be recognized as local and put in the same AgentCluster. It does work since all file URLs get assigned to this special AgentCluster, but it also means that the URL we stored in the AgentClusterKey might not match that of later documents.
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 33 comments![]( "Open in Gerrit")
Sorry for the long delay here. Most of the comments are around some plumbing and (hopefully) simplification; a few comments about the file: question.
// The origin of the document which triggered cross-origin isolation. It
// might be different from the origin associated with the AgentClusterKey
// when cross-origin isolation is enabled by COOP + COEP. When cross-originIt might be nice to give a concrete example or link to an example of what this would look like in practice, or give some background about why it works this way.
url.mojom.Origin common_coi_origin;
CrossOriginIsolationMode cross_origin_isolation_mode;```suggestion
url.mojom.Origin common_origin;
CrossOriginIsolationMode mode;
```
Since "cross origin isolation" mode is already in the type name, and otherwise, the field names are internally inconsistent.
std::variant<WebOriginKeyedAgentClusterKey, WebURL> key;It's a bit unusual to provide constructors and also provide direct field access.
I would probably make this one a class and add the conversion operators for AgentClusterKey here (similarly to how it works for `WebSecurityOrigin` and `WebURL`, which define conversion operators if `INSIDE_BLINK` is defined) so we don't need to add any getters. No reason for people to use the fields directly once it's constructed, since it only goes from //content into Blink anyway.
const std::optional<WebCrossOriginIsolationKey>& isolation_key)Nit: it would be better to pass as `const WebCrossOriginIsolationKey*` or as `base::optional_ref<const WebCrossOriginIsolationKey>`; passing by `const std::optional<T>&` is prone to "copies by implicit conversions". I don't have a strong preference which.
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs: "Generally, use std::optional to represent optional by-value inputs, and use a const pointer when the non-optional form would have used a reference."
However, even better here is to omit the constructors here and use designated initialisers; see my example down below.
struct WebOriginKeyedAgentClusterKey {Nit: add a comment here
WebSecurityOrigin common_coi_origin;
mojom::CrossOriginIsolationMode cross_origin_isolation_mode;
};1. It's a bit inconsistent that one field is named "coi" and the other expands the acronym.
2. cross origin isolation is in the type name already.
So I'd suggest:
```suggestion
WebSecurityOrigin common_origin;
mojom::CrossOriginIsolationMode mode;
};
```
WebCrossOriginIsolationKey(Let's just use designated initialisers.
static AgentClusterKey EmptyValue() { return AgentClusterKey::CreateEmpty(); }Nit: I think these can return const refs, since `AgentClusterKey` is somewhat large and not completely free to copy.
key_status += (1 << 1);Very minor nit: I would expect to see |= for bitmasks.
std::variant<KURL, scoped_refptr<SecurityOrigin>> key_;
// This is used by DocumentIsolationPolicy and COOP and COEP to isolate
// execution contexts with extra-capabilities in an agent cluster with the
// appropriate cross-origin isolation status. Setting this to nullopt means
// that the AgentClusterKey is not cross-origin isolated.
std::optional<CrossOriginIsolationKey> isolation_key_;A slightly more compact representation would be to have an inner `struct Empty` and `struct Deleted` instead of the bool fields. It's probably worth doing.
I think we should also group the origin-keyed info together like we do for the Web version, so this gives us:
```suggestion
// Tombstone markers for `blink::HashTraits`.
struct Empty {};
struct Deleted {};
struct OriginKey {
scoped_refptr<SecurityOrigin> origin;
std::optional<CrossOriginIsolationKey> isolation_key;
};
std::variant<KURL, OriginKey, Empty, Deleted> key_;
```
bool GetIsEmpty() const { return is_empty_; }Similarly let's restrict this to the `HashTraits`.
static AgentClusterKey CreateDeleted();Can we use passkey to restrict this to the HashTraits, since no "normal" code should be creating these values ever?
const std::optional<CrossOriginIsolationKey>& isolation_key);Similar comment about using `base::optional_ref` or a pointer here.
But given the fact that we have `CreateOriginKeyed()` above, I'm wondering if this even needs to be optional; the call in the .cc file always passes a `std::optional` with an engaged value.
scoped_refptr<SecurityOrigin> common_coi_origin;Naming-wise, it's a bit inconsistent that we use "common_coi_origin" here and "cross_origin_isolation_mode" below.
Given the name of the class, I would suggest simplifying things and just calling this `common_origin` and `mode`; that makes the names a bit shorter, since "cross origin isolation" is already in the type name.
scoped_refptr<SecurityOrigin> origin);Throughout this class, I think we probably want `scoped_refptr<const SecurityOrigin>`, which will remove the need for `IsolatedCopy()`.
This will have transitive effects elsewhere in Blink code, but it means that the SecurityOrigin can't change from underneath us which is good.
DISALLOW_NEW();This doesn't really hurt, but I'm also wondering if it's needed? There's nothing that needs to be traced, and it makes things a bit more complicated elsewhere (e.g. DisallowNewWrapper)
AgentClusterKey();Camille LamyWhat does this create by default? Is it site keyed, origin keyed, or neither?
A site-keyed AgentClusterKey with KURL(). It's only there so that it works with the map. Added comments to clarify.
I don't quite get why this is needed for map still; what am I missing? :)
return AgentClusterKey(origin, std::nullopt);Nit: std::move() (and include `<utility>`)
: common_coi_origin(common_coi_origin),Nit: std::move()
return AgentClusterKey(origin, isolation_key);Nit: std::move()
AgentClusterKey AgentClusterKey::CreateFromWebAgentClusterKey(Let's define these helpers as conversion operators on `WebAgentClusterKey` instead of here.
if (std::holds_alternative<WebURL>(web_ac_key.key)) {
return AgentClusterKey::CreateSiteKeyed(std::get<WebURL>(web_ac_key.key));
}
CHECK(std::holds_alternative<WebOriginKeyedAgentClusterKey>(web_ac_key.key));
const auto& origin_keyed_key =
std::get<WebOriginKeyedAgentClusterKey>(web_ac_key.key);
if (origin_keyed_key.isolation_key.has_value()) {
std::optional<CrossOriginIsolationKey> coi_key = CrossOriginIsolationKey(
origin_keyed_key.isolation_key->common_coi_origin.Get()->IsolatedCopy(),
origin_keyed_key.isolation_key->cross_origin_isolation_mode);
return AgentClusterKey::CreateWithCrossOriginIsolationKey(
origin_keyed_key.origin.Get()->IsolatedCopy(), coi_key);
}
return AgentClusterKey::CreateOriginKeyed(
origin_keyed_key.origin.Get()->IsolatedCopy());Note: I dropped the `IsolatedCopy()` calls below since it's not clear they're actually needed.
```suggestion
return std::visit(absl::Overload {
[] (const WebURL& site_url) {
return AgentClusterKey::CreateSiteKeyed(site_url);
},
[] (const WebOriginKeyedAgentClusterKey& cluster_key) {
if (cluster_key.isolation_key.has_value()) {
std::optional<CrossOriginIsolationKey> coi_key =
CrossOriginIsolationKey(
cluster_key.isolation_key->common_coi_origin,
cluster_key.isolation_key->cross_origin_isolation_mode);
return AgentClusterKey::CreateWithCrossOriginIsolationKey(
cluster_key.origin, coi_key);
}
return AgentClusterKey::CreateOriginKeyed(
cluster_key.origin);
}, web_ac_key.key);
```
WebAgentClusterKey web_ac_key;
if (IsSiteKeyed()) {
web_ac_key.key = std::get<KURL>(key_);
return web_ac_key;
}
std::optional<WebCrossOriginIsolationKey> coi_key;
if (isolation_key_.has_value()) {
coi_key = WebCrossOriginIsolationKey(
WebSecurityOrigin(isolation_key_->common_coi_origin),
isolation_key_->cross_origin_isolation_mode);
}
web_ac_key.key = WebOriginKeyedAgentClusterKey(
WebSecurityOrigin(std::get<scoped_refptr<SecurityOrigin>>(key_)),
coi_key);
return web_ac_key;Similarly let's use `std::visit()` here:
```suggestion
return std::visit(absl::Overload {
[] (const KURL& site_url) {
return WebAgentClusterKey(site_url);
},
// Assuming we group the origin params into a struct
[] (const OriginKey& key) {
if (key.isolation_key.has_value()) {
// Assuming we use designated initialisers.
return WebAgentClusterKey(
WebOriginKeyedAgentClusterKey {
.origin = key.origin,
.isolation_key = {
.common_coi_origin = key.isolation_key_->common_coi_origin,
.cross_origin_isolation_mode =
key.isolation_key_->cross_origin_isolation_mode
}
});
}
return WebOriginKeyedAgentClusterKey(
WebOriginKeyedAgentClusterKey { .origin = key.origin, });
},
[] (const auto&) { NOTREACHED(); },
},
});
```
if (is_deleted_ != b.is_deleted_ || is_empty_ != b.is_empty_) {
return false;
}
if (IsSiteKeyed()) {
if (!b.IsSiteKeyed()) {
return false;
}
CHECK(!GetCrossOriginIsolationKey().has_value() &&
!b.GetCrossOriginIsolationKey().has_value());
return GetURL() == b.GetURL();
}
CHECK(IsOriginKeyed());
if (!b.IsOriginKeyed()) {
return false;
}
if (!GetOrigin()->IsSameOriginWith(b.GetOrigin())) {
return false;
}
if (GetCrossOriginIsolationKey().has_value() !=
b.GetCrossOriginIsolationKey().has_value()) {
return false;
}
if (GetCrossOriginIsolationKey().has_value()) {
return GetCrossOriginIsolationKey().value() ==
b.GetCrossOriginIsolationKey().value();
}
return true;If we define operator== for all the types in `std::variant`, then I think we can simplify this (or default it)
: key_(origin), isolation_key_(isolation_key) {}Nit: std::move()
using AgentsMap = HeapHashMap<AgentClusterKey,
WeakMember<WindowAgent>,
HashTraits<AgentClusterKey>>;```suggestion
using AgentsMap = HeapHashMap<AgentClusterKey,
WeakMember<WindowAgent>>;
```
(I think the explicit HashTraits here is already the default anyway)
// Returns an instance of WindowAgent for |agent_cluster_key|.Very minor nit: since we're changing this, let's use backticks.
```suggestion
// Returns an instance of WindowAgent for `agent_cluster_key`.
```
// For `file:` scheme origins.This comment should be a few lines down.
: SecurityOrigin::Create(agent_cluster_key.GetURL());Do we really need to keep the site URL around, if we end up turning it into an origin here?
if (origin->IsLocal()) {Alex MoshchukDouble-checking the file: URL flow: I'm not sure if I'm following this correctly, but the new code in DocumentLoader::InitializeWindow() seems to [create](https://chromium-review.googlesource.com/c/chromium/src/+/7041044/25/third_party/blink/renderer/core/loader/document_loader.cc#2650) a default blink::AgentClusterKey() for the origin->IsLocal case. IIRC, this default key is site-keyed with an empty URL. So how will we still end up with an origin for which IsLocal() is true here and use file_url_agent_?
Camille LamyPing on this question, though I actually defer to Daniel for blink things :)
Right this was indeed an issue and we did not go into this code block. I now create a site-keyed AgentClusterKey in the file case using the file URL, so we do hit that. As explained in the comment below, the AgentCluster is created with the first file AgentClusterKey the WindowAgentFactory encounters. We do need a key with a file URL stored in the AgentCluster so that an about blank document inherits a key that will be recognized as local and put in the same AgentCluster. It does work since all file URLs get assigned to this special AgentCluster, but it also means that the URL we stored in the AgentClusterKey might not match that of later documents.
... yeah it's a bit weird but maybe it's "okay"? But see my question above; I'm wondering if we should be passing down site keys as a URL at all, or if it should just be an origin. If it was a file: origin, maybe that'd prevent the mismatch from happening at all?
// For all other cases, we rely on the provided AgentClusterKey.This comment seems out of place, since the extensions special case is right below it.
if (CommonSchemeRegistry::IsExtensionScheme(origin->Protocol().Ascii())) {Should we keep the comment for the extension case?
Related details
- Alex Moshchuk
- Camille Lamy
Camille Lamy (Gerrit)
Camille Lamy added 34 comments![]( "Open in Gerrit")
GetRenderFrameHost()
->GetSiteInstance()
->GetSiteInfo()
.agent_cluster_key()
.IsOriginKeyed();Camille LamyI forget, would this introduce any new cases for which the renderer would see an origin-keyed key, which it wasn't seeing before?
Alex MoshchukIn terms of blink internals yes, but in terms in web visible behavior no.
Basically, even if cross-origin isolated Agents were not marked as origin keyed, we already have code that prevent document.domain from working in cross-origin isolated documents. And we also had code that would expose the agent as origin keyed to JS when the Agent is cross-origin isolated (there's a test for that somewhere that failed at one point when developing this). So this would not be an issue for the web visible behavior, in fact it's aligning more closely Blink's internals with what we actually expose to the page.
Thanks, makes sense. This might be worth mentioning in the last sentence of the CL description, since it's a little more nuanced than a pure refactoring CL.
I have added a detailed explanation of the change.
// The origin of the document which triggered cross-origin isolation. It
// might be different from the origin associated with the AgentClusterKey
// when cross-origin isolation is enabled by COOP + COEP. When cross-originIt might be nice to give a concrete example or link to an example of what this would look like in practice, or give some background about why it works this way.
I have added extra details.
url.mojom.Origin common_coi_origin;
CrossOriginIsolationMode cross_origin_isolation_mode;```suggestion
url.mojom.Origin common_origin;CrossOriginIsolationMode mode;
```Since "cross origin isolation" mode is already in the type name, and otherwise, the field names are internally inconsistent.
Done
std::variant<WebOriginKeyedAgentClusterKey, WebURL> key;It's a bit unusual to provide constructors and also provide direct field access.
I would probably make this one a class and add the conversion operators for AgentClusterKey here (similarly to how it works for `WebSecurityOrigin` and `WebURL`, which define conversion operators if `INSIDE_BLINK` is defined) so we don't need to add any getters. No reason for people to use the fields directly once it's constructed, since it only goes from //content into Blink anyway.
Done
const std::optional<WebCrossOriginIsolationKey>& isolation_key)Nit: it would be better to pass as `const WebCrossOriginIsolationKey*` or as `base::optional_ref<const WebCrossOriginIsolationKey>`; passing by `const std::optional<T>&` is prone to "copies by implicit conversions". I don't have a strong preference which.
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs: "Generally, use std::optional to represent optional by-value inputs, and use a const pointer when the non-optional form would have used a reference."
However, even better here is to omit the constructors here and use designated initialisers; see my example down below.
Done
Nit: add a comment here
Done
WebSecurityOrigin common_coi_origin;
mojom::CrossOriginIsolationMode cross_origin_isolation_mode;
};1. It's a bit inconsistent that one field is named "coi" and the other expands the acronym.
2. cross origin isolation is in the type name already.So I'd suggest:
```suggestion
WebSecurityOrigin common_origin;
mojom::CrossOriginIsolationMode mode;
};
```
Done
Let's just use designated initialisers.
Done
static AgentClusterKey EmptyValue() { return AgentClusterKey::CreateEmpty(); }Nit: I think these can return const refs, since `AgentClusterKey` is somewhat large and not completely free to copy.
If I were to return const refs, I would need to define a static value whose reference I return, right? However, to do so I would generally use DEFINE_STATIC_LOCAL which does not work with factory methods like AgentClusterKey::CreateEmpty. I'd like to keep the factory methods, so not sure how to define the static variable in that case. Or am I missing something?
Very minor nit: I would expect to see |= for bitmasks.
Done
std::variant<KURL, scoped_refptr<SecurityOrigin>> key_;
// This is used by DocumentIsolationPolicy and COOP and COEP to isolate
// execution contexts with extra-capabilities in an agent cluster with the
// appropriate cross-origin isolation status. Setting this to nullopt means
// that the AgentClusterKey is not cross-origin isolated.
std::optional<CrossOriginIsolationKey> isolation_key_;A slightly more compact representation would be to have an inner `struct Empty` and `struct Deleted` instead of the bool fields. It's probably worth doing.
I think we should also group the origin-keyed info together like we do for the Web version, so this gives us:
```suggestion
// Tombstone markers for `blink::HashTraits`.
struct Empty {};
struct Deleted {};
struct OriginKey {
scoped_refptr<SecurityOrigin> origin;
std::optional<CrossOriginIsolationKey> isolation_key;
};
std::variant<KURL, OriginKey, Empty, Deleted> key_;
```
Done
Similarly let's restrict this to the `HashTraits`.
Done
Can we use passkey to restrict this to the HashTraits, since no "normal" code should be creating these values ever?
Done
const std::optional<CrossOriginIsolationKey>& isolation_key);Similar comment about using `base::optional_ref` or a pointer here.
But given the fact that we have `CreateOriginKeyed()` above, I'm wondering if this even needs to be optional; the call in the .cc file always passes a `std::optional` with an engaged value.
Done
Naming-wise, it's a bit inconsistent that we use "common_coi_origin" here and "cross_origin_isolation_mode" below.
Given the name of the class, I would suggest simplifying things and just calling this `common_origin` and `mode`; that makes the names a bit shorter, since "cross origin isolation" is already in the type name.
Done
Throughout this class, I think we probably want `scoped_refptr<const SecurityOrigin>`, which will remove the need for `IsolatedCopy()`.
This will have transitive effects elsewhere in Blink code, but it means that the SecurityOrigin can't change from underneath us which is good.
Done
This doesn't really hurt, but I'm also wondering if it's needed? There's nothing that needs to be traced, and it makes things a bit more complicated elsewhere (e.g. DisallowNewWrapper)
Done
AgentClusterKey();Camille LamyWhat does this create by default? Is it site keyed, origin keyed, or neither?
Daniel ChengA site-keyed AgentClusterKey with KURL(). It's only there so that it works with the map. Added comments to clarify.
I don't quite get why this is needed for map still; what am I missing? :)
Ok it seems that it is indeed not needed so I have removed it.
Nit: std::move() (and include `<utility>`)
Done
: common_coi_origin(common_coi_origin),Camille LamyNit: std::move()
Done
return AgentClusterKey(origin, isolation_key);Camille LamyNit: std::move()
Done
AgentClusterKey AgentClusterKey::CreateFromWebAgentClusterKey(Let's define these helpers as conversion operators on `WebAgentClusterKey` instead of here.
Done
if (std::holds_alternative<WebURL>(web_ac_key.key)) {
return AgentClusterKey::CreateSiteKeyed(std::get<WebURL>(web_ac_key.key));
}
CHECK(std::holds_alternative<WebOriginKeyedAgentClusterKey>(web_ac_key.key));
const auto& origin_keyed_key =
std::get<WebOriginKeyedAgentClusterKey>(web_ac_key.key);
if (origin_keyed_key.isolation_key.has_value()) {
std::optional<CrossOriginIsolationKey> coi_key = CrossOriginIsolationKey(
origin_keyed_key.isolation_key->common_coi_origin.Get()->IsolatedCopy(),
origin_keyed_key.isolation_key->cross_origin_isolation_mode);
return AgentClusterKey::CreateWithCrossOriginIsolationKey(
origin_keyed_key.origin.Get()->IsolatedCopy(), coi_key);
}
return AgentClusterKey::CreateOriginKeyed(
origin_keyed_key.origin.Get()->IsolatedCopy());Note: I dropped the `IsolatedCopy()` calls below since it's not clear they're actually needed.
```suggestion
return std::visit(absl::Overload {
[] (const WebURL& site_url) {
return AgentClusterKey::CreateSiteKeyed(site_url);
},
[] (const WebOriginKeyedAgentClusterKey& cluster_key) {
if (cluster_key.isolation_key.has_value()) {
std::optional<CrossOriginIsolationKey> coi_key =
CrossOriginIsolationKey(
cluster_key.isolation_key->common_coi_origin,
cluster_key.isolation_key->cross_origin_isolation_mode);
return AgentClusterKey::CreateWithCrossOriginIsolationKey(
cluster_key.origin, coi_key);
}
return AgentClusterKey::CreateOriginKeyed(
cluster_key.origin);
}, web_ac_key.key);
```
Done
WebAgentClusterKey web_ac_key;
if (IsSiteKeyed()) {
web_ac_key.key = std::get<KURL>(key_);
return web_ac_key;
}
std::optional<WebCrossOriginIsolationKey> coi_key;
if (isolation_key_.has_value()) {
coi_key = WebCrossOriginIsolationKey(
WebSecurityOrigin(isolation_key_->common_coi_origin),
isolation_key_->cross_origin_isolation_mode);
}
web_ac_key.key = WebOriginKeyedAgentClusterKey(
WebSecurityOrigin(std::get<scoped_refptr<SecurityOrigin>>(key_)),
coi_key);
return web_ac_key;Similarly let's use `std::visit()` here:
```suggestion
return std::visit(absl::Overload {
[] (const KURL& site_url) {
return WebAgentClusterKey(site_url);
},
// Assuming we group the origin params into a struct
[] (const OriginKey& key) {
if (key.isolation_key.has_value()) {
// Assuming we use designated initialisers.
return WebAgentClusterKey(
WebOriginKeyedAgentClusterKey {
.origin = key.origin,
.isolation_key = {
.common_coi_origin = key.isolation_key_->common_coi_origin,
.cross_origin_isolation_mode =
key.isolation_key_->cross_origin_isolation_mode
}
});
}
return WebOriginKeyedAgentClusterKey(
WebOriginKeyedAgentClusterKey { .origin = key.origin, });
},
[] (const auto&) { NOTREACHED(); },
},
});
```
Done
if (is_deleted_ != b.is_deleted_ || is_empty_ != b.is_empty_) {
return false;
}
if (IsSiteKeyed()) {
if (!b.IsSiteKeyed()) {
return false;
}
CHECK(!GetCrossOriginIsolationKey().has_value() &&
!b.GetCrossOriginIsolationKey().has_value());
return GetURL() == b.GetURL();
}
CHECK(IsOriginKeyed());
if (!b.IsOriginKeyed()) {
return false;
}
if (!GetOrigin()->IsSameOriginWith(b.GetOrigin())) {
return false;
}
if (GetCrossOriginIsolationKey().has_value() !=
b.GetCrossOriginIsolationKey().has_value()) {
return false;
}
if (GetCrossOriginIsolationKey().has_value()) {
return GetCrossOriginIsolationKey().value() ==
b.GetCrossOriginIsolationKey().value();
}
return true;If we define operator== for all the types in `std::variant`, then I think we can simplify this (or default it)
Done
: key_(origin), isolation_key_(isolation_key) {}Camille LamyNit: std::move()
This constructor no longer exists.
using AgentsMap = HeapHashMap<AgentClusterKey,
WeakMember<WindowAgent>,
HashTraits<AgentClusterKey>>;```suggestion
using AgentsMap = HeapHashMap<AgentClusterKey,
WeakMember<WindowAgent>>;
```(I think the explicit HashTraits here is already the default anyway)
Done
// Returns an instance of WindowAgent for |agent_cluster_key|.Very minor nit: since we're changing this, let's use backticks.
```suggestion
// Returns an instance of WindowAgent for `agent_cluster_key`.
```
Done
// For `file:` scheme origins.This comment should be a few lines down.
Done
: SecurityOrigin::Create(agent_cluster_key.GetURL());Do we really need to keep the site URL around, if we end up turning it into an origin here?
We do need to distinguish between site keyed and origin keyed AgentClusterKeys as it has web visible implications. Technically, we could store a site key as an origin, but I think it makes AgentClusterKey less readable:
- now key is a std::variant<scoped_refptr<const SecurityOrigin>, OriginKey, Empty, Deleted> where OriginKey corresponds to the origin-keyed situation while scoped_refptr<const SecurityOrigin> is in fact the site-keyed key.
- this also mimics what we're doing in the browser process
- we have to go reintroduce a more complex comparison operator in AgentClusterKey as scoped_refptr<const SecurityOrigin>::== does a pointer comparison, while we want a IsSameOriginWith comparison.
So overall, I think it's better to keep the std::variant<KURL, OriginKey, Empty, Deleted>.
Now if the question is whether we can create the universal file AgentClusterKey as origin-keyed, the answer is no as some tests expect it to be site keyed in this case.
if (origin->IsLocal()) {Alex MoshchukDouble-checking the file: URL flow: I'm not sure if I'm following this correctly, but the new code in DocumentLoader::InitializeWindow() seems to [create](https://chromium-review.googlesource.com/c/chromium/src/+/7041044/25/third_party/blink/renderer/core/loader/document_loader.cc#2650) a default blink::AgentClusterKey() for the origin->IsLocal case. IIRC, this default key is site-keyed with an empty URL. So how will we still end up with an origin for which IsLocal() is true here and use file_url_agent_?
Camille LamyPing on this question, though I actually defer to Daniel for blink things :)
Daniel ChengRight this was indeed an issue and we did not go into this code block. I now create a site-keyed AgentClusterKey in the file case using the file URL, so we do hit that. As explained in the comment below, the AgentCluster is created with the first file AgentClusterKey the WindowAgentFactory encounters. We do need a key with a file URL stored in the AgentCluster so that an about blank document inherits a key that will be recognized as local and put in the same AgentCluster. It does work since all file URLs get assigned to this special AgentCluster, but it also means that the URL we stored in the AgentClusterKey might not match that of later documents.
... yeah it's a bit weird but maybe it's "okay"? But see my question above; I'm wondering if we should be passing down site keys as a URL at all, or if it should just be an origin. If it was a file: origin, maybe that'd prevent the mismatch from happening at all?
I don't think this would solve the issue? Basically, the problem is that we require that when have two documents hosting two different file URLs they must use the same AgentCluster. But the two files could technically have different hosts, which would be reflected both in origin and URL. FWIW, I don't think it is an issue as the URL/Origin in AgentClusterKey is just here to select an AgentCluster, so if we need to special case this one case, this should be fine.
// For all other cases, we rely on the provided AgentClusterKey.This comment seems out of place, since the extensions special case is right below it.
Done
if (CommonSchemeRegistry::IsExtensionScheme(origin->Protocol().Ascii())) {Should we keep the comment for the extension case?
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
5 comments
LGTM w/nits
(If we do anything about the comments that require bigger changes, I would prefer a followup so we can land this)
static AgentClusterKey EmptyValue() { return AgentClusterKey::CreateEmpty(); }Camille LamyNit: I think these can return const refs, since `AgentClusterKey` is somewhat large and not completely free to copy.
If I were to return const refs, I would need to define a static value whose reference I return, right? However, to do so I would generally use DEFINE_STATIC_LOCAL which does not work with factory methods like AgentClusterKey::CreateEmpty. I'd like to keep the factory methods, so not sure how to define the static variable in that case. Or am I missing something?
It should be able to write something like:
```
DEFINE_STATIC_LOCAL(
AgentClusterKey, kEmpty, AgentClusterKey::CreateEmpty(PassKey()));
```
: SecurityOrigin::Create(agent_cluster_key.GetURL());Camille LamyDo we really need to keep the site URL around, if we end up turning it into an origin here?
We do need to distinguish between site keyed and origin keyed AgentClusterKeys as it has web visible implications. Technically, we could store a site key as an origin, but I think it makes AgentClusterKey less readable:
- now key is a std::variant<scoped_refptr<const SecurityOrigin>, OriginKey, Empty, Deleted> where OriginKey corresponds to the origin-keyed situation while scoped_refptr<const SecurityOrigin> is in fact the site-keyed key.
- this also mimics what we're doing in the browser process
- we have to go reintroduce a more complex comparison operator in AgentClusterKey as scoped_refptr<const SecurityOrigin>::== does a pointer comparison, while we want a IsSameOriginWith comparison.
So overall, I think it's better to keep the std::variant<KURL, OriginKey, Empty, Deleted>.
Now if the question is whether we can create the universal file AgentClusterKey as origin-keyed, the answer is no as some tests expect it to be site keyed in this case.
I'm not going to push strongly about it in this CL, but I think we should consider if it makes sense to use an origin to represent the site URL.
In a **site** URL, everything other than the scheme, host, and port are meaningless, right? But by allowing the caller to pass a URL, it implies that the path and other URL components might have a meaning (similar to a regular URL).
We can reduce the confusion by using something like `StrongAlias` and then only using an aliased name for it.
if (origin->IsLocal()) {Alex MoshchukDouble-checking the file: URL flow: I'm not sure if I'm following this correctly, but the new code in DocumentLoader::InitializeWindow() seems to [create](https://chromium-review.googlesource.com/c/chromium/src/+/7041044/25/third_party/blink/renderer/core/loader/document_loader.cc#2650) a default blink::AgentClusterKey() for the origin->IsLocal case. IIRC, this default key is site-keyed with an empty URL. So how will we still end up with an origin for which IsLocal() is true here and use file_url_agent_?
Camille LamyPing on this question, though I actually defer to Daniel for blink things :)
Daniel ChengRight this was indeed an issue and we did not go into this code block. I now create a site-keyed AgentClusterKey in the file case using the file URL, so we do hit that. As explained in the comment below, the AgentCluster is created with the first file AgentClusterKey the WindowAgentFactory encounters. We do need a key with a file URL stored in the AgentCluster so that an about blank document inherits a key that will be recognized as local and put in the same AgentCluster. It does work since all file URLs get assigned to this special AgentCluster, but it also means that the URL we stored in the AgentClusterKey might not match that of later documents.
Camille Lamy... yeah it's a bit weird but maybe it's "okay"? But see my question above; I'm wondering if we should be passing down site keys as a URL at all, or if it should just be an origin. If it was a file: origin, maybe that'd prevent the mismatch from happening at all?
I don't think this would solve the issue? Basically, the problem is that we require that when have two documents hosting two different file URLs they must use the same AgentCluster. But the two files could technically have different hosts, which would be reflected both in origin and URL. FWIW, I don't think it is an issue as the URL/Origin in AgentClusterKey is just here to select an AgentCluster, so if we need to special case this one case, this should be fine.
I kind of wonder if that means AgentClusterKey should have a file-specific variant... since COI can't apply to the file agent cluster can it? And it's not a traditional site URL either, since we allow all files to share.
(But in the interest of getting this landed, I think this should be a followup as well, if we decide it is actionable)
WebAgentClusterKey::WebAgentClusterKey(const AgentClusterKey& key) {
key_ = std::visit(
absl::Overload{
[](const KURL& site_url) {
return std::variant<WebOriginKeyedAgentClusterKey, WebURL>(
site_url);
},
[](const AgentClusterKey::OriginKey& origin_key) {
if (origin_key.isolation_key.has_value()) {
return std::variant<WebOriginKeyedAgentClusterKey, WebURL>(
WebOriginKeyedAgentClusterKey{
.origin = origin_key.origin,
.isolation_key = WebCrossOriginIsolationKey{
.common_origin =
origin_key.isolation_key->common_origin,
.mode = origin_key.isolation_key->mode}});
}
return std::variant<WebOriginKeyedAgentClusterKey, WebURL>(
WebOriginKeyedAgentClusterKey{
.origin = origin_key.origin,
});
},
[](const auto&) {
NOTREACHED();
return std::variant<WebOriginKeyedAgentClusterKey, WebURL>();
},
},
key.key_);
}
Nit: can we move this to the initializer list? Otherwise, it technically gets default-initialized to the first subvariant, before we then overwrite it. Sometimes the compiler can optimize it out, but not always.
The other suggestion is to use an explicit return type to simplify the lambdas. By having an explicit return type, this simplifies the lambdas a bit: the second lambda doesn't need to repeat the variant name twice, and the third lambda doesn't need to have a `return` statement that isn't reachable.
The two ways to do this are `decltype()` (which I've provided in the suggested edit) or providing a `private` alias for the variant type (maybe `KeyVariant`) and then using it below. `decltype()` is "clever"–possibly too clever. But I think it's acceptable.
```suggestion
WebAgentClusterKey::WebAgentClusterKey(const AgentClusterKey& key)
: key_(std::visit(
absl::Overload{
[](const KURL& site_url) -> decltype(key_) {
return WebURL(site_url);
},
[](const AgentClusterKey::OriginKey& origin_key)
-> decltype(key_) {
if (origin_key.isolation_key.has_value()) {
return WebOriginKeyedAgentClusterKey{
.origin = origin_key.origin,
.isolation_key = WebCrossOriginIsolationKey{
.common_origin =
origin_key.isolation_key->common_origin,
.mode = origin_key.isolation_key->mode}});
}
return WebOriginKeyedAgentClusterKey{
.origin = origin_key.origin});
},
[](const auto&) -> decltype(key_) {
NOTREACHED();
},
},
key.key_)) {
}
```
Related details
- Alex Moshchuk
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
1 comment
- Camille Lamy
Camille Lamy (Gerrit)
Camille Lamy voted and added 6 comments![]( "Open in Gerrit")
Votes added by Camille Lamy
| Commit-Queue | +2 |
6 comments
Thanks for the review! As mentioned in the comments, I'll work on the remaining comments in a follow-up CL.
// The origin of the document which triggered cross-origin isolation. It
// might be different from the origin associated with the AgentClusterKey
// when cross-origin isolation is enabled by COOP + COEP. When cross-originCamille LamyIt might be nice to give a concrete example or link to an example of what this would look like in practice, or give some background about why it works this way.
I have added extra details.
Done
static AgentClusterKey EmptyValue() { return AgentClusterKey::CreateEmpty(); }Camille LamyNit: I think these can return const refs, since `AgentClusterKey` is somewhat large and not completely free to copy.
Daniel ChengIf I were to return const refs, I would need to define a static value whose reference I return, right? However, to do so I would generally use DEFINE_STATIC_LOCAL which does not work with factory methods like AgentClusterKey::CreateEmpty. I'd like to keep the factory methods, so not sure how to define the static variable in that case. Or am I missing something?
It should be able to write something like:
```
DEFINE_STATIC_LOCAL(
AgentClusterKey, kEmpty, AgentClusterKey::CreateEmpty(PassKey()));```
Ah it compiles but only if you write
```
DEFINE_STATIC_LOCAL(
AgentClusterKey, kEmpty, (AgentClusterKey::CreateEmpty(PassKey())));
```
With parenthesis around the factory method. Otherwise it doesn't. Changed to the const ref as requested.
: SecurityOrigin::Create(agent_cluster_key.GetURL());Camille LamyDo we really need to keep the site URL around, if we end up turning it into an origin here?
Daniel ChengWe do need to distinguish between site keyed and origin keyed AgentClusterKeys as it has web visible implications. Technically, we could store a site key as an origin, but I think it makes AgentClusterKey less readable:
- now key is a std::variant<scoped_refptr<const SecurityOrigin>, OriginKey, Empty, Deleted> where OriginKey corresponds to the origin-keyed situation while scoped_refptr<const SecurityOrigin> is in fact the site-keyed key.
- this also mimics what we're doing in the browser process
- we have to go reintroduce a more complex comparison operator in AgentClusterKey as scoped_refptr<const SecurityOrigin>::== does a pointer comparison, while we want a IsSameOriginWith comparison.
So overall, I think it's better to keep the std::variant<KURL, OriginKey, Empty, Deleted>.
Now if the question is whether we can create the universal file AgentClusterKey as origin-keyed, the answer is no as some tests expect it to be site keyed in this case.
I'm not going to push strongly about it in this CL, but I think we should consider if it makes sense to use an origin to represent the site URL.
In a **site** URL, everything other than the scheme, host, and port are meaningless, right? But by allowing the caller to pass a URL, it implies that the path and other URL components might have a meaning (similar to a regular URL).
We can reduce the confusion by using something like `StrongAlias` and then only using an aliased name for it.
I have filed crbug.com/476389699 to track that this should be improved in a follow-up CL.
if (origin->IsLocal()) {Alex MoshchukDouble-checking the file: URL flow: I'm not sure if I'm following this correctly, but the new code in DocumentLoader::InitializeWindow() seems to [create](https://chromium-review.googlesource.com/c/chromium/src/+/7041044/25/third_party/blink/renderer/core/loader/document_loader.cc#2650) a default blink::AgentClusterKey() for the origin->IsLocal case. IIRC, this default key is site-keyed with an empty URL. So how will we still end up with an origin for which IsLocal() is true here and use file_url_agent_?
Camille LamyPing on this question, though I actually defer to Daniel for blink things :)
Daniel ChengRight this was indeed an issue and we did not go into this code block. I now create a site-keyed AgentClusterKey in the file case using the file URL, so we do hit that. As explained in the comment below, the AgentCluster is created with the first file AgentClusterKey the WindowAgentFactory encounters. We do need a key with a file URL stored in the AgentCluster so that an about blank document inherits a key that will be recognized as local and put in the same AgentCluster. It does work since all file URLs get assigned to this special AgentCluster, but it also means that the URL we stored in the AgentClusterKey might not match that of later documents.
Camille Lamy... yeah it's a bit weird but maybe it's "okay"? But see my question above; I'm wondering if we should be passing down site keys as a URL at all, or if it should just be an origin. If it was a file: origin, maybe that'd prevent the mismatch from happening at all?
Daniel ChengI don't think this would solve the issue? Basically, the problem is that we require that when have two documents hosting two different file URLs they must use the same AgentCluster. But the two files could technically have different hosts, which would be reflected both in origin and URL. FWIW, I don't think it is an issue as the URL/Origin in AgentClusterKey is just here to select an AgentCluster, so if we need to special case this one case, this should be fine.
I kind of wonder if that means AgentClusterKey should have a file-specific variant... since COI can't apply to the file agent cluster can it? And it's not a traditional site URL either, since we allow all files to share.
(But in the interest of getting this landed, I think this should be a followup as well, if we decide it is actionable)
I have filed crbug.com/476389699 to track that this should be improved in a follow-up CL.
WebAgentClusterKey::WebAgentClusterKey(const AgentClusterKey& key) {Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
33 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/execution_context/agent_cluster_key.h
Insertions: 8, Deletions: 4.
@@ -182,11 +182,15 @@
return StringHasher::HashMemory(base::as_byte_span(hash_codes));
}
- static AgentClusterKey EmptyValue() {
- return AgentClusterKey::CreateEmpty(PassKey());
+ static AgentClusterKey& EmptyValue() {
+ DEFINE_STATIC_LOCAL(AgentClusterKey, kEmpty,
+ (AgentClusterKey::CreateEmpty(PassKey())));
+ return kEmpty;
}
- static AgentClusterKey DeletedValue() {
- return AgentClusterKey::CreateDeleted(PassKey());
+ static AgentClusterKey& DeletedValue() {
+ DEFINE_STATIC_LOCAL(AgentClusterKey, kDeleted,
+ (AgentClusterKey::CreateDeleted(PassKey())));
+ return kDeleted;
}
};
```
```
The name of the file: third_party/blink/renderer/core/exported/web_agent_cluster_key.cc
Insertions: 17, Deletions: 23.
@@ -16,35 +16,29 @@
WebAgentClusterKey::WebAgentClusterKey(const WebOriginKeyedAgentClusterKey& key)
: key_(key) {}
-WebAgentClusterKey::WebAgentClusterKey(const AgentClusterKey& key) {
- key_ = std::visit(
- absl::Overload{
- [](const KURL& site_url) {
- return std::variant<WebOriginKeyedAgentClusterKey, WebURL>(
- site_url);
- },
- [](const AgentClusterKey::OriginKey& origin_key) {
- if (origin_key.isolation_key.has_value()) {
- return std::variant<WebOriginKeyedAgentClusterKey, WebURL>(
- WebOriginKeyedAgentClusterKey{
+WebAgentClusterKey::WebAgentClusterKey(const AgentClusterKey& key)
+ : key_(std::visit(
+ absl::Overload{
+ [](const KURL& site_url) -> decltype(key_) {
+ return WebURL(site_url);
+ },
+ [](const AgentClusterKey::OriginKey& origin_key)
+ -> decltype(key_) {
+ if (origin_key.isolation_key.has_value()) {
+ return WebOriginKeyedAgentClusterKey{
.origin = origin_key.origin,
.isolation_key = WebCrossOriginIsolationKey{
.common_origin =
origin_key.isolation_key->common_origin,
- .mode = origin_key.isolation_key->mode}});
- }
- return std::variant<WebOriginKeyedAgentClusterKey, WebURL>(
- WebOriginKeyedAgentClusterKey{
+ .mode = origin_key.isolation_key->mode}};
+ }
+ return WebOriginKeyedAgentClusterKey{
.origin = origin_key.origin,
- });
+ };
+ },
+ [](const auto&) -> decltype(key_) { NOTREACHED(); },
},
- [](const auto&) {
- NOTREACHED();
- return std::variant<WebOriginKeyedAgentClusterKey, WebURL>();
- },
- },
- key.key_);
-}
+ key.key_)) {}
WebAgentClusterKey::operator AgentClusterKey() const {
return std::visit(
```
Change information
Pass the AgentClusterKey from the browser process
This CL refactors the tracking of origin-keyed agent clusters in Blink.
Instead of passing booleans requiring origin-keyed agent clusters during
the navigation commit, the browser will pass an AgentClusterKey (which
may be site-keyed or origin-keyed). This AgentClusterKey will then be
used as a key in the map of Agents in WindowAgentFactory, replacing the
three separate maps of origin-keyed agents, opaque origin agents and
site-keyed agents.
This is mostly a refactoring CL which has no web-visible impact. However, it
does introduce one change to the code, where AgentClusters for cross-origin
isolated contexts are now marked as origin-keyed. Previously, they were marked
as site keyed, though they were exposed as origin-keyed to JavaScript by
overriding the origin-keyed status of the AgentCluster in cross-origin isolated
context. document.domain was also already forbidden in cross-origin isolated
context, so marking the AgentClusters as origin-keyed (which they are per spec)
should not result in any web-visible behavior change and should make the code
clearer.
'NO_IFTTT=Initializing the LINT block'
- M content/browser/BUILD.gn
- M content/browser/agent_cluster_key.cc
- M content/browser/agent_cluster_key.h
- M content/browser/agent_cluster_key_unittest.cc
- M content/browser/renderer_host/navigation_controller_impl.cc
- M content/browser/renderer_host/navigation_entry_impl.cc
- M content/browser/renderer_host/navigation_request.cc
- M content/browser/renderer_host/navigation_request.h
- M content/browser/renderer_host/render_process_host_impl.cc
- D content/browser/security/coop/cross_origin_isolation_mode.h
- M content/browser/site_info.cc
- M content/browser/url_info.cc
- M content/renderer/render_frame_impl.cc
- M third_party/blink/common/navigation/navigation_params.cc
- M third_party/blink/public/BUILD.gn
- M third_party/blink/public/mojom/BUILD.gn
- A third_party/blink/public/mojom/frame/agent_cluster_key.mojom
- M third_party/blink/public/mojom/navigation/navigation_params.mojom
- A third_party/blink/public/web/web_agent_cluster_key.h
- M third_party/blink/public/web/web_navigation_params.h
- M third_party/blink/renderer/bindings/core/v8/binding_security.cc
- M third_party/blink/renderer/core/dom/document.cc
- M third_party/blink/renderer/core/execution_context/agent.cc
- M third_party/blink/renderer/core/execution_context/agent.h
- A third_party/blink/renderer/core/execution_context/agent_cluster_key.cc
- A third_party/blink/renderer/core/execution_context/agent_cluster_key.h
- M third_party/blink/renderer/core/execution_context/build.gni
- M third_party/blink/renderer/core/execution_context/window_agent.cc
- M third_party/blink/renderer/core/execution_context/window_agent.h
- M third_party/blink/renderer/core/execution_context/window_agent_factory.cc
- M third_party/blink/renderer/core/execution_context/window_agent_factory.h
- M third_party/blink/renderer/core/exported/build.gni
- A third_party/blink/renderer/core/exported/web_agent_cluster_key.cc
- M third_party/blink/renderer/core/frame/local_dom_window.cc
- M third_party/blink/renderer/core/frame/web_local_frame_impl.cc
- M third_party/blink/renderer/core/loader/document_loader.cc
- M third_party/blink/renderer/core/loader/document_loader.h
- M third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Code-Review: +1 by Daniel Cheng, +1 by Alex Moshchuk
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |