[tracing] Remove json allowlist [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Related details
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted Code-Review+1![]( "Open in Gerrit")
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
- Nate Fischer
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Fischer (Gerrit)
Nate Fischer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nate Fischer
| Code-Review | +1 |
1 comment
// as is the case by default in aw_tracing_controller.ccDoes this require changes to aw_tracing_controller.cc? Is this related to https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/tracing/aw_tracing_controller.cc;l=88;drc=1df1c1323290afd34f4e594be0f54beb2ee5968d or is this referring to a different filter?
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
// as is the case by default in aw_tracing_controller.ccDoes this require changes to aw_tracing_controller.cc? Is this related to https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/tracing/aw_tracing_controller.cc;l=88;drc=1df1c1323290afd34f4e594be0f54beb2ee5968d or is this referring to a different filter?
So apparently TracingController is a public API on webview:
https://developer.android.com/reference/android/webkit/TracingController#start(android.webkit.TracingConfig)
This is a bit silly though, because the allowlist as-is only allows collecting memory-infra (heap dumps).
Removing this would expose a lot more things - I don't know if it makes sense to do so, but the current allowlist doesn't make a lot of sense either. Thoughts?
Related details
- Nate Fischer
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. |
Nate Fischer (Gerrit)
Nate Fischer added 1 comment![]( "Open in Gerrit")
// as is the case by default in aw_tracing_controller.ccEtienne Pierre-DorayDoes this require changes to aw_tracing_controller.cc? Is this related to https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/tracing/aw_tracing_controller.cc;l=88;drc=1df1c1323290afd34f4e594be0f54beb2ee5968d or is this referring to a different filter?
So apparently TracingController is a public API on webview:
https://developer.android.com/reference/android/webkit/TracingController#start(android.webkit.TracingConfig)
This is a bit silly though, because the allowlist as-is only allows collecting memory-infra (heap dumps).
Removing this would expose a lot more things - I don't know if it makes sense to do so, but the current allowlist doesn't make a lot of sense either. Thoughts?
We discussed offline, but is it possible to separate out the chrome change from the WebView change? If so, then I'd like to see if we can put the WebView behavior behind a base::Feature flag to allow us a more cautious rollout.
Related details
- Etienne Pierre-Doray
- Mikhail Khokhlov
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
// as is the case by default in aw_tracing_controller.ccEtienne Pierre-DorayDoes this require changes to aw_tracing_controller.cc? Is this related to https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/tracing/aw_tracing_controller.cc;l=88;drc=1df1c1323290afd34f4e594be0f54beb2ee5968d or is this referring to a different filter?
Nate FischerSo apparently TracingController is a public API on webview:
https://developer.android.com/reference/android/webkit/TracingController#start(android.webkit.TracingConfig)
This is a bit silly though, because the allowlist as-is only allows collecting memory-infra (heap dumps).
Removing this would expose a lot more things - I don't know if it makes sense to do so, but the current allowlist doesn't make a lot of sense either. Thoughts?
We discussed offline, but is it possible to separate out the chrome change from the WebView change? If so, then I'd like to see if we can put the WebView behavior behind a base::Feature flag to allow us a more cautious rollout.
This is a bit silly though, because the allowlist as-is only allows collecting memory-infra (heap dumps).
This wasn't quite true, trace events are still emitted, but with no arguments.
Still, doing this will now expose all protos that are privacy reviewed - the same one could get through devtools API.
I added Android.WebView.ApiCall.TracingController which shows this API is mostly unused - O(10) API calls - probably only for local debuging.
Related details
- Mikhail Khokhlov
- Nate Fischer
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. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted Code-Review+1![]( "Open in Gerrit")
- Etienne Pierre-Doray
- Nate Fischer
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. |
Nate Fischer (Gerrit)
Nate Fischer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nate Fischer
| Code-Review | +1 |
1 comment
// as is the case by default in aw_tracing_controller.ccEtienne Pierre-DorayDoes this require changes to aw_tracing_controller.cc? Is this related to https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/tracing/aw_tracing_controller.cc;l=88;drc=1df1c1323290afd34f4e594be0f54beb2ee5968d or is this referring to a different filter?
Nate FischerSo apparently TracingController is a public API on webview:
https://developer.android.com/reference/android/webkit/TracingController#start(android.webkit.TracingConfig)
This is a bit silly though, because the allowlist as-is only allows collecting memory-infra (heap dumps).
Removing this would expose a lot more things - I don't know if it makes sense to do so, but the current allowlist doesn't make a lot of sense either. Thoughts?
Etienne Pierre-DorayWe discussed offline, but is it possible to separate out the chrome change from the WebView change? If so, then I'd like to see if we can put the WebView behavior behind a base::Feature flag to allow us a more cautious rollout.
This is a bit silly though, because the allowlist as-is only allows collecting memory-infra (heap dumps).
This wasn't quite true, trace events are still emitted, but with no arguments.
Still, doing this will now expose all protos that are privacy reviewed - the same one could get through devtools API.
I added Android.WebView.ApiCall.TracingController which shows this API is mostly unused - O(10) API calls - probably only for local debuging.
Acknowledged
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
[tracing] Remove json allowlist
This CL removes json args allow list.
The allowlist is unnecessary because privacy filters
are now always run before JSON conversion when requested.
- M android_webview/browser/BUILD.gn
- D android_webview/browser/tracing/aw_trace_event_args_allowlist.cc
- D android_webview/browser/tracing/aw_trace_event_args_allowlist.h
- M android_webview/lib/aw_main_delegate.cc
- M base/trace_event/trace_log.cc
- M base/trace_event/trace_log.h
- M services/tracing/perfetto/consumer_host.cc
- M services/tracing/perfetto/consumer_host_unittest.cc
- M services/tracing/public/cpp/BUILD.gn
- D services/tracing/public/cpp/trace_event_args_allowlist.cc
- D services/tracing/public/cpp/trace_event_args_allowlist.h
- M services/tracing/public/cpp/trace_startup.cc
Code-Review: +1 by Nate Fischer, +1 by Mikhail Khokhlov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |