Add support for not-allowed hints in AcceptCHFrameInterceptor [chromium/src : main]
Victor Tan (Gerrit)
Victor Tan added 5 comments![]( "Open in Gerrit")
Add support for not-allowed hints in AcceptCHFrameInterceptorstarting M139 we saw the `ClientHints.AcceptCHFrame` drop significantly, it would be better understand the reason before we turn on related feature in stable.
enabled_client_hints.not_allowed_hints.push_back(hint);It would cause confused for people about this two variables meaning, could you please document/comment the actually meaning of `enabled_client_hints.hints` and `enabled_client_hints.not_allowed_hints`
based on your current logic:
- `enabled_client_hints.hints` is allowed and enabled client hints.
- `enabled_client_hints.not_allowed_hints` is not allowed client hints
am i understanding correctly? if so, can we just use the `if` instead of `else if`?
EXPECT_TRUE(actual_hints.not_allowed_hints.empty());can you add a positive tests case that not_allowed_hints actually has something?
if (!std::all_of(as the lambda function because complex, I don't think it's easy to review. could you refactor it to a separate function? or add documents what it does.
array<WebClientHintsType> not_allowed_hints;please document the difference between hints and not_allowed_hints, what actually are they?
Related details
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 5 comments![]( "Open in Gerrit")
Add support for not-allowed hints in AcceptCHFrameInterceptorstarting M139 we saw the `ClientHints.AcceptCHFrame` drop significantly, it would be better understand the reason before we turn on related feature in stable.
It might be tracked with crbug.com/442599247, right?
I set up an internal chat thread to discuss that with the network experts, and let me continue the discussion there.
In the meantime, I'd like to proceed with this CL as the changes here are self-contained. I will make sure to CC you on any related follow-ups, especially the Finch config GCL, to ensure you're in the loop on its path to stable.
It would cause confused for people about this two variables meaning, could you please document/comment the actually meaning of `enabled_client_hints.hints` and `enabled_client_hints.not_allowed_hints`
based on your current logic:
- `enabled_client_hints.hints` is allowed and enabled client hints.
- `enabled_client_hints.not_allowed_hints` is not allowed client hints
am i understanding correctly? if so, can we just use the `if` instead of `else if`?
Exactly. I have revised the code like so.
can you add a positive tests case that not_allowed_hints actually has something?
Done
The new test has been added for that purpose.
if (!std::all_of(as the lambda function because complex, I don't think it's easy to review. could you refactor it to a separate function? or add documents what it does.
Good point. I agree that the lambda function has grown a bit too complex. It was simpler initially, but with more logic added, it became hard to review.
To improve readability, I've refactored it from using `std::all_of` to a standard `for` loop, which makes the logic more explicit.
What do you think of this approach?
please document the difference between hints and not_allowed_hints, what actually are they?
- Victor Tan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Tan (Gerrit)
Victor Tan voted and added 3 comments![]( "Open in Gerrit")
Votes added by Victor Tan
| Code-Review | +1 |
3 comments
Add support for not-allowed hints in AcceptCHFrameInterceptorYoshisato Yanagisawastarting M139 we saw the `ClientHints.AcceptCHFrame` drop significantly, it would be better understand the reason before we turn on related feature in stable.
It might be tracked with crbug.com/442599247, right?
I set up an internal chat thread to discuss that with the network experts, and let me continue the discussion there.In the meantime, I'd like to proceed with this CL as the changes here are self-contained. I will make sure to CC you on any related follow-ups, especially the Finch config GCL, to ensure you're in the loop on its path to stable.
one concern is that if the metrics keeps drops, you would get mixed signal about your offload mechanism experiment. I'm fine to land this changes.
if (!std::all_of(Yoshisato Yanagisawaas the lambda function because complex, I don't think it's easy to review. could you refactor it to a separate function? or add documents what it does.
Good point. I agree that the lambda function has grown a bit too complex. It was simpler initially, but with more logic added, it became hard to review.
To improve readability, I've refactored it from using `std::all_of` to a standard `for` loop, which makes the logic more explicit.
What do you think of this approach?
either works for me, feel free to add some comments.
"Net.AcceptCHFrameInterceptor.MismatchClientHint", h);1) if that's the case, do we need to update the document for `Net.AcceptCHFrameInterceptor.MismatchClientHint` since this patch changes the meaning of the metric? after your enabling `kAcceptCHFrameOffloadNotAllowedHints`: a hint to be valid for offloading if it is in either the allowed hints list (hints) or the new not_allowed_hints list.
2) also, people might feel confused about the `hint_enabled` variable, do you mean `is_valid_for_offload`?
3) `is_in_not_allowed_hints && !is_in_hints` , does `!is_in_hints` check is necessary?
Related details
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 2 comments![]( "Open in Gerrit")
if (!std::all_of(Yoshisato Yanagisawaas the lambda function because complex, I don't think it's easy to review. could you refactor it to a separate function? or add documents what it does.
Victor TanGood point. I agree that the lambda function has grown a bit too complex. It was simpler initially, but with more logic added, it became hard to review.
To improve readability, I've refactored it from using `std::all_of` to a standard `for` loop, which makes the logic more explicit.
What do you think of this approach?
either works for me, feel free to add some comments.
Done
1) if that's the case, do we need to update the document for `Net.AcceptCHFrameInterceptor.MismatchClientHint` since this patch changes the meaning of the metric? after your enabling `kAcceptCHFrameOffloadNotAllowedHints`: a hint to be valid for offloading if it is in either the allowed hints list (hints) or the new not_allowed_hints list.
2) also, people might feel confused about the `hint_enabled` variable, do you mean `is_valid_for_offload`?
3) `is_in_not_allowed_hints && !is_in_hints` , does `!is_in_hints` check is necessary?
1) done.
Updated the histograms.xml and updated the metric name because its meaning has slightly changed.
2) done.
3) `!is_in_hints` is necessary. This clarifies the hints that would cause the browser IPC without the new feature param.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: toyo...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): toyo...@chromium.org
Note: IPC gwsq added no new reviewers; existing reviewers satisfied requirements!
Reviewer source(s):
toyo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 2 comments![]( "Open in Gerrit")
if (is_in_not_allowed_hints && !is_in_hints) {
base::UmaHistogramEnumeration(
"Net.AcceptCHFrameInterceptor.OffloadSuccessForNotAllowedHint", h);
}talked offline about this, but we may scan all hints for this metric, even after the early-return condition below.
const GURL kUrl("https://a.com");Can we use example.com, or other standardized testing domain name?
https://blog.jxck.io/entries/2017-09-27/example-local-test-domains.html
Same for another test below, and also for other existing tests in this file.
Related details
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Commit-Queue | +1 |
2 comments
if (is_in_not_allowed_hints && !is_in_hints) {
base::UmaHistogramEnumeration(
"Net.AcceptCHFrameInterceptor.OffloadSuccessForNotAllowedHint", h);
}talked offline about this, but we may scan all hints for this metric, even after the early-return condition below.
Yes. Updated the code to record all hints.
Can we use example.com, or other standardized testing domain name?
https://blog.jxck.io/entries/2017-09-27/example-local-test-domains.html
Same for another test below, and also for other existing tests in this file.
Done
I have also updated services/network/public/cpp/url_request_mojom_traits_unittest.cc.
Related details
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
2 comments
"Net.AcceptCHFrameInterceptor.MismatchClientHint2", h);nit: Can we break early here?
Related details
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
"Net.AcceptCHFrameInterceptor.MismatchClientHint2", h);nit: Can we break early here?
Unfortunately, no.
Upon https://chromium-review.googlesource.com/c/chromium/src/+/6894100/comment/93473c05_557c3e4e/, I have changed the logic to record all mismatched hints, and we cannot do early return for that.
Since the number of hints are at most 30 (https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/mojom/web_client_hints_types.mojom;l=65;drc=983f9254c5bb2c3333f26df50ed19b2d8ef0cda0), the loop may not be a large issue.
Related details
- Kenichi Ishibashi
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
"Net.AcceptCHFrameInterceptor.MismatchClientHint2", h);Yoshisato Yanagisawanit: Can we break early here?
Unfortunately, no.
Upon https://chromium-review.googlesource.com/c/chromium/src/+/6894100/comment/93473c05_557c3e4e/, I have changed the logic to record all mismatched hints, and we cannot do early return for that.
Since the number of hints are at most 30 (https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/mojom/web_client_hints_types.mojom;l=65;drc=983f9254c5bb2c3333f26df50ed19b2d8ef0cda0), the loop may not be a large issue.
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add support for not-allowed hints in AcceptCHFrameInterceptor
The Accept-CH offload mechanism was underperforming due to frequent,
unnecessary fallbacks to the browser process. This occurred when hints
in an Accept_CH frame did not match the `EnabledClientHints` in the
`network::ResourceRequest::TrustedParams`. This mismatch triggered a
fallback, but the browser process's `AreCriticalClientHintsMissing()`
check would then return `false`, resulting in an `kOk` status. It brings
needless roundtrips.
The root cause was a logic discrepancy. The network service's check did
not replicate the browser's logic, which considers persisted but
not-currently-allowed hints (e.g., due to Feature Policy) as valid for
this specific check. The network service incorrectly treated them as a
mismatch.
This change aligns the logic by making the network service aware of
these "not-allowed" hints. The browser now sends a list of not-allowed
hints in addition to allowed ones, and the `AcceptCHFrameInterceptor`
checks both. This avoids the unnecessary round trip.
This behavior is controlled by the
`kAcceptCHFrameOffloadNotAllowedHints` parameter of the
`kOffloadAcceptCHFrameCheck` feature.
OBSOLETE_HISTOGRAM[Net.AcceptCHFrameInterceptor.MismatchClientHint]=Replaced by Net.AcceptCHFrameInterceptor.MismatchClientHint2
- M content/browser/client_hints/client_hints.cc
- M content/browser/client_hints/client_hints_unittest.cc
- M services/network/accept_ch_frame_interceptor.cc
- M services/network/accept_ch_frame_interceptor_unittest.cc
- M services/network/public/cpp/features.cc
- M services/network/public/cpp/features.h
- M services/network/public/cpp/resource_request.h
- M services/network/public/cpp/url_request_mojom_traits.cc
- M services/network/public/cpp/url_request_mojom_traits.h
- M services/network/public/cpp/url_request_mojom_traits_unittest.cc
- M services/network/public/mojom/url_request.mojom
- M services/network/url_loader_unittest.cc
- M tools/metrics/histograms/metadata/net/histograms.xml
Code-Review: +1 by Kenichi Ishibashi, +1 by Victor Tan, +1 by Takashi Toyoshima
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |