[Connection-Allowlist] Add UseCounters for allowlist enforcement. [chromium/src : main]
Andrew Verge (Gerrit)
Andrew Verge added 2 comments![]( "Open in Gerrit")
navigation_request.frame_tree_node()->current_frame_host(),Since we are mid-navigation here, and have not committed yet iiuc, I believe this refers to the current document in the frame rather than the new one. This feels a bit weird, since the Connection-Allowlist header applies for the new document. But if we just need a valid frame to trigger a UseCounter log, maybe that doesn't matter?
base::WeakPtr<RenderFrameHostImpl> creator_render_frame_host_;I originally tried storing this as a `raw_ptr`, but during tests, the RFH was being deleted before this throttle, so I switched to WeakPtr instead. I'm a bit confused about this. I figured the RFH's lifetime would be much longer than this object's.
Related details
- Alex Moshchuk
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. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
creator_render_frame_host) {This check might not be necessary: if the creator rfh is somehow null, we can just skip logging the UseCounter, but maybe we shouldn't derail the whole throttle because of it?
Alex Moshchuk (Gerrit)
Alex Moshchuk added 6 comments![]( "Open in Gerrit")
However, I figured we could try this out first.Can probably rephrase this to be more of a TODO to do UMA later if we proceed here. :)
Change-Id: I4aa01ebc8d9be34ec8c2a6c2c7713929f819132cAny bug number to add here?
std::string content_type;nit: consider creating a custom constructor so that it's not necessary to specify `/*content_type=*/""` in all places where we create ResponseEntry below and don't care about content_type.
navigation_request.frame_tree_node()->current_frame_host(),Since we are mid-navigation here, and have not committed yet iiuc, I believe this refers to the current document in the frame rather than the new one. This feels a bit weird, since the Connection-Allowlist header applies for the new document. But if we just need a valid frame to trigger a UseCounter log, maybe that doesn't matter?
It does seem that something like `LogWebFeatureForCurrentPage` should record features for the page that's using the feature, rather than the previous page.
FWIW, this is called from WillProcessResponse() and WillCommitWithoutUrlLoader(), and in both of those cases we should have already determined the final RFH for this navigation, even though we haven't committed it yet. That would be in `navigation_request->GetRenderFrameHost()`. Would that work here? One caveat is that the metrics code will need to work on a speculative RFH - not sure if that's a problem.
base::WeakPtr<RenderFrameHostImpl> creator_render_frame_host_;I originally tried storing this as a `raw_ptr`, but during tests, the RFH was being deleted before this throttle, so I switched to WeakPtr instead. I'm a bit confused about this. I figured the RFH's lifetime would be much longer than this object's.
I'd have expected the same for these workers, but I'm not familiar enough with them. @nhi...@chromium.org, do you know if it's expected that the RFH that's creating a worker might be gone here?
creator_render_frame_host) {This check might not be necessary: if the creator rfh is somehow null, we can just skip logging the UseCounter, but maybe we shouldn't derail the whole throttle because of it?
Yeah, seems dangerous in that if this timing is somehow possible to exploit, a page might abuse it to bypass the network restrictions for workers.
Related details
- Andrew Verge
- Hiroki Nakagawa
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. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
Just a heads-up that I had some discussion with mk...@chromium.org, and we may attempt to do this logging in Blink instead, which would also help resolve some of the ambiguity here around frame and worker lifetime stuff.
Related details
- Hiroki Nakagawa
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. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 2 comments![]( "Open in Gerrit")
base::WeakPtr<RenderFrameHostImpl> creator_render_frame_host_;Alex MoshchukI originally tried storing this as a `raw_ptr`, but during tests, the RFH was being deleted before this throttle, so I switched to WeakPtr instead. I'm a bit confused about this. I figured the RFH's lifetime would be much longer than this object's.
I'd have expected the same for these workers, but I'm not familiar enough with them. @nhi...@chromium.org, do you know if it's expected that the RFH that's creating a worker might be gone here?
Yes, this throttle can outlive the RFH. The throttle is owned by WorkerScriptFetcher, which handles its own lifecycle. The fetcher self-destructs upon load completion or failure, independent of the RFH's lifetime.
creator_render_frame_host) {Alex MoshchukThis check might not be necessary: if the creator rfh is somehow null, we can just skip logging the UseCounter, but maybe we shouldn't derail the whole throttle because of it?
Yeah, seems dangerous in that if this timing is somehow possible to exploit, a page might abuse it to bypass the network restrictions for workers.
`creator_render_frame_host` is null here when this is a nested worker (i.e., when a dedicated worker creates another dedicated worker). Instead of that, `ancestor_render_frame_host` should be used.
Related details
- Andrew Verge
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. |
Andrew Verge (Gerrit)
Andrew Verge added 3 comments![]( "Open in Gerrit")
No new code for now, just replying on some of the more ambiguous threads while I test out a renderer-side implementation.
navigation_request.frame_tree_node()->current_frame_host(),Alex MoshchukSince we are mid-navigation here, and have not committed yet iiuc, I believe this refers to the current document in the frame rather than the new one. This feels a bit weird, since the Connection-Allowlist header applies for the new document. But if we just need a valid frame to trigger a UseCounter log, maybe that doesn't matter?
It does seem that something like `LogWebFeatureForCurrentPage` should record features for the page that's using the feature, rather than the previous page.
FWIW, this is called from WillProcessResponse() and WillCommitWithoutUrlLoader(), and in both of those cases we should have already determined the final RFH for this navigation, even though we haven't committed it yet. That would be in `navigation_request->GetRenderFrameHost()`. Would that work here? One caveat is that the metrics code will need to work on a speculative RFH - not sure if that's a problem.
I'm not sure how we'd test that a speculative RFH would work here.
The problem with testing use counters logged from the browser is that you essentially have to use mocks to verify them: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/use_counter_wiki.md#:~:text=For%20use%20counters%20recorded%20from%20the%20browser%20process
Because the mock isn't actually going to do anything with it once provided, any behavior specific to speculative RFHs won't be exercised.
I clicked through the code for ChromeContentBrowserClient to see what will actually happen in prod, and I see we eventually need the PageLoadTracker for a frame, which I imagine doesn't exist for a speculative one: https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/metrics_web_contents_observer.cc;drc=10b6082ae57b34c1d623275ed00dae987fb2a590;l=1446
Looking through write references to active_pages_ and inactive_pages_ in that file, I don't see anything implying that a PageLoadTracker exists for a pending navigation, which to me implies that we're not expected to log usecounters for pending navigations.
base::WeakPtr<RenderFrameHostImpl> creator_render_frame_host_;Alex MoshchukI originally tried storing this as a `raw_ptr`, but during tests, the RFH was being deleted before this throttle, so I switched to WeakPtr instead. I'm a bit confused about this. I figured the RFH's lifetime would be much longer than this object's.
Hiroki NakagawaI'd have expected the same for these workers, but I'm not familiar enough with them. @nhi...@chromium.org, do you know if it's expected that the RFH that's creating a worker might be gone here?
Yes, this throttle can outlive the RFH. The throttle is owned by WorkerScriptFetcher, which handles its own lifecycle. The fetcher self-destructs upon load completion or failure, independent of the RFH's lifetime.
Do you think keeping the (creator/ancestor) RFH as a WeakPtr is acceptable here? Under test we confirm that if both are alive at once the metric will be logged. I imagine that in normal operation (meaning, not immediate teardown at the end of a browsertest), the RFH will generally be alive longer than the duration of a single script load.
Related details
- Alex Moshchuk
- Hiroki Nakagawa
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 2 comments![]( "Open in Gerrit")
navigation_request.frame_tree_node()->current_frame_host(),Alex MoshchukSince we are mid-navigation here, and have not committed yet iiuc, I believe this refers to the current document in the frame rather than the new one. This feels a bit weird, since the Connection-Allowlist header applies for the new document. But if we just need a valid frame to trigger a UseCounter log, maybe that doesn't matter?
Andrew VergeIt does seem that something like `LogWebFeatureForCurrentPage` should record features for the page that's using the feature, rather than the previous page.
FWIW, this is called from WillProcessResponse() and WillCommitWithoutUrlLoader(), and in both of those cases we should have already determined the final RFH for this navigation, even though we haven't committed it yet. That would be in `navigation_request->GetRenderFrameHost()`. Would that work here? One caveat is that the metrics code will need to work on a speculative RFH - not sure if that's a problem.
I'm not sure how we'd test that a speculative RFH would work here.
The problem with testing use counters logged from the browser is that you essentially have to use mocks to verify them: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/use_counter_wiki.md#:~:text=For%20use%20counters%20recorded%20from%20the%20browser%20process
Because the mock isn't actually going to do anything with it once provided, any behavior specific to speculative RFHs won't be exercised.
I clicked through the code for ChromeContentBrowserClient to see what will actually happen in prod, and I see we eventually need the PageLoadTracker for a frame, which I imagine doesn't exist for a speculative one: https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/metrics_web_contents_observer.cc;drc=10b6082ae57b34c1d623275ed00dae987fb2a590;l=1446
Looking through write references to active_pages_ and inactive_pages_ in that file, I don't see anything implying that a PageLoadTracker exists for a pending navigation, which to me implies that we're not expected to log usecounters for pending navigations.
OK, sounds like going via a speculative RFH might not work then, sigh. (It does kind of make sense that a document shouldn't be using any features unless it actually commits.)
base::WeakPtr<RenderFrameHostImpl> creator_render_frame_host_;Alex MoshchukI originally tried storing this as a `raw_ptr`, but during tests, the RFH was being deleted before this throttle, so I switched to WeakPtr instead. I'm a bit confused about this. I figured the RFH's lifetime would be much longer than this object's.
Hiroki NakagawaI'd have expected the same for these workers, but I'm not familiar enough with them. @nhi...@chromium.org, do you know if it's expected that the RFH that's creating a worker might be gone here?
Andrew VergeYes, this throttle can outlive the RFH. The throttle is owned by WorkerScriptFetcher, which handles its own lifecycle. The fetcher self-destructs upon load completion or failure, independent of the RFH's lifetime.
Do you think keeping the (creator/ancestor) RFH as a WeakPtr is acceptable here? Under test we confirm that if both are alive at once the metric will be logged. I imagine that in normal operation (meaning, not immediate teardown at the end of a browsertest), the RFH will generally be alive longer than the duration of a single script load.
Deferring this to nhiroki@. Maybe that's acceptable if this is just for metrics, where it might be ok to not log them if it's very rare for the RFH to not be alive here.
Related details
- Andrew Verge
- Hiroki Nakagawa
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. |