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