[tracing] Support regular privacy filter on json traces [chromium/src : main]
0 views
Skip to first unread message
Etienne Pierre-Doray (Gerrit)
Feb 10, 2026, 5:46:48 PMFeb 10
to Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Mikhail Khokhlov
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Mikhail Khokhlov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Feb 11, 2026, 6:08:43 AMFeb 11
to Etienne Pierre-Doray, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Etienne Pierre-Doray
Mikhail Khokhlov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mikhail Khokhlov
| Code-Review | +1 |
3 comments
Patchset-level comments
Mikhail Khokhlov . resolved
In general, LGTM.
Should we add an end-to-end test for privacy filtering + json conversion?
File components/heap_profiling/multi_process/test_driver.cc
Line 662, Patchset 5 (Latest):
/* anonymize= */ false);Mikhail Khokhlov . unresolved
Why is this change necessary? Does this test break with anonymize=true?
File services/tracing/perfetto/consumer_host.cc
Line 91, Patchset 5 (Latest):
using Slice = std::string;Mikhail Khokhlov . unresolved
nit: perhaps we can remove this alias now, since we don't make distinction between slices/other strings in the code anyway?
Related details
Attention is currently required from:
- Etienne Pierre-Doray
Submit Requirements:
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)
Feb 13, 2026, 2:53:47 PMFeb 13
to Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Mikhail Khokhlov
Etienne Pierre-Doray added 4 comments![]( "Open in Gerrit")
Patchset-level comments
Mikhail Khokhlov . resolved
In general, LGTM.
Should we add an end-to-end test for privacy filtering + json conversion?
Etienne Pierre-Doray
Done for end-to-end test.
File components/heap_profiling/multi_process/test_driver.cc
Line 662, Patchset 5:
/* anonymize= */ false);Mikhail Khokhlov . unresolved
Why is this change necessary? Does this test break with anonymize=true?
Etienne Pierre-Doray
Yes and no.
Calling RequestTraceWithHeapDump(/* anonymize= */ true) does break org.chromium.android_webview.test.HeapProfilingTest#testModeBrowserDynamicPseudo because heap profiling data is filtered out.
RequestTraceWithHeapDump() is always called with `anonymize = false` except in this test though, so it's weird to test it that way.
File services/tracing/perfetto/consumer_host.cc
nit: perhaps we can remove this alias now, since we don't make distinction between slices/other strings in the code anyway?
Attention is currently required from:
- Mikhail Khokhlov
Submit Requirements:
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)
Feb 16, 2026, 6:36:40 AM (12 days ago) Feb 16
to Etienne Pierre-Doray, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Etienne Pierre-Doray
Mikhail Khokhlov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mikhail Khokhlov
| Code-Review | +1 |
3 comments
Patchset-level comments
File components/heap_profiling/multi_process/test_driver.cc
Line 662, Patchset 5:
/* anonymize= */ false);Mikhail Khokhlov . resolved
Etienne Pierre-DorayWhy is this change necessary? Does this test break with anonymize=true?
Yes and no.
Calling RequestTraceWithHeapDump(/* anonymize= */ true) does break org.chromium.android_webview.test.HeapProfilingTest#testModeBrowserDynamicPseudo because heap profiling data is filtered out.RequestTraceWithHeapDump() is always called with `anonymize = false` except in this test though, so it's weird to test it that way.
Mikhail Khokhlov
Acknowledged
File content/browser/tracing/tracing_end_to_end_browsertest.cc
Line 1001, Patchset 6 (Latest):
IN_PROC_BROWSER_TEST_F(SystemTracingEndToEndBrowserTest,Mikhail Khokhlov . unresolved
nit: Since it's using the custom backend, and not system tracing, it should go into `TracingEndToEndBrowserTest` rather than `SystemTracingEndToEndBrowserTest`.
Related details
Attention is currently required from:
- Etienne Pierre-Doray
Submit Requirements:
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)
Feb 16, 2026, 6:56:54 AM (12 days ago) Feb 16
to Francois Pierre Doray, Dave Tapuska, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Dave Tapuska, Francois Pierre Doray and Joe Mason
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 6:
Etienne Pierre-Doray . resolved
+fdoray@ for base/test
+joenotcharles@ for components/heap_profiling
+dtapuska@ for content/public
File content/browser/tracing/tracing_end_to_end_browsertest.cc
Line 1001, Patchset 6:
IN_PROC_BROWSER_TEST_F(SystemTracingEndToEndBrowserTest,Mikhail Khokhlov . resolved
nit: Since it's using the custom backend, and not system tracing, it should go into `TracingEndToEndBrowserTest` rather than `SystemTracingEndToEndBrowserTest`.
Attention is currently required from:
- Dave Tapuska
- Francois Pierre Doray
- Joe Mason
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Feb 16, 2026, 10:17:16 AM (12 days ago) Feb 16
to Etienne Pierre-Doray, Dave Tapuska, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Dave Tapuska, Etienne Pierre-Doray and Joe Mason
Francois Pierre Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Francois Pierre Doray
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Dave Tapuska
- Etienne Pierre-Doray
- Joe Mason
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Feb 17, 2026, 9:56:39 AM (11 days ago) Feb 17
to Etienne Pierre-Doray, Francois Pierre Doray, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Etienne Pierre-Doray and Joe Mason
Dave Tapuska added 1 comment![]( "Open in Gerrit")
File content/browser/tracing/tracing_controller_impl.h
Line 67, Patchset 7 (Latest):
bool privacy_filtering_enabled = false) override;Dave Tapuska . unresolved
I realize this is moving a default initialized virtual over, but this is banned in the Google C++ coding guide. See https://google.github.io/styleguide/cppguide.html#Default_Arguments
We should define StartTracing as an inline call in TracingController that invokes the virtual which has two args (with no default).
Related details
Attention is currently required from:
- Etienne Pierre-Doray
- Joe Mason
Submit Requirements:
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)
Feb 17, 2026, 2:13:13 PM (10 days ago) Feb 17
to Keren Zhu, Nate Fischer, Francois Pierre Doray, Dave Tapuska, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Dave Tapuska, Joe Mason, Keren Zhu and Nate Fischer
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 8 (Latest):
Etienne Pierre-Doray . resolved
PTAnL
+kerenzhu@ for components/ui_devtools
+ntfschr@ for android_webview/
File content/browser/tracing/tracing_controller_impl.h
I realize this is moving a default initialized virtual over, but this is banned in the Google C++ coding guide. See https://google.github.io/styleguide/cppguide.html#Default_Arguments
We should define StartTracing as an inline call in TracingController that invokes the virtual which has two args (with no default).
Attention is currently required from:
- Dave Tapuska
- Joe Mason
- Keren Zhu
- Nate Fischer
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Feb 17, 2026, 2:14:47 PM (10 days ago) Feb 17
to Etienne Pierre-Doray, Nate Fischer, Francois Pierre Doray, Dave Tapuska, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Dave Tapuska, Joe Mason and Nate Fischer
Keren Zhu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Keren Zhu
| Code-Review | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Dave Tapuska
- Joe Mason
- Nate Fischer
Dave Tapuska (Gerrit)
Feb 17, 2026, 2:15:25 PM (10 days ago) Feb 17
to Etienne Pierre-Doray, Keren Zhu, Nate Fischer, Francois Pierre Doray, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Etienne Pierre-Doray, Joe Mason and Nate Fischer
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Etienne Pierre-Doray
- Joe Mason
- Nate Fischer
Joe Mason (Gerrit)
Feb 17, 2026, 4:27:42 PM (10 days ago) Feb 17
to Etienne Pierre-Doray, Dave Tapuska, Keren Zhu, Nate Fischer, Francois Pierre Doray, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Etienne Pierre-Doray and Nate Fischer
Joe Mason voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
1 comment
File content/browser/tracing/tracing_end_to_end_browsertest.cc
Line 892, Patchset 9 (Latest):
session->SetOnStartCallback([&run_loop] { run_loop.QuitWhenIdle(); });Joe Mason . unresolved
Nit: this could just be `SetOnStartCallback(run_loop.QuitWhenIdleClosure())`
Related details
Attention is currently required from:
- Etienne Pierre-Doray
- Nate Fischer
Submit Requirements:
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)
Feb 17, 2026, 5:57:20 PM (10 days ago) Feb 17
to Dave Tapuska, Keren Zhu, Nate Fischer, Francois Pierre Doray, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Nate Fischer
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
File content/browser/tracing/tracing_end_to_end_browsertest.cc
Line 892, Patchset 9:
session->SetOnStartCallback([&run_loop] { run_loop.QuitWhenIdle(); });Joe Mason . resolved
Nit: this could just be `SetOnStartCallback(run_loop.QuitWhenIdleClosure())`
Etienne Pierre-Doray
Apparently no because SetOnStartCallback takes a std::function (perfetto API)
Related details
Attention is currently required from:
- Nate Fischer
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Fischer (Gerrit)
Feb 18, 2026, 5:37:49 PM (9 days ago) Feb 18
to Etienne Pierre-Doray, Dave Tapuska, Keren Zhu, Francois Pierre Doray, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Etienne Pierre-Doray
Nate Fischer voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Etienne Pierre-Doray
Submit Requirements:
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)
Feb 18, 2026, 5:49:31 PM (9 days ago) Feb 18
to Nate Fischer, Dave Tapuska, Keren Zhu, Francois Pierre Doray, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
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)
Feb 18, 2026, 5:53:40 PM (9 days ago) Feb 18
to Etienne Pierre-Doray, Nate Fischer, Dave Tapuska, Keren Zhu, Francois Pierre Doray, Mikhail Khokhlov, chromium...@chromium.org, devtools...@chromium.org, android-web...@chromium.org, ipc-securi...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[tracing] Support regular privacy filter on json traces
This CL runs privacy filters before JSON conversion. This makes
legacy allow_list redundant, which is removed in follow-up.
Change-Id: I63f503e7e270779ce07d4317da754bfcf4b1cb51
Reviewed-by: Francois Pierre Doray <fdo...@chromium.org>
Reviewed-by: Dave Tapuska <dtap...@chromium.org>
Reviewed-by: Joe Mason <joenot...@google.com>
Commit-Queue: Etienne Pierre-Doray <etie...@chromium.org>
Reviewed-by: Keren Zhu <kere...@chromium.org>
Reviewed-by: Mikhail Khokhlov <khok...@google.com>
Reviewed-by: Nate Fischer <ntf...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1586713}
Files:
- M android_webview/browser/tracing/aw_tracing_controller.cc
- M base/test/test_trace_processor.cc
- M base/test/test_trace_processor.h
- M components/heap_profiling/multi_process/supervisor.cc
- M components/heap_profiling/multi_process/test_driver.cc
- M components/ui_devtools/tracing_agent.cc
- M content/browser/tracing/tracing_controller_browsertest.cc
- M content/browser/tracing/tracing_controller_impl.cc
- M content/browser/tracing/tracing_controller_impl.h
- M content/browser/tracing/tracing_end_to_end_browsertest.cc
- M content/public/browser/tracing_controller.h
- M services/tracing/perfetto/consumer_host.cc
- M services/tracing/perfetto/consumer_host.h
- M services/tracing/perfetto/consumer_host_unittest.cc
- M services/tracing/public/cpp/perfetto/perfetto_tracing_backend.cc
- M services/tracing/public/mojom/perfetto_service.mojom
Change size: M
Delta: 16 files changed, 114 insertions(+), 127 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Keren Zhu, +1 by Dave Tapuska, +1 by Francois Pierre Doray, +1 by Nate Fischer, +1 by Mikhail Khokhlov, +1 by Joe Mason
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages