Ignore FrameReplicationState updates from speculative RFHs [chromium/src : main]
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Hi Charlie,
Could you please take a look?
+CC Rakina (OOO) / Fergal: FYI
Related details
- Charlie Reis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis added 3 comments![]( "Open in Gerrit")
Thanks! A few initial questions as I get caught up from OOO.
pending deletion, or in the back-forward cache. kPendingCommit is
intentionally still accepted, since the renderer sends these messages
while committing the new document and before the browser has processed
DidCommitNavigation.I'm curious whether this is a problem? It seems similar to the CSP case being changed in https://chromium-review.googlesource.com/c/chromium/src/+/7877125, so that the state is sent at DidCommitNavigation time and not before.
// kPendingCommit, or kPrerendering). Note that kPendingCommit must beI'm not sure I understand why prerendering is ok. That document might not activate for quite a while after it commits, which makes it seem like it could set values that are seen by the current RFH in the meantime. Does it not use the same BrowsingContextState, or is there another reason it's ok?
Related details
- Arthur Sonzogni
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 2 comments![]( "Open in Gerrit")
pending deletion, or in the back-forward cache. kPendingCommit is
intentionally still accepted, since the renderer sends these messages
while committing the new document and before the browser has processed
DidCommitNavigation.I'm curious whether this is a problem? It seems similar to the CSP case being changed in https://chromium-review.googlesource.com/c/chromium/src/+/7877125, so that the state is sent at DidCommitNavigation time and not before.
Yes, this annoys me. At the moment, we can handle IPCs about the document, prior to receiving the DidCommit of the document, which is problematic.
Ideally, we would break the IPC into two parts:
- Initials IPCs to setup the communications with the renderer and initiate the creation of the document.
- IPC sent from the committed document.
The second parts would be "bound" to the RenderFrameHost after DidCommit (e.g. this could be an interface requested with browser_interface_binders).
I don't have easy solution I can implement immediately. Please see this as a better than nothing improvement.
// kPendingCommit, or kPrerendering). Note that kPendingCommit must beI'm not sure I understand why prerendering is ok. That document might not activate for quite a while after it commits, which makes it seem like it could set values that are seen by the current RFH in the meantime. Does it not use the same BrowsingContextState, or is there another reason it's ok?
My understanding of prerendering (subject to recent updates): Like BFCache, we only support full 'page' prerendering rather than individual iframe documents. Because prerendering runs inside an independent FrameTree, it cannot affect other active pages. Additionally, prerendering is disallowed if the page maintains a window.open relationship with another context, which would have been problematic.
Related details
- Charlie Reis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis voted and added 3 comments![]( "Open in Gerrit")
Votes added by Charlie Reis
| Code-Review | +1 |
3 comments
Thanks! Basically LGTM apart from the prerendering case. Feel free to pull in a prerendering-knowledgable reviewer if that's useful for resolving the question.
pending deletion, or in the back-forward cache. kPendingCommit is
intentionally still accepted, since the renderer sends these messages
while committing the new document and before the browser has processed
DidCommitNavigation.Arthur SonzogniI'm curious whether this is a problem? It seems similar to the CSP case being changed in https://chromium-review.googlesource.com/c/chromium/src/+/7877125, so that the state is sent at DidCommitNavigation time and not before.
Yes, this annoys me. At the moment, we can handle IPCs about the document, prior to receiving the DidCommit of the document, which is problematic.
Ideally, we would break the IPC into two parts:
- Initials IPCs to setup the communications with the renderer and initiate the creation of the document.
- IPC sent from the committed document.
The second parts would be "bound" to the RenderFrameHost after DidCommit (e.g. this could be an interface requested with browser_interface_binders).
I don't have easy solution I can implement immediately. Please see this as a better than nothing improvement.
Thanks, that sounds like a good plan, and consistent in spirit with what Diana did in the CL above. No need to solve in this CL.
// kPendingCommit, or kPrerendering). Note that kPendingCommit must beArthur SonzogniI'm not sure I understand why prerendering is ok. That document might not activate for quite a while after it commits, which makes it seem like it could set values that are seen by the current RFH in the meantime. Does it not use the same BrowsingContextState, or is there another reason it's ok?
My understanding of prerendering (subject to recent updates): Like BFCache, we only support full 'page' prerendering rather than individual iframe documents. Because prerendering runs inside an independent FrameTree, it cannot affect other active pages. Additionally, prerendering is disallowed if the page maintains a window.open relationship with another context, which would have been problematic.
Those sound like reasons to treat it the same as BFCache, since it sounds like neither will attempt to make changes to the BrowsingContextState of the current RFH.
Does anything fail if we leave kPrerendering out of the condition, and ideally cancel the prerender similar to line 539?
If there is a reason that prerendering has to be allowed to target the current BrowsingContextState, we should leave a comment about it, but so far that doesn't sound safe to me. (I suppose we could leave a TODO to tackle that next if necessary, but it seems a lot like the other cases we're guarding against here.)
Related details
- Arthur Sonzogni
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Commit-Queue | +2 |
1 comment
// kPendingCommit, or kPrerendering). Note that kPendingCommit must beArthur SonzogniI'm not sure I understand why prerendering is ok. That document might not activate for quite a while after it commits, which makes it seem like it could set values that are seen by the current RFH in the meantime. Does it not use the same BrowsingContextState, or is there another reason it's ok?
Charlie ReisMy understanding of prerendering (subject to recent updates): Like BFCache, we only support full 'page' prerendering rather than individual iframe documents. Because prerendering runs inside an independent FrameTree, it cannot affect other active pages. Additionally, prerendering is disallowed if the page maintains a window.open relationship with another context, which would have been problematic.
Those sound like reasons to treat it the same as BFCache, since it sounds like neither will attempt to make changes to the BrowsingContextState of the current RFH.
Does anything fail if we leave kPrerendering out of the condition, and ideally cancel the prerender similar to line 539?
If there is a reason that prerendering has to be allowed to target the current BrowsingContextState, we should leave a comment about it, but so far that doesn't sound safe to me. (I suppose we could leave a TODO to tackle that next if necessary, but it seems a lot like the other cases we're guarding against here.)
Unlike BFCache (where the FrameTreeNode is shared with the active page), prerender runs in a completely isolated FrameTree that has never seen any active RenderFrameHost before activation. Its replication updates only mutate its own isolated BrowsingContextState.
Additionally, prerendered pages actively load and legitimately receive updates (e.g., ad tagging, CSP).
Tested on CQ:
- **Dropping `kPendingCommit`** (crrev.com/c/8157252): Fails `content_unittests` (`RenderFrameHostManagerAdTaggingSignalTest.*`) because `UpdateIsAdFrame` arrives before `DidCommitNavigation` is processed.
- **Cancelling on `kPrerendering`** (crrev.com/c/8169280): Fails `browser_tests` (`*PrerenderingBrowserTest.*AdTagging*`) because prerendered pages validly update ad tagging status.
I've updated the comment in `CanApplyFrameReplicationUpdate` and filed crbug.com/547754865 with TODOs for both cases.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Adding the missing owner for extending the enum:
- chrome_track_event.proto => nu...@chromium.org
- Page.pdl => alexr...@chromium.org
- enums.xml => chromium-met...@google.com
Related details
- Alex Rudenko
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From analysis/uma/chrome-metrics.gwsq:
Histograms should by default be reviewed by the owners of the subdirectories. The chromium-met...@google.com gwsq should be used when there are no individual owners, or for escalation to the Metrics team.
If you are interested in becoming a metrics reviewer, please review the instructions at https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#Becoming-a-Metrics-Reviewer
Reviewer source(s):
rka...@chromium.org is from context(analysis/uma/chrome-metrics.gwsq)
Related details
- Alex Rudenko
- Robert Kaplow
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
RfhEnforceInsecureNavigationsSetIs there a name that would make it clear for external consumers what the reason is about? RFH is a Chromium-specific internal term so it might not be well understood by developers of CDP clients. Perhaps we can drop the Rfh prefix?
Related details
- Arthur Sonzogni
- Robert Kaplow
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
Thanks, devtools LGTM + a suggestion to improve naming of the new reason codes.
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
1 comment
ChromeTrackEvent LGTM assuming agreement on the names for the reasons.
Related details
- Arthur Sonzogni
- Robert Kaplow
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
RfhEnforceInsecureNavigationsSetIs there a name that would make it clear for external consumers what the reason is about? RFH is a Chromium-specific internal term so it might not be well understood by developers of CDP clients. Perhaps we can drop the Rfh prefix?
Thanks Alex. Since these reasons reflect temporary Chromium-internal RFH state that we plan to resolve at some point, I decided not to expose them in DevTools at all. They are important for chromium metrics, but not for devtools consumers. I've reverted the Page.pdl changes and mapped them to Unknown in page_handler.cc instead.
Does that sounds good to you? See change in: content/browser/devtools/protocol/page_handler.cc
Related details
- Alex Rudenko
- Charlie Reis
- Robert Kaplow
- Stephen Nusko
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 Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
RfhEnforceInsecureNavigationsSetArthur SonzogniIs there a name that would make it clear for external consumers what the reason is about? RFH is a Chromium-specific internal term so it might not be well understood by developers of CDP clients. Perhaps we can drop the Rfh prefix?
Thanks Alex. Since these reasons reflect temporary Chromium-internal RFH state that we plan to resolve at some point, I decided not to expose them in DevTools at all. They are important for chromium metrics, but not for devtools consumers. I've reverted the Page.pdl changes and mapped them to Unknown in page_handler.cc instead.
Does that sounds good to you? See change in: content/browser/devtools/protocol/page_handler.cc
- Arthur Sonzogni
- Charlie Reis
- Robert Kaplow
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Auto-Submit | +1 |
1 comment
Thanks! Waiting for Robert review now.
Related details
- Robert Kaplow
Robert Kaplow (Gerrit)
Robert Kaplow voted Code-Review+1![]( "Open in Gerrit")
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Commit-Queue+2![]( "Open in Gerrit")
Arthur Sonzogni (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Ignore FrameReplicationState updates from speculative RFHs
EnforceInsecureRequestPolicy, EnforceInsecureNavigationsSet and
HadStickyUserActivationBeforeNavigationChanged write directly to the
BrowsingContextState that a speculative RenderFrameHost shares with the
FrameTreeNode's current document. This means that updates received on a
speculative RenderFrameHost would overwrite the FrameReplicationState of
the still-current cross-origin document and be broadcast to its proxies.
Ignore these updates when the receiving RenderFrameHost is speculative,
pending deletion, or in the back-forward cache. kPendingCommit is
intentionally still accepted, since the renderer sends these messages
while committing the new document and before the browser has processed
DidCommitNavigation.
- M base/tracing/protos/chrome_track_event.proto
- M content/browser/back_forward_cache/back_forward_cache_can_store_document_result.cc
- M content/browser/back_forward_cache_internal_browsertest.cc
- M content/browser/devtools/protocol/page_handler.cc
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/security_exploit_browsertest.cc
- M content/public/browser/back_forward_cache.h
- M tools/metrics/histograms/metadata/navigation/enums.xml
Code-Review: +1 by Robert Kaplow
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |