Add js-profiling-mode enum Document Policy with eager/lazy V8 profiler initialization [chromium/src : main]
Michal Mocny (Gerrit)
Michal Mocny added 11 comments![]( "Open in Gerrit")
This is not my area of code expertise (doc policy not js profiling) but here is an initial review.
I haven't really analyzed the tests, but I think this is a solid start and we can expand the test cases a bit later, as needed.
You may be able/want to address some of my superficial comments, but don't hesitate to start to ask experts to start review for feedback / guidance on direction.
This is the first enum-type Document Policy feature in Chromium.Fancy!
const char* EnumValueToToken(int32_t value) {Style on returning char* based strings...
const char* token = EnumValueToToken(value.IntValue());So *any* enum will map to this specific enum values?
This feels fine as a local prototype, but I think you need some machinery to handle mapping the right enum type to the right enum strings. As a bad alternative, find a way to break this loudly when the next enum type is introduced so the next person has to deal with it...
default_value: "0",The `default_value` of `0` corresponds to `JSProfilingMode::kNone`. However, there isn't a corresponding token in `TokenToEnumValue` (line 29 in `document_policy_parser.cc`) or `EnumValueToToken` (line 40 in `document_policy.cc`) to represent `kNone`.
This means that while `kNone` is the default, it cannot be explicitly set via a Document Policy header. Only `eager` and `lazy` can be specified.
Is this intentional? If so, it might be worth adding a comment to clarify that `kNone` is an implicit default and cannot be set explicitly via the policy header, to avoid confusion if someone tries to write `js-profiling-mode='none'`.
std::optional<int32_t> TokenToEnumValue(const std::string& token) {Hmm, we go from enum string to enum value back to enum string later... Ideally these to_string and from_string would be defined together, indeally in `profiler_group.h`?
(Also this implementation has the same issue as the other one in that its generic for all enums but specific to this particular enum.)
---
WDYT about adding a second enum directly to `third_party/blink/public/mojom/permissions_policy/document_policy_feature.mojom`?
And then use `document_policy_features.json5`?
...that might need some build file pre-processing changes to support enum *values*, but I think this feels like the correct approach.
(I'm totally new to this area, but this is what I found with a bit of digging.)
void InitV8Profiler(v8::CpuProfilingLoggingMode logging_mode);Similarly here, can we just have one variant?
void OnProfilingContextAddedLazy(ExecutionContext* context);Nit: Just add a mode arg to the method above, with a default of eager?
int32_t enum_val = mode_value.IntValue();
if (enum_val == static_cast<int32_t>(JSProfilingMode::kEager) ||
enum_val == static_cast<int32_t>(JSProfilingMode::kLazy)) {
return static_cast<JSProfilingMode>(enum_val);
}Nit: style, but let's iterate on the design of the enum first
return true;If `GetProfilingMode()` returns `kNone`, it means that `IsFeatureEnabled(kJSProfiling)` has already returned `false`. Therefore, the `IsFeatureEnabled` check on line 102 will also be `false`, making this `return true;` unreachable.
Additionally, my understanding is that `IsFeatureEnabled` do some logging/reporting and the GetProfilingMode would skip its check if the new profiling mode was supplied without the general profiling policy.
---
If kJSProfiling is still required alongside kJSProfilingMode, I think you could swap the order of feature detection and if kJSProfiling is disabled always return kNone, otherwise return provided policy or default to eager.
If kJSProfiling is optional... maybe the current implementation is fine, but perhaps add a comment here?
context_observers_.insert(observer);THis will require expert review, but, it surpises me that you can init via `InitV8Profiler(v8::kLazyLogging);` but that we choose not to do so.
So you can have tri-state:
- Un-init
- Init w/ Lazy
- Init w/ Eager
This document policy adds 2 modes and picks (1) and (3).
(But it also creates a `ProfilingContextObserver` eagerly, which afaik isn't needed until you start to profile? Not sure.)
Related details
- Thomas BERTET
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. |
Monica Chintala (Gerrit)
Monica Chintala added 4 comments![]( "Open in Gerrit")
const char* EnumValueToToken(int32_t value) {Style on returning char* based strings...
Chromium style prefers std::string_view (or absl::string_view) over const char* for string returns. Also, nullptr is fragile, CHECK(token) would crash. Using std::optional<std::string_view> is better
std::optional<int32_t> TokenToEnumValue(const std::string& token) {Hmm, we go from enum string to enum value back to enum string later... Ideally these to_string and from_string would be defined together, indeally in `profiler_group.h`?
(Also this implementation has the same issue as the other one in that its generic for all enums but specific to this particular enum.)
---
WDYT about adding a second enum directly to `third_party/blink/public/mojom/permissions_policy/document_policy_feature.mojom`?
And then use `document_policy_features.json5`?
...that might need some build file pre-processing changes to support enum *values*, but I think this feels like the correct approach.
(I'm totally new to this area, but this is what I found with a bit of digging.)
Agree, If the enum values ever change, or another enum type is added, the parser/serializer silently break with no compiler error and enum should be in public files
//
// Enum feature tests.
//
{
"ParseEnumFeatureEager",
"f-enum=eager",
/* parsed_policy */
{
/* feature_state */ {{kEnumFeature, PolicyValue::CreateEnum(1)}},
/* endpoint_map */ {},
},
/* messages */ {},
},
{
"ParseEnumFeatureLazy",
"f-enum=lazy",
/* parsed_policy */
{
/* feature_state */ {{kEnumFeature, PolicyValue::CreateEnum(2)}},
/* endpoint_map */ {},
},
/* messages */ {},should we also have test coverage for verifying js-profiling-mode without a value ie., kNone default to check it throws warning
},Can we also have WPT tests under external/wpt/js-self-profiling/without-document-policy?
Related details
- Thomas BERTET
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. |
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
FYI, I am working on enabling profiling in [Workers](https://chromium-review.googlesource.com/c/chromium/src/+/7546208). We should also make this mode should works in Workers too.
Thomas Bertet (Gerrit)
Thomas Bertet added 1 comment![]( "Open in Gerrit")
FYI, I am working on enabling profiling in [Workers](https://chromium-review.googlesource.com/c/chromium/src/+/7546208). We should also make this mode should works in Workers too.
Oh that's great news! Thank you for sharing and for taking a look on this patchset!
Related details
- Thomas BERTET
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. |
Thomas Bertet (Gerrit)
Thomas Bertet added 12 comments![]( "Open in Gerrit")
Address review comments: API cleanup, lazy V8 init, parser tests, WPT tests..
Monica ChintalaStyle on returning char* based strings...
Chromium style prefers std::string_view (or absl::string_view) over const char* for string returns. Also, nullptr is fragile, CHECK(token) would crash. Using std::optional<std::string_view> is better
Acknowledged
So *any* enum will map to this specific enum values?
This feels fine as a local prototype, but I think you need some machinery to handle mapping the right enum type to the right enum strings. As a bad alternative, find a way to break this loudly when the next enum type is introduced so the next person has to deal with it...
Acknowledged
default_value: "0",The `default_value` of `0` corresponds to `JSProfilingMode::kNone`. However, there isn't a corresponding token in `TokenToEnumValue` (line 29 in `document_policy_parser.cc`) or `EnumValueToToken` (line 40 in `document_policy.cc`) to represent `kNone`.
This means that while `kNone` is the default, it cannot be explicitly set via a Document Policy header. Only `eager` and `lazy` can be specified.
Is this intentional? If so, it might be worth adding a comment to clarify that `kNone` is an implicit default and cannot be set explicitly via the policy header, to avoid confusion if someone tries to write `js-profiling-mode='none'`.
I think I would find the `none` enum value an interesting idea, even though it's not in the specification update yet. I guess we can do that in a the meantime.
In the next patchset, I've added a `none` enum value, let me know if you feel that's good!
std::optional<int32_t> TokenToEnumValue(const std::string& token) {Monica ChintalaHmm, we go from enum string to enum value back to enum string later... Ideally these to_string and from_string would be defined together, indeally in `profiler_group.h`?
(Also this implementation has the same issue as the other one in that its generic for all enums but specific to this particular enum.)
---
WDYT about adding a second enum directly to `third_party/blink/public/mojom/permissions_policy/document_policy_feature.mojom`?
And then use `document_policy_features.json5`?
...that might need some build file pre-processing changes to support enum *values*, but I think this feels like the correct approach.
(I'm totally new to this area, but this is what I found with a bit of digging.)
Agree, If the enum values ever change, or another enum type is added, the parser/serializer silently break with no compiler error and enum should be in public files
I've tentatively updated the code to follow your recommendation. Let me know what you think!
// Enum feature tests.
//
{
"ParseEnumFeatureEager",
"f-enum=eager",
/* parsed_policy */
{
/* feature_state */ {{kEnumFeature, PolicyValue::CreateEnum(1)}},
/* endpoint_map */ {},
},
/* messages */ {},
},
{
"ParseEnumFeatureLazy",
"f-enum=lazy",
/* parsed_policy */
{
/* feature_state */ {{kEnumFeature, PolicyValue::CreateEnum(2)}},
/* endpoint_map */ {},
},
/* messages */ {},should we also have test coverage for verifying js-profiling-mode without a value ie., kNone default to check it throws warning
Acknowledged
Can we also have WPT tests under external/wpt/js-self-profiling/without-document-policy?
Acknowledged
void InitV8Profiler(v8::CpuProfilingLoggingMode logging_mode);Similarly here, can we just have one variant?
Acknowledged
void OnProfilingContextAddedLazy(ExecutionContext* context);Nit: Just add a mode arg to the method above, with a default of eager?
Acknowledged
int32_t enum_val = mode_value.IntValue();
if (enum_val == static_cast<int32_t>(JSProfilingMode::kEager) ||
enum_val == static_cast<int32_t>(JSProfilingMode::kLazy)) {
return static_cast<JSProfilingMode>(enum_val);
}Nit: style, but let's iterate on the design of the enum first
Acknowledged
return true;If `GetProfilingMode()` returns `kNone`, it means that `IsFeatureEnabled(kJSProfiling)` has already returned `false`. Therefore, the `IsFeatureEnabled` check on line 102 will also be `false`, making this `return true;` unreachable.
Additionally, my understanding is that `IsFeatureEnabled` do some logging/reporting and the GetProfilingMode would skip its check if the new profiling mode was supplied without the general profiling policy.
---
If kJSProfiling is still required alongside kJSProfilingMode, I think you could swap the order of feature detection and if kJSProfiling is disabled always return kNone, otherwise return provided policy or default to eager.
If kJSProfiling is optional... maybe the current implementation is fine, but perhaps add a comment here?
I've reworked the logic to avoid unreachable return. Let me know what you think!
context_observers_.insert(observer);THis will require expert review, but, it surpises me that you can init via `InitV8Profiler(v8::kLazyLogging);` but that we choose not to do so.
So you can have tri-state:
- Un-init
- Init w/ Lazy
- Init w/ Eager
This document policy adds 2 modes and picks (1) and (3).
(But it also creates a `ProfilingContextObserver` eagerly, which afaik isn't needed until you start to profile? Not sure.)
Right! It makes sense to me to simplify this. I've reworked this to get rid of the `OnProfilingContextAddedLazy` and initialize V8 with `kLazyLogging`
Related details
- Michal Mocny
- Monica Chintala
- Thomas BERTET
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. |
Monica Chintala (Gerrit)
Monica Chintala added 8 comments![]( "Open in Gerrit")
return 0;Here and other places in the file, adding inline comments like // JSProfilingMode::kNone
next to each value would help readability
// Copyright 2025 The Chromium Authorsnit: 2026?
// profiler is ready during its lifetime. In kEager mode the V8 profiler is
// initialized immediately; in kLazy mode initialization is deferred until
// the first CreateProfiler() call.Should this be?
// In kEager mode the V8 profiler is initialized with eager logging
// in kLazy mode it is initialized with lazy logging (JIT event
// enumeration deferred until StartProfiling).
return static_cast<JSProfilingMode>(mode_value.IntValue());If mode_value.IntValue() is ever a value outside {0,1,2} this produces an undefined JSProfilingMode. A defensive switch or range check would be safer:
int32_t v = mode_value.IntValue();
if (v >= 0 && v <= static_cast<int32_t>(JSProfilingMode::kLazy))
return static_cast<JSProfilingMode>(v);
return JSProfilingMode::kNone;
bool ProfilerGroup::CanProfile(LocalDOMWindow* local_window,Also a WPT for policy interaction (header with js-profiling, js-profiling-mode=none) to confirm js-profiling-mode is prioritized.
// Check the new js-profiling-mode enum policy first.may be brief comment on explaining why it re-fetches rather than calling GetProfilingMode() would help for later
DCHECK(cpu_profiler_);nit: If mode == kNone and cpu_profiler_ is null, neither modes initializes it but this dcheck fires. The caller guards against kNone, but adding a DCHECK_NE(mode, JSProfilingMode::kNone) at function entry would make the contract explicit.
<!DOCTYPE html>Should we also add a test for invalid enum value (js-profiling-mode=unknown) to verify profiling is blocked?
Related details
- Michal Mocny
- Thomas BERTET
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. |
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
Adding lzhao@ who will be reviewing this work while I'm on leave for next couple of months.
Related details
- Michal Mocny
- Thomas BERTET
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. |
Thomas Bertet (Gerrit)
Thomas Bertet added 9 comments![]( "Open in Gerrit")
Following the review from Monica, Patchset 4 is fixing a bug in CanProfile where js-profiling-mode=none could be overridden by a concurrent js-profiling header, causing a DCHECK crash due to the V8 profiler never being initialized. The fix checks whether the enum policy is explicitly present and, if so, skips the legacy boolean policy fallback entirely.
Also added WPT tests covering the none-overrides-legacy case and unknown/invalid token handling.
Here and other places in the file, adding inline comments like // JSProfilingMode::kNone
next to each value would help readability
Done
// Copyright 2025 The Chromium AuthorsThomas Bertetnit: 2026?
Done
// profiler is ready during its lifetime. In kEager mode the V8 profiler is
// initialized immediately; in kLazy mode initialization is deferred until
// the first CreateProfiler() call.Should this be?
// In kEager mode the V8 profiler is initialized with eager logging
// in kLazy mode it is initialized with lazy logging (JIT event
// enumeration deferred until StartProfiling).
Done
return static_cast<JSProfilingMode>(mode_value.IntValue());If mode_value.IntValue() is ever a value outside {0,1,2} this produces an undefined JSProfilingMode. A defensive switch or range check would be safer:
int32_t v = mode_value.IntValue();
if (v >= 0 && v <= static_cast<int32_t>(JSProfilingMode::kLazy))
return static_cast<JSProfilingMode>(v);
return JSProfilingMode::kNone;
Done
bool ProfilerGroup::CanProfile(LocalDOMWindow* local_window,Also a WPT for policy interaction (header with js-profiling, js-profiling-mode=none) to confirm js-profiling-mode is prioritized.
Done
// Check the new js-profiling-mode enum policy first.may be brief comment on explaining why it re-fetches rather than calling GetProfilingMode() would help for later
Done
nit: If mode == kNone and cpu_profiler_ is null, neither modes initializes it but this dcheck fires. The caller guards against kNone, but adding a DCHECK_NE(mode, JSProfilingMode::kNone) at function entry would make the contract explicit.
Done
Should we also add a test for invalid enum value (js-profiling-mode=unknown) to verify profiling is blocked?
- Michal Mocny
- Monica Chintala
- Thomas BERTET
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. |
Liang Zhao (Gerrit)
Liang Zhao added 4 comments![]( "Open in Gerrit")
if (execution_context->IsDedicatedWorkerGlobalScope() &&
!RuntimeEnabledFeatures::DocumentPolicyInDedicatedWorkerEnabled()) {
if (exception_state) {
exception_state->ThrowDOMException(
DOMExceptionCode::kNotSupportedError,
"Document Policy is not enabled for this context.");
}
return false;
}
Should we still keep this block of code?
// Re-fetch the raw policy value rather than calling GetProfilingMode() so we
// can distinguish "enum policy explicitly present" from "absent/legacy". If
// the enum policy is present we honour it exclusively (even kNone) and skip
// the legacy kJSProfiling check entirely. If it is absent we fall back toGetProfilingMode() also ignores legacy kJSProfiling when kJSProfilingMode exits. Don't see why we could not simply call GetProfilingMode() and allow profiling when it is not kNone, as we do in InitializeIfEnabled(). Actually, it would be good to keep CanProfile and InitializeIfEnabled in sync.
JSProfilingMode mode = GetProfilingMode(execution_context);Do we need to change flow of this function? As CanProfile() has extra checks like for dedicated worker, it might be better to keep the overall code and just add GetProfilingMode() before calling OnProfilingContextAdded.
const profiler = new Profiler({I don't see difference between eager and lazy mode. Is there any way to show the difference?
Thomas Bertet (Gerrit)
Thomas Bertet added 4 comments![]( "Open in Gerrit")
if (execution_context->IsDedicatedWorkerGlobalScope() &&
!RuntimeEnabledFeatures::DocumentPolicyInDedicatedWorkerEnabled()) {
if (exception_state) {
exception_state->ThrowDOMException(
DOMExceptionCode::kNotSupportedError,
"Document Policy is not enabled for this context.");
}
return false;
}
Should we still keep this block of code?
Oh totally. Re-adding it, and making sure it's still used too in `InitializeIfEnabled`
// Re-fetch the raw policy value rather than calling GetProfilingMode() so we
// can distinguish "enum policy explicitly present" from "absent/legacy". If
// the enum policy is present we honour it exclusively (even kNone) and skip
// the legacy kJSProfiling check entirely. If it is absent we fall back toGetProfilingMode() also ignores legacy kJSProfiling when kJSProfilingMode exits. Don't see why we could not simply call GetProfilingMode() and allow profiling when it is not kNone, as we do in InitializeIfEnabled(). Actually, it would be good to keep CanProfile and InitializeIfEnabled in sync.
Acknowledged
JSProfilingMode mode = GetProfilingMode(execution_context);Do we need to change flow of this function? As CanProfile() has extra checks like for dedicated worker, it might be better to keep the overall code and just add GetProfilingMode() before calling OnProfilingContextAdded.
You are right, I re-added the DedicatedWorker check and used `CanProfile` in `InitializeIfEnabled` so that they are both using the same path. WDYT ?
const profiler = new Profiler({I don't see difference between eager and lazy mode. Is there any way to show the difference?
The tests are intentionally structured the same way — they verify that the mode is recognised by the browser and that the API is usable, which would fail if, e.g., initialisation were broken for that mode. The eager vs. lazy difference is covered at the unit test level in profiler_group_test.cc. Do you think we need to do something more here ?
Related details
- Liang Zhao
Liang Zhao (Gerrit)
Liang Zhao added 4 comments![]( "Open in Gerrit")
// Profiling is disabled. If the enum policy is absent, re-check the legacy
// kJSProfiling policy with report_options so violation reports are emitted.
PolicyValue mode_value = execution_context->GetDocumentPolicyValue(
mojom::blink::DocumentPolicyFeature::kJSProfilingMode);
if (mode_value.Type() != mojom::blink::PolicyValueType::kEnum) {
execution_context->IsFeatureEnabled(
mojom::blink::DocumentPolicyFeature::kJSProfiling, report_options);
}
Just a thought, if we add a ReportOptions report_options parameter to GetProfilingMode, we don't have to do this check here again. But it could feel strange if a Get mothod has side effects. So, unless we consider renaming it to something like CheckProfileMode, just a thought.
if (mode == JSProfilingMode::kNone) {
return;
}This is after CanProfile returning true. We probably should CHECK that mode is not kNone instead of siliently returning. As OnProfilingContextAdded already has a CHECK for mode, we could simply remove these.
// kLazyLogging) and that CreateProfiler succeeds without additional init.How is this verified and what's the observed differences between this test and CreateProfiler?
const profiler = new Profiler({Thomas BertetI don't see difference between eager and lazy mode. Is there any way to show the difference?
The tests are intentionally structured the same way — they verify that the mode is recognised by the browser and that the API is usable, which would fail if, e.g., initialisation were broken for that mode. The eager vs. lazy difference is covered at the unit test level in profiler_group_test.cc. Do you think we need to do something more here ?
Normally, we want to verify differences for different input. If there is no observable difference, using internal browser tests or unit tests are OK. Feel free to resolve this comment if it is not possible to observe the difference.
Related details
- Michal Mocny
- Monica Chintala
- Thomas BERTET
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. |
Thomas Bertet (Gerrit)
Thomas Bertet added 4 comments![]( "Open in Gerrit")
// Profiling is disabled. If the enum policy is absent, re-check the legacy
// kJSProfiling policy with report_options so violation reports are emitted.
PolicyValue mode_value = execution_context->GetDocumentPolicyValue(
mojom::blink::DocumentPolicyFeature::kJSProfilingMode);
if (mode_value.Type() != mojom::blink::PolicyValueType::kEnum) {
execution_context->IsFeatureEnabled(
mojom::blink::DocumentPolicyFeature::kJSProfiling, report_options);
}
Just a thought, if we add a ReportOptions report_options parameter to GetProfilingMode, we don't have to do this check here again. But it could feel strange if a Get mothod has side effects. So, unless we consider renaming it to something like CheckProfileMode, just a thought.
Thanks for the suggestion. The reason we can't simply pass `report_options` into `GetProfilingMode()` is - as you pointed out - that it would conflate two separate concerns: `mode` selection (used by `InitializeIfEnabled` at context init time, where no violation reporting is wanted) and authorization gating (used by `CanProfile` when the JS API is called).
Adding `report_options` with a default of `kDoNotReport` would work mechanically, but it would make `GetProfilingMode` responsible for a side effect that only makes sense in one of its two call sites. I think keeping the re-fetch in `CanProfile` is cleaner.
This is after CanProfile returning true. We probably should CHECK that mode is not kNone instead of siliently returning. As OnProfilingContextAdded already has a CHECK for mode, we could simply remove these.
Acknowledged
// kLazyLogging) and that CreateProfiler succeeds without additional init.How is this verified and what's the observed differences between this test and CreateProfiler?
You're right that this test doesn't actually verify the V8 logging mode — `kLazyLogging` vs `kEagerLogging` is not observable through the public v8::CpuProfiler API, so we can't assert it directly.
The test is a smoke test verifying that `OnProfilingContextAdded(kLazy)` initialises the profiler and that `CreateProfiler` succeeds on the lazy path. I've renamed it to `LazyModeAllowsProfiling` and updated the comment to make that scope explicit. Would that work for you ? Or do you think that's not needed maybe ?
const profiler = new Profiler({Thomas BertetI don't see difference between eager and lazy mode. Is there any way to show the difference?
Liang ZhaoThe tests are intentionally structured the same way — they verify that the mode is recognised by the browser and that the API is usable, which would fail if, e.g., initialisation were broken for that mode. The eager vs. lazy difference is covered at the unit test level in profiler_group_test.cc. Do you think we need to do something more here ?
Normally, we want to verify differences for different input. If there is no observable difference, using internal browser tests or unit tests are OK. Feel free to resolve this comment if it is not possible to observe the difference.
Do you mean we should remove the new `lazy` wpt test then since we can't really observe a difference ?
Related details
- Liang Zhao
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
// kLazyLogging) and that CreateProfiler succeeds without additional init.Thomas BertetHow is this verified and what's the observed differences between this test and CreateProfiler?
You're right that this test doesn't actually verify the V8 logging mode — `kLazyLogging` vs `kEagerLogging` is not observable through the public v8::CpuProfiler API, so we can't assert it directly.
The test is a smoke test verifying that `OnProfilingContextAdded(kLazy)` initialises the profiler and that `CreateProfiler` succeeds on the lazy path. I've renamed it to `LazyModeAllowsProfiling` and updated the comment to make that scope explicit. Would that work for you ? Or do you think that's not needed maybe ?
Code owner might have better suggestions. I think that we still want to have a test to verify that the handling of the lazy mode works from the observable behavior. If we really could not observe the difference between lazy and eager mode, a comment might be helpful.
const profiler = new Profiler({Thomas BertetI don't see difference between eager and lazy mode. Is there any way to show the difference?
Liang ZhaoThe tests are intentionally structured the same way — they verify that the mode is recognised by the browser and that the API is usable, which would fail if, e.g., initialisation were broken for that mode. The eager vs. lazy difference is covered at the unit test level in profiler_group_test.cc. Do you think we need to do something more here ?
Thomas BertetNormally, we want to verify differences for different input. If there is no observable difference, using internal browser tests or unit tests are OK. Feel free to resolve this comment if it is not possible to observe the difference.
Do you mean we should remove the new `lazy` wpt test then since we can't really observe a difference ?
I think that we still want the test to ensure that at least handling of the enum works.
Related details
- Michal Mocny
- Monica Chintala
- Thomas BERTET
- Thomas Bertet
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. |
Thomas Bertet (Gerrit)
Thomas Bertet added 3 comments![]( "Open in Gerrit")
context_observers_.insert(observer);Thomas BertetTHis will require expert review, but, it surpises me that you can init via `InitV8Profiler(v8::kLazyLogging);` but that we choose not to do so.
So you can have tri-state:
- Un-init
- Init w/ Lazy
- Init w/ Eager
This document policy adds 2 modes and picks (1) and (3).
(But it also creates a `ProfilingContextObserver` eagerly, which afaik isn't needed until you start to profile? Not sure.)
Right! It makes sense to me to simplify this. I've reworked this to get rid of the `OnProfilingContextAddedLazy` and initialize V8 with `kLazyLogging`
@mmo...@chromium.org do the changes look good to you ?
// kLazyLogging) and that CreateProfiler succeeds without additional init.Thomas BertetHow is this verified and what's the observed differences between this test and CreateProfiler?
Liang ZhaoYou're right that this test doesn't actually verify the V8 logging mode — `kLazyLogging` vs `kEagerLogging` is not observable through the public v8::CpuProfiler API, so we can't assert it directly.
The test is a smoke test verifying that `OnProfilingContextAdded(kLazy)` initialises the profiler and that `CreateProfiler` succeeds on the lazy path. I've renamed it to `LazyModeAllowsProfiling` and updated the comment to make that scope explicit. Would that work for you ? Or do you think that's not needed maybe ?
Code owner might have better suggestions. I think that we still want to have a test to verify that the handling of the lazy mode works from the observable behavior. If we really could not observe the difference between lazy and eager mode, a comment might be helpful.
@acom...@meta.com do you have a suggestion here ? :) Thank you!
const profiler = new Profiler({Thomas BertetI don't see difference between eager and lazy mode. Is there any way to show the difference?
Liang ZhaoThe tests are intentionally structured the same way — they verify that the mode is recognised by the browser and that the API is usable, which would fail if, e.g., initialisation were broken for that mode. The eager vs. lazy difference is covered at the unit test level in profiler_group_test.cc. Do you think we need to do something more here ?
Thomas BertetNormally, we want to verify differences for different input. If there is no observable difference, using internal browser tests or unit tests are OK. Feel free to resolve this comment if it is not possible to observe the difference.
Liang ZhaoDo you mean we should remove the new `lazy` wpt test then since we can't really observe a difference ?
I think that we still want the test to ensure that at least handling of the enum works.
- Liang Zhao
- Michal Mocny
- Monica Chintala
- Thomas BERTET
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. |
Thomas Bertet (Gerrit)
Thomas Bertet added 1 comment![]( "Open in Gerrit")
Hi there, I believe the CL looks reasonable now, if you could take a look as code owners, it would be great! Thank you !
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
Shadow: ari...@chromium.org; IPC: dch...@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/).
Shadow IPC reviewer(s): ari...@chromium.org. Please conduct an IPC review and CR+1 when satisfied. Remember to add the main reviewers to the attention set if needed.
Main IPC reviewer(s): dch...@chromium.org. Please wait for the shadowed IPC reviewer to CR+1 before reviewing.
Note: IPC gwsq added no new reviewers; existing reviewers satisfied requirements!
Reviewer source(s):
dch...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Alex Russell
- Annie Sullivan
- Ari Chivukula
- Daniel Cheng
- Daniel Vogelheim
- Dave Tapuska
- Ian Clelland
- Jeremy Roman
- Kenneth Russell
- Kent Tamura
- Kentaro Hara
- Liang Zhao
- Michael Lippautz
- Michal Mocny
- Monica Chintala
- Nate Chapin
- Nico Weber
- Nicolás Peña
- Philip Jägenstedt
- Rick Byers
- Rob Buis
- Sandor «Alex» Major
- Scott Haseley
- Stefan Zager
- Thomas BERTET
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. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
Please don't add lots of people as reviewers. Normally you'd pick one owner. Removing all the folks you added again.
I don't know how much "should we do this" was discussed. The bug mentions "v8 profiler", so I'm bmeurer from v8/src/profiler/OWNERS. If doing this is fine in principle, pick one blink owner to do the owners review.
Related details
- Benedikt Meurer
- Liang Zhao
- Michal Mocny
- Monica Chintala
- Thomas BERTET
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. |
Ari Chivukula (Gerrit)
Ari Chivukula added 12 comments![]( "Open in Gerrit")
(IPC shadow review) I don't have familiarity with whatever risks the profiler setting itself has, but the IPC here generally seems low risk, if a bit hard to follow due to the conversions between string <-> int <-> enum.
I almost wonder if we should require kEnum support on the PolicyFeature require mojom based enums so it can handle verification/conversion internally without the need for so much new code with each enum. If you do want to do that, it should probably be its own CL to reduce the complexity of this one
case 0: // JSProfilingMode::kNonesame nit here, except that the enum value should be used in the switch cases
return 0; // JSProfilingMode::kNoneWhy not have this enum be available for use here and return the std::to_underlying of the enum itself? That seems less error prone if this expands in future
DCHECK(document_policy_);I see we use DCHECK elsewhere in this file, but why not CHECK here?
default_value: "0",Thomas BertetThe `default_value` of `0` corresponds to `JSProfilingMode::kNone`. However, there isn't a corresponding token in `TokenToEnumValue` (line 29 in `document_policy_parser.cc`) or `EnumValueToToken` (line 40 in `document_policy.cc`) to represent `kNone`.
This means that while `kNone` is the default, it cannot be explicitly set via a Document Policy header. Only `eager` and `lazy` can be specified.
Is this intentional? If so, it might be worth adding a comment to clarify that `kNone` is an implicit default and cannot be set explicitly via the policy header, to avoid confusion if someone tries to write `js-profiling-mode='none'`.
I think I would find the `none` enum value an interesting idea, even though it's not in the specification update yet. I guess we can do that in a the meantime.
In the next patchset, I've added a `none` enum value, let me know if you feel that's good!
I think you're trending new ground here with supporting enums, so should we have the default_value be the string representation instead of the numerical one? The string representation is what's expected in the header right?
// mojom::blink::DocumentPolicyFeature is a type alias forthe comments here and elsewhere about not casting seem unnecessary
// kEnumFeature is aliased to kJSProfilingMode so that serialization round-tripsWhy not just use kJSProfilingMode directly? It seems worse for future expansion that we're making this the generic enum feature.
v8::CpuProfilingLoggingMode logging_mode = v8::kEagerLogging);same note here
JSProfilingMode mode = JSProfilingMode::kEager);if kNone is the default should it be used here as well? Or maybe don't have a default argument, that seems risky as it means future callers might forget it.
return static_cast<JSProfilingMode>(mode_value.IntValue());It feels risky to defer all bounds checking upstream, we might want a kMax value in JSProfilingMode so we can check it's between 0 and it
DCHECK_NE(mode, JSProfilingMode::kNone);CHECK_NE?
DCHECK_NE(mode, JSProfilingMode::kNone);CHECK_NE
Thomas Bertet (Gerrit)
Thomas Bertet added 14 comments![]( "Open in Gerrit")
(IPC shadow review) I don't have familiarity with whatever risks the profiler setting itself has, but the IPC here generally seems low risk, if a bit hard to follow due to the conversions between string <-> int <-> enum.
I almost wonder if we should require kEnum support on the PolicyFeature require mojom based enums so it can handle verification/conversion internally without the need for so much new code with each enum. If you do want to do that, it should probably be its own CL to reduce the complexity of this one
I'd be in favour of doing this improvement later, to avoid scope creep and more delays if that's ok with you ? 😊
same nit here, except that the enum value should be used in the switch cases
Acknowledged
return 0; // JSProfilingMode::kNoneWhy not have this enum be available for use here and return the std::to_underlying of the enum itself? That seems less error prone if this expands in future
Did it, and in the process I moved the JSProfilingMode to its own header file to avoid relying on /renderer file. I hope that's what you were thinking of. Happy to change if it was not!
I see we use DCHECK elsewhere in this file, but why not CHECK here?
Done
default_value: "0",Thomas BertetThe `default_value` of `0` corresponds to `JSProfilingMode::kNone`. However, there isn't a corresponding token in `TokenToEnumValue` (line 29 in `document_policy_parser.cc`) or `EnumValueToToken` (line 40 in `document_policy.cc`) to represent `kNone`.
This means that while `kNone` is the default, it cannot be explicitly set via a Document Policy header. Only `eager` and `lazy` can be specified.
Is this intentional? If so, it might be worth adding a comment to clarify that `kNone` is an implicit default and cannot be set explicitly via the policy header, to avoid confusion if someone tries to write `js-profiling-mode='none'`.
Ari ChivukulaI think I would find the `none` enum value an interesting idea, even though it's not in the specification update yet. I guess we can do that in a the meantime.
In the next patchset, I've added a `none` enum value, let me know if you feel that's good!
I think you're trending new ground here with supporting enums, so should we have the default_value be the string representation instead of the numerical one? The string representation is what's expected in the header right?
Yes it's the string that is expected, as per the spec update: https://github.com/WICG/js-self-profiling/pull/87. However, it seems this file is processed by https://chromium.googlesource.com/experimental/chromium/src/+/refs/tags/88.0.4281.0/third_party/blink/renderer/build/scripts/make_document_policy_features_util.py and only bool or Double are supported. Also looking at `PolicyValueType` https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/permissions_policy/policy_value.mojom I can see `kEnum` is supported but that's the integer representation ?
// mojom::blink::DocumentPolicyFeature is a type alias forthe comments here and elsewhere about not casting seem unnecessary
Done
// kEnumFeature is aliased to kJSProfilingMode so that serialization round-tripsWhy not just use kJSProfilingMode directly? It seems worse for future expansion that we're making this the generic enum feature.
Acknowledged
v8::CpuProfilingLoggingMode logging_mode = v8::kEagerLogging);Thomas Bertetsame note here
Done
if kNone is the default should it be used here as well? Or maybe don't have a default argument, that seems risky as it means future callers might forget it.
Removed the default as per your suggestion.
return static_cast<JSProfilingMode>(mode_value.IntValue());It feels risky to defer all bounds checking upstream, we might want a kMax value in JSProfilingMode so we can check it's between 0 and it
Done
return true;Thomas BertetIf `GetProfilingMode()` returns `kNone`, it means that `IsFeatureEnabled(kJSProfiling)` has already returned `false`. Therefore, the `IsFeatureEnabled` check on line 102 will also be `false`, making this `return true;` unreachable.
Additionally, my understanding is that `IsFeatureEnabled` do some logging/reporting and the GetProfilingMode would skip its check if the new profiling mode was supplied without the general profiling policy.
---
If kJSProfiling is still required alongside kJSProfilingMode, I think you could swap the order of feature detection and if kJSProfiling is disabled always return kNone, otherwise return provided policy or default to eager.
If kJSProfiling is optional... maybe the current implementation is fine, but perhaps add a comment here?
I've reworked the logic to avoid unreachable return. Let me know what you think!
Done
DCHECK_NE(mode, JSProfilingMode::kNone);Thomas BertetCHECK_NE?
Done
DCHECK_NE(mode, JSProfilingMode::kNone);Thomas BertetCHECK_NE
Done
context_observers_.insert(observer);Thomas BertetTHis will require expert review, but, it surpises me that you can init via `InitV8Profiler(v8::kLazyLogging);` but that we choose not to do so.
So you can have tri-state:
- Un-init
- Init w/ Lazy
- Init w/ Eager
This document policy adds 2 modes and picks (1) and (3).
(But it also creates a `ProfilingContextObserver` eagerly, which afaik isn't needed until you start to profile? Not sure.)
Thomas BertetRight! It makes sense to me to simplify this. I've reworked this to get rid of the `OnProfilingContextAddedLazy` and initialize V8 with `kLazyLogging`
@mmo...@chromium.org do the changes look good to you ?
- Ari Chivukula
- Liang Zhao
- Michal Mocny
- Monica Chintala
- Thomas BERTET
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. |
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +1 |
1 comment
(IPC shadow) I'm a little worried about how extendable the new enum policy code is (or isn't) to additional enums, but that's not really a security concern
Related details
- Liang Zhao
- Michal Mocny
- Monica Chintala
- Thomas BERTET
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. |
Thomas Bertet (Gerrit)
Thomas Bertet added 1 comment![]( "Open in Gerrit")
Please don't add lots of people as reviewers. Normally you'd pick one owner. Removing all the folks you added again.
I don't know how much "should we do this" was discussed. The bug mentions "v8 profiler", so I'm bmeurer from v8/src/profiler/OWNERS. If doing this is fine in principle, pick one blink owner to do the owners review.
Hi @tha...@chromium.org, thank you for fixing up the reviewers and sorry for the mess! I must have missed where this guideline was written 😊 . About "should we do this", it has been discussed during a WebPerf WG Call and then in GH. Everyone involved seemed to be onboard with the idea and the Spec was validated and updated. Please feel free to look at the GH issue for more info. Also happy to jump on a call if need be! Also it seems @bme...@chromium.org suggested @szu...@chromium.org who in turn removed himself as reviewer 😞
Anyone else I can borrow time from to review the blink's part ? :) Thank you!
Related details
- Liang Zhao
- Michal Mocny
- Monica Chintala
- Nico Weber
- Thomas BERTET
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 5 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
5 comments
It appears like the vast majority of this patch is the new document policy integration and plumbing. At the end of the day, there is a single line of change for the JS self profiling-- passing a `mode` to `v8::CpuProfiler::New`.
LGTM (with some unresolved comments) deferring to the local owners (esp for document policy) to confirm that looks good.
case mojom::PolicyValueType::kEnum: {`DocumentPolicyEnumValueToToken` expects only `feature` that implement enum mappings to be called, otherwise we trigger NOTREACHED.
Is it possible for a developer to accidentally return a `kEnum` policy value type that isn't for the `kJSProfilingMode` feature, or do we guard against that earlier before the call to `PolicyValueToItem`?
---
And if it is possible could you add tests to verify that someone setting the wrong Policy headers for an unexpected feature type trying to use an enum value, would not crash renderer? (It may literally be impossible to get an enum type for a non-enum supporting feature, I dont know).
std::optional<int32_t> TokenToEnumValue(const std::string& token) {Monica ChintalaHmm, we go from enum string to enum value back to enum string later... Ideally these to_string and from_string would be defined together, indeally in `profiler_group.h`?
(Also this implementation has the same issue as the other one in that its generic for all enums but specific to this particular enum.)
---
WDYT about adding a second enum directly to `third_party/blink/public/mojom/permissions_policy/document_policy_feature.mojom`?
And then use `document_policy_features.json5`?
...that might need some build file pre-processing changes to support enum *values*, but I think this feels like the correct approach.
(I'm totally new to this area, but this is what I found with a bit of digging.)
Thomas BertetAgree, If the enum values ever change, or another enum type is added, the parser/serializer silently break with no compiler error and enum should be in public files
Michal MocnyI've tentatively updated the code to follow your recommendation. Let me know what you think!
Acknowledged
{"f-enum", mojom::blink::DocumentPolicyFeature::kJSProfilingMode},Nit: could this define a `kEnumFeature` for testing (above) instead of using this specific one? Or would you need more plumbing to define a new enum type?
// Profiling is disabled. If the enum policy is absent, re-check the legacy
// kJSProfiling policy with report_options so violation reports are emitted.Nit: Would this be cleaner if `GetProfilingMode` just accepted `report_options` perhaps?
Related details
- Liang Zhao
- Monica Chintala
- Nico Weber
- Thomas BERTET
- Thomas Bertet
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Thomas Bertet (Gerrit)
Thomas Bertet added 5 comments![]( "Open in Gerrit")
(IPC shadow review) I don't have familiarity with whatever risks the profiler setting itself has, but the IPC here generally seems low risk, if a bit hard to follow due to the conversions between string <-> int <-> enum.
I almost wonder if we should require kEnum support on the PolicyFeature require mojom based enums so it can handle verification/conversion internally without the need for so much new code with each enum. If you do want to do that, it should probably be its own CL to reduce the complexity of this one
I'd be in favour of doing this improvement later, to avoid scope creep and more delays if that's ok with you ? 😊
Acknowledged
case mojom::PolicyValueType::kEnum: {`DocumentPolicyEnumValueToToken` expects only `feature` that implement enum mappings to be called, otherwise we trigger NOTREACHED.
Is it possible for a developer to accidentally return a `kEnum` policy value type that isn't for the `kJSProfilingMode` feature, or do we guard against that earlier before the call to `PolicyValueToItem`?
---
And if it is possible could you add tests to verify that someone setting the wrong Policy headers for an unexpected feature type trying to use an enum value, would not crash renderer? (It may literally be impossible to get an enum type for a non-enum supporting feature, I dont know).
1 - We don't prevent developers to return a `kEnum` type for something else than the `kJSProfilingMode`, as long as the feature is registered in the feature map as `Enum`. If it's not, then it's dismissed earlier by the `ParseFeature` method.
2 - When a wrong value of the enum is set, the `DocumentPolicyEnumValueToToken` will clean it up without crashing.
3 - When developer set a new feature as type `Enum`, without specifying list of values nor updating `DocumentPolicyEnumValueToToken`, then the `NOTREACHED()` will be called. I'm adding a test case to catch that, making sure devs won't forget to add their enum values into `DocumentPolicyEnumValueToToken`
return 0; // JSProfilingMode::kNoneThomas BertetWhy not have this enum be available for use here and return the std::to_underlying of the enum itself? That seems less error prone if this expands in future
Did it, and in the process I moved the JSProfilingMode to its own header file to avoid relying on /renderer file. I hope that's what you were thinking of. Happy to change if it was not!
Acknowledged
{"f-enum", mojom::blink::DocumentPolicyFeature::kJSProfilingMode},Nit: could this define a `kEnumFeature` for testing (above) instead of using this specific one? Or would you need more plumbing to define a new enum type?
Done
// Profiling is disabled. If the enum policy is absent, re-check the legacy
// kJSProfiling policy with report_options so violation reports are emitted.Nit: Would this be cleaner if `GetProfilingMode` just accepted `report_options` perhaps?
- Ari Chivukula
- Liang Zhao
- Michal Mocny
- Monica Chintala
- Nico Weber
- Thomas BERTET
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. |
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +1 |
Michal Mocny (Gerrit)
Michal Mocny voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
2 comments
Still LGTM, thanks for the extra test and various improvements!
case mojom::PolicyValueType::kEnum: {Thomas Bertet`DocumentPolicyEnumValueToToken` expects only `feature` that implement enum mappings to be called, otherwise we trigger NOTREACHED.
Is it possible for a developer to accidentally return a `kEnum` policy value type that isn't for the `kJSProfilingMode` feature, or do we guard against that earlier before the call to `PolicyValueToItem`?
---
And if it is possible could you add tests to verify that someone setting the wrong Policy headers for an unexpected feature type trying to use an enum value, would not crash renderer? (It may literally be impossible to get an enum type for a non-enum supporting feature, I dont know).
1 - We don't prevent developers to return a `kEnum` type for something else than the `kJSProfilingMode`, as long as the feature is registered in the feature map as `Enum`. If it's not, then it's dismissed earlier by the `ParseFeature` method.
2 - When a wrong value of the enum is set, the `DocumentPolicyEnumValueToToken` will clean it up without crashing.
3 - When developer set a new feature as type `Enum`, without specifying list of values nor updating `DocumentPolicyEnumValueToToken`, then the `NOTREACHED()` will be called. I'm adding a test case to catch that, making sure devs won't forget to add their enum values into `DocumentPolicyEnumValueToToken`
- Liang Zhao
- Monica Chintala
- Nico Weber
- Thomas BERTET
- Thomas Bertet
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Nico Weber (Gerrit)
Nico Weber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nico Weber
| Code-Review | +1 |
1 comment
Remaining blink files lg, given mmocny's review.
You probably still need a security reviewer for that one .mojom file.
Related details
- Liang Zhao
- Monica Chintala
- Thomas BERTET
- Thomas Bertet
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. |
Thomas Bertet (Gerrit)
Thomas Bertet added 1 comment![]( "Open in Gerrit")
Would anyone from the Chromium IPC team be able to take a look at the one remaining file? Really appreciate it!
Related details
- Chromium IPC Reviews
- Liang Zhao
- Monica Chintala
- Thomas BERTET
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
Shadow: ari...@chromium.org; IPC: ke...@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/).
Shadow IPC reviewer(s): ari...@chromium.org. Please conduct an IPC review and CR+1 when satisfied. Remember to add the main reviewers to the attention set if needed.
Main IPC reviewer(s): ke...@chromium.org. Please wait for the shadowed IPC reviewer to CR+1 before reviewing.
Shadowed: ari...@chromium.org
Reviewer source(s):
ari...@chromium.org, ke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Ken Buchanan
- Liang Zhao
- Monica Chintala
- Thomas BERTET
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. |
Ken Buchanan (Gerrit)
Ken Buchanan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ken Buchanan
| Code-Review | +1 |
1 comment
- Liang Zhao
- Monica Chintala
- Thomas BERTET
Code-Coverage
Code-Owners
Thomas Bertet (Gerrit)
Thomas Bertet added 1 comment![]( "Open in Gerrit")
The legacy `js-profiling` header tests are failing. With all the changes we did along the way, we lost the backward compatibility 😭. (I should have ran the WPT tests before, my bad)
The DocumentPolicy framework initialises every registered feature with a default value, regardless of whether the response header mentions it.
For boolean features this is mostly harmless — "not set" and "disabled by default" are the same thing.
For `js-profiling-mode`, they are not: "not set" should fall back to the legacy `js-profiling` boolean policy, while `js-profiling-mode=none` should explicitly block profiling even when `js-profiling` is also set.
Because the framework merges header values over defaults into a flat map (and defaults is `kNone`), `GetDocumentPolicyValue` cannot tell them apart.
Potential solutions:
We could use a sentinel value (3, outside the valid token range) as a workaround: the parser can only produce 0/1/2 for known tokens, so value 3 can only come from the default, allowing `GetProfilingMode` to treat it as "not explicitly set" and fall through to the legacy path. It may not be very elegant though.
A more idiomatic fix would be to either extend the framework to track which features were explicitly present in the header, or drop the `none` token entirely since it's the only reason the distinction matters.
I'm leaning towards removing the `none` token since it was not really part of the initial spec update anyway, and is the reason we have this "default vs. not-set" confusion. I'll update the CL do remove `kNone` but in the meantime, if you disagree, please let me know!
Related details
- Liang Zhao
- Monica Chintala
- Nico Weber
- Thomas BERTET
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. |
Thomas Bertet (Gerrit)
Thomas Bertet added 1 comment![]( "Open in Gerrit")
default_value: "0",The `default_value` of `0` corresponds to `JSProfilingMode::kNone`. However, there isn't a corresponding token in `TokenToEnumValue` (line 29 in `document_policy_parser.cc`) or `EnumValueToToken` (line 40 in `document_policy.cc`) to represent `kNone`.
This means that while `kNone` is the default, it cannot be explicitly set via a Document Policy header. Only `eager` and `lazy` can be specified.
Is this intentional? If so, it might be worth adding a comment to clarify that `kNone` is an implicit default and cannot be set explicitly via the policy header, to avoid confusion if someone tries to write `js-profiling-mode='none'`.
Ari ChivukulaI think I would find the `none` enum value an interesting idea, even though it's not in the specification update yet. I guess we can do that in a the meantime.
In the next patchset, I've added a `none` enum value, let me know if you feel that's good!
Thomas BertetI think you're trending new ground here with supporting enums, so should we have the default_value be the string representation instead of the numerical one? The string representation is what's expected in the header right?
Yes it's the string that is expected, as per the spec update: https://github.com/WICG/js-self-profiling/pull/87. However, it seems this file is processed by https://chromium.googlesource.com/experimental/chromium/src/+/refs/tags/88.0.4281.0/third_party/blink/renderer/build/scripts/make_document_policy_features_util.py and only bool or Double are supported. Also looking at `PolicyValueType` https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/permissions_policy/policy_value.mojom I can see `kEnum` is supported but that's the integer representation ?
Patchset 10 is now removing the `kNone` token, so that, as you suggested Michal, there is a comment explaining 0 is the default, but do not translate to any value enum value and is representing "not set"
Related details
- Ari Chivukula
- Ken Buchanan
- Liang Zhao
- Michal Mocny
- Monica Chintala
- Nico Weber
- Thomas BERTET
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. |
Ken Buchanan (Gerrit)
Ken Buchanan voted Code-Review+1![]( "Open in Gerrit")
- Ari Chivukula
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +1 |
3 comments
inline std::optional<int32_t> DocumentPolicyEnumTokenToValue(mind outlining in a README (maybe in this folder) the spots that need to be updated to support new enum values?
// valid c++ expression strings, e.g. true/false, 1.0, -1.maybe a note here that enum values are underlying type not named constants
Related details
- Liang Zhao
- Michal Mocny
- Monica Chintala
- Nico Weber
- Thomas BERTET
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement