Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400163003:
arc: enable Android tracing in verified-boot mode (Closed)
|
Created:
4 years, 2 months ago by shunhsingou Modified:
3 years, 7 months ago Reviewers:
Primiano Tucci (use gerrit), drinkcat1, oystein (OOO til 10th of July), Luis Héctor Chávez, dcheng, Nat, dsinclair, bccheng, achuithb, Yusuke Sato, clamy, Earl Ou CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, davemoore+watch_chromium.org, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, jam, lhchavez+watch_chromium.org, oshima+watch_chromium.org, qsr+mojo_chromium.org, tracing+reviews_chromium.org, viettrungluu+watch_chromium.org, wfh+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: enable Android tracing in verified-boot mode
In verified-boot mode, ftrace trace_marker is not allowed for writting
due to security concern. As an altenative, we use a socket for processes in
Android to write trace events, and send back the trace data via Mojo
handle.
All change set includes:
- http://crrev.com/2400163003 (Chromium)
- http://ag/2028075 (Android system/core)
- http://ag/1738271 (Android device/google/cheets2)
BUG=653795
TEST=Run chrome://tracing and see events from Android.
Review-Url: https://codereview.chromium.org/2400163003
Cr-Commit-Position: refs/heads/master@{#468306}
Committed: https://chromium.googlesource.com/chromium/src/+/7092984694bd56fc05488582a93a0fda0c8d7f29
Patch Set 1 #Patch Set 2 : arc: enable Android framework tracing in chrome://tracing #Patch Set 3 : arc: enable Android framework tracing in chrome://tracing #Patch Set 4 : arc: enable Android framework tracing in chrome://tracing #
Total comments: 28
Patch Set 5 : arc: enable Android framework tracing in chrome://tracing #Patch Set 6 : arc: enable Android framework tracing in chrome://tracing #
Total comments: 4
Patch Set 7 : Use struct instead of pair to save category data and fix lints #
Total comments: 64
Patch Set 8 : Create another pipe instead of ftrace for security #Patch Set 9 : Fix according to review comments #
Total comments: 58
Patch Set 10 : Fix according to reviewer comment and git cl lint #Patch Set 11 : add FakeArcTraceAgent for linux and test #
Total comments: 26
Patch Set 12 : Fix according to comments #Patch Set 13 : Fix according to comments #
Total comments: 35
Patch Set 14 : Move pipe read-end from debugd to ArcTraceService #Patch Set 15 : Fix some nits #
Total comments: 31
Patch Set 16 : Fix according to comments #Patch Set 17 : fix nit #
Total comments: 6
Patch Set 18 : Fix nit according to the comments #Patch Set 19 : Pass a socket handle for tracing and move buffer to the host side #Patch Set 20 : Fix some nits #
Total comments: 38
Patch Set 21 : Split trigger tracing part into another patch #Patch Set 22 : Rebase to crrev.com/2699833003 as it is merged #
Total comments: 32
Patch Set 23 : Address reviewer comments #Patch Set 24 : address reviewer comments #Patch Set 25 : Update comments #
Total comments: 26
Patch Set 26 : Address comments #
Total comments: 10
Patch Set 27 : Address comments #
Total comments: 12
Patch Set 28 : Address comments #Patch Set 29 : address comments #Patch Set 30 : Address comments #
Total comments: 25
Patch Set 31 : Address comments #Patch Set 32 : Address comments #
Total comments: 17
Patch Set 33 : Address comments #Patch Set 34 : address comments #
Total comments: 2
Patch Set 35 : address comments #Patch Set 36 : Only compile arc_tracing_agent.* for CrOS to avoid breaking other builds. #Patch Set 37 : rebase to tot #Messages
Total messages: 126 (46 generated)
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS. - http://crosreview.com/ - http://crrev.com/ - http://ag/ BUG=653795 TEST=1. Start container with CHROMEOS_DEV_MODE=0 2. Open chrome://tracing 3. Start tracing with android.trace.xxx enabled. 4. Confirm there is tracing results. ========== to ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1516244 (Android framework/base) - http://ag/1516604 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) BUG=653795 TEST=1. Start container with CHROMEOS_DEV_MODE=0 2. Open chrome://tracing 3. Start tracing with android.trace.xxx enabled. 4. Confirm there is tracing results. ==========
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1516244 (Android framework/base) - http://ag/1516604 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) BUG=653795 TEST=1. Start container with CHROMEOS_DEV_MODE=0 2. Open chrome://tracing 3. Start tracing with android.trace.xxx enabled. 4. Confirm there is tracing results. ========== to ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1516244 (Android framework/base) - http://ag/1516604 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) BUG=653795 TEST=1. Start container with CHROMEOS_DEV_MODE=0 2. Open chrome://tracing 3. Start tracing with android.trace.xxx enabled. 4. Confirm there is tracing results. ==========
shunhsingou@chromium.org changed reviewers: + bccheng@chromium.org, cnwan@chromium.org, drinkcat@chromium.org
bccheng@chromium.org changed reviewers: + nduca@google.com - cnwan@chromium.org
Nat, Earl is unifying atrace for Android with Chrome on ARC++. Could you review his CL or recommend other reviewers if applicable? Thanks!
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/trace/sys_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:21: const char kCrosTracingAgentName[] = "cros"; nit: constexpr. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:46: if (debug_daemon) { nit: if (!debug_daemon) return; https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:49: LOG(WARNING) << "Tracing ARC Start!!!"; This does not qualify as a warning. At most it's informative, and it's probably a candidate for VLOG. Also, try to avoid using "!" in logging statements. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:52: if (arc_trace_bridge) arc_trace_bridge->StartTracing(trace_config); nit: if (!arc_trace_bridge) return; https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:56: void SysTraceAgent::StopAgentTracing( What thread does this run as? This and the rest of the functions in this class probably needs thread-checking, like DCHECK_CURRENTLY_ON(content::BrowserThread::UI); https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:78: SysTraceAgent::SysTraceAgent() {} nit: = default; same below. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/trace/sys_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.h:21: Can you run `git cl format` and `git cl lint`? https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... File components/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:22: const std::string g_category_prefix( nit: constexpr char kCategoryPrefix[] = ...; https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:48: << "but no trace instance found"; GetInstanceForMethod already does logging, so this is superfluous. In fact, you can probably just call QueryAvailableCategories(). https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:57: if (trace_instance) { nit: if (!trace_instance) return; https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:65: categories_ = categories; This needs thread-checking code, like DCHECK(thread_checker_.CalledOnValidThread()); Same in all other methods that use |categories_|. https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:66: for (size_t i = 0; i < categories_.size(); ++i) { You can use for (const auto& category : categories_) { Same below. https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:83: std::string full_category_name = g_category_prefix + category_name; Should you store the full name in addition to the short name to avoid performing recalculation every time you do this? https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:91: if (trace_instance) trace_instance->StartTracing(std::move(categories)); This should be at the beginning of the function, using the if (!trace_instance) return; pattern.
https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/trace/sys_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:21: const char kCrosTracingAgentName[] = "cros"; On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > nit: constexpr. Done. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:46: if (debug_daemon) { On 2016/10/11 03:38:48, Luis Héctor Chávez wrote: > nit: > > if (!debug_daemon) > return; Done. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:49: LOG(WARNING) << "Tracing ARC Start!!!"; On 2016/10/11 03:38:48, Luis Héctor Chávez wrote: > This does not qualify as a warning. At most it's informative, and it's probably > a candidate for VLOG. Also, try to avoid using "!" in logging statements. Done. It's just for local debugging. Removed. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:52: if (arc_trace_bridge) arc_trace_bridge->StartTracing(trace_config); On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > nit: > > if (!arc_trace_bridge) > return; Done. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:56: void SysTraceAgent::StopAgentTracing( On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > What thread does this run as? This and the rest of the functions in this class > probably needs thread-checking, like > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Done. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.cc:78: SysTraceAgent::SysTraceAgent() {} On 2016/10/11 03:38:48, Luis Héctor Chávez wrote: > nit: = default; same below. Done. https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/trace/sys_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/trace/sys_trace_agent.h:21: On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > Can you run `git cl format` and `git cl lint`? Done. https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... File components/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:22: const std::string g_category_prefix( On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > nit: constexpr char kCategoryPrefix[] = ...; Done. https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:48: << "but no trace instance found"; On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > GetInstanceForMethod already does logging, so this is superfluous. In fact, you > can probably just call QueryAvailableCategories(). Done. https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:57: if (trace_instance) { On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > nit: > > if (!trace_instance) > return; Done. https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:65: categories_ = categories; On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > This needs thread-checking code, like > DCHECK(thread_checker_.CalledOnValidThread()); Same in all other methods that > use |categories_|. Done. https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:66: for (size_t i = 0; i < categories_.size(); ++i) { On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > You can use > > for (const auto& category : categories_) { > > Same below. Done. https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:83: std::string full_category_name = g_category_prefix + category_name; On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > Should you store the full name in addition to the short name to avoid performing > recalculation every time you do this? Done. https://codereview.chromium.org/2400163003/diff/60001/components/arc/trace/ar... components/arc/trace/arc_trace_bridge.cc:91: if (trace_instance) trace_instance->StartTracing(std::move(categories)); On 2016/10/11 03:38:49, Luis Héctor Chávez wrote: > This should be at the beginning of the function, using the > > if (!trace_instance) > return; > > pattern. Done.
overall comment: every time you git cl upload, write a short comment of what you changed from the previous patch set instead of leaving it blank (which will insert the change title). that makes reviewing easier. https://codereview.chromium.org/2400163003/diff/100001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/100001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:98: if (!success) { nit: elide braces if both the condition and the block fit on a single line. https://codereview.chromium.org/2400163003/diff/100001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/100001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:53: std::vector<std::pair<std::string, std::string>> categories_; can you create a struct to hold both strings? that'll require less explanation.
https://codereview.chromium.org/2400163003/diff/100001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/100001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:98: if (!success) { On 2016/10/11 13:13:03, Luis Héctor Chávez wrote: > nit: elide braces if both the condition and the block fit on a single line. Done. https://codereview.chromium.org/2400163003/diff/100001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/100001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:53: std::vector<std::pair<std::string, std::string>> categories_; On 2016/10/11 13:13:03, Luis Héctor Chávez wrote: > can you create a struct to hold both strings? that'll require less explanation. Done.
yusukes@chromium.org changed reviewers: + yusukes@chromium.org
nice feature! :) I'll probably be using this for tracing ARC container boot. I'm sure I'll need some more tweaks to do so, though. drive-by comments and questions below: https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:1204: "trace/sys_trace_agent.cc", move to chromeos/ probably? see my comment in DEPS. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/trace/sys_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.cc:7: #include <string> you can remove this (see my comment below) https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.cc:12: #include "base/trace_event/trace_event.h" same https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.cc:46: chromeos::DebugDaemonClient* debug_daemon = chromeos:: seems unnecessary (here and elsewhere) https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.cc:55: return; This path is taken if the user who hasn't opted into ARC tries to use this agent, and it's totally normal (hence no LOG()s), correct? If so, can you add some code comments? https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/trace/sys_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.h:14: namespace base { spaces between 14 and 15, 16 and 17 are missing. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.h:17: } } // namespace base ? https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.h:38: scoped_refptr<base::TaskRunner> task_runner); IWYU: #include for scoped_refptr<> missing? (IWYU == include what you use, btw) https://codereview.chromium.org/2400163003/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_host_impl.cc (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_host_impl.cc:203: void ArcBridgeHostImpl::OnTraceInstanceReady( sort https://codereview.chromium.org/2400163003/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_host_impl.h (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_host_impl.h:72: void OnTraceInstanceReady(mojom::TraceInstancePtr trace_ptr) override; sort https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:4: // Next MinVersion: 1 https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:7: interface TraceHost { remove line 7-8? I'm not sure defining an empty host makes sense. https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:10: interface TraceInstance { Mind adding comments to the interface and all methods? That would help the security review etc. https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:15: QueryAvailableCategories@2() => (array<string> categories); sort https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:7: #include <string> remove. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes "However, any includes present in the related header do not need to be included again in the related cc (i.e., foo.cc can rely on foo.h's includes)." https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:11: #include "base/trace_event/trace_event.h" remove https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:16: namespace { space between line 16 and 17. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:27: DCHECK(g_arc_trace_bridge); Does this make sense? Your c/b/chromeos/ code has null-checks. remove? https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:60: category, std::string(kCategoryPrefix) + std::string(category))); Converting both to std::string seems redundant. kCategoryPrefix + category.get() or something alike? https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:61: // Shows the category name in the selection UI. nit: Show (imperative form) https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:78: if (trace_config.IsCategoryGroupEnabled(category.full_name.c_str())) { nit: no {} for one-line if. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:16: #include "components/arc/arc_bridge_service.h" nit: is this #include necessary? namespace arc { class ArcBridgeService; seems sufficient. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:26: public mojom::TraceHost { Not sure if it makes sense to implement an empty host. remove? https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:34: // Callback when the bridge is ready. // InstanceHolder<mojom::TraceInstance>::Observer overrides: ? https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:53: mojo::Binding<mojom::TraceHost> binding_; same, remove? https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... File content/browser/tracing/DEPS (right): https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... content/browser/tracing/DEPS:11: "+chrome/browser/chromeos", hmm.. is this okay? I don't have Chromium code access right now, but my best guess is that there is no chrome/ dependencies in content/ atm.
https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... File content/browser/tracing/DEPS (right): https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... content/browser/tracing/DEPS:11: "+chrome/browser/chromeos", On 2016/10/12 05:58:14, Yusuke Sato wrote: > hmm.. is this okay? I don't have Chromium code access right now, but my best > guess is that there is no chrome/ dependencies in content/ atm. I tried to put codes (sys_trace_agent.*) in chromeos/. However, this introduces components/arc dependencies in chromeos/, which causes a circular dependency (chromeos/ is in components/arc dependencies now). I'm still seeking solutions for this. Any suggestion?
https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... File content/browser/tracing/DEPS (right): https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... content/browser/tracing/DEPS:11: "+chrome/browser/chromeos", On 2016/10/12 08:03:19, shunhsingou wrote: > On 2016/10/12 05:58:14, Yusuke Sato wrote: > > hmm.. is this okay? I don't have Chromium code access right now, but my best > > guess is that there is no chrome/ dependencies in content/ atm. > > I tried to put codes (sys_trace_agent.*) in chromeos/. However, this introduces > components/arc dependencies in chromeos/, which causes a circular dependency > (chromeos/ is in components/arc dependencies now). > > I'm still seeking solutions for this. Any suggestion? components/arc/DEPS only depends on a few chromeos/ sub directories and header files. So if you move them to chromeos/<new_directory>/ (e.g. chromeos/tracing/ ?) and add DEPS (with +components/arc) to the new directory, that won't introduce a circular dependency. WDYT? You'd probably want to ask one of the chromeos/OWNERS though.
On 2016/10/12 17:07:34, Yusuke Sato wrote: > https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... > File content/browser/tracing/DEPS (right): > > https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... > content/browser/tracing/DEPS:11: "+chrome/browser/chromeos", > On 2016/10/12 08:03:19, shunhsingou wrote: > > On 2016/10/12 05:58:14, Yusuke Sato wrote: > > > hmm.. is this okay? I don't have Chromium code access right now, but my best > > > guess is that there is no chrome/ dependencies in content/ atm. > > > > I tried to put codes (sys_trace_agent.*) in chromeos/. However, this > introduces > > components/arc dependencies in chromeos/, which causes a circular dependency > > (chromeos/ is in components/arc dependencies now). > > > > I'm still seeking solutions for this. Any suggestion? > > components/arc/DEPS only depends on a few chromeos/ sub directories and header > files. So if you move them to chromeos/<new_directory>/ (e.g. chromeos/tracing/ > ?) and add DEPS (with +components/arc) to the new directory, that won't > introduce a circular dependency. WDYT? You'd probably want to ask one of the > chromeos/OWNERS though. If the issue is because you want to register it in the ArcServiceManager, you can actually add it here to avoid the circular dependency: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_service_...
On 2016/10/12 17:07:34, Yusuke Sato wrote: > https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... > File content/browser/tracing/DEPS (right): > > https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... > content/browser/tracing/DEPS:11: "+chrome/browser/chromeos", > On 2016/10/12 08:03:19, shunhsingou wrote: > > On 2016/10/12 05:58:14, Yusuke Sato wrote: > > > hmm.. is this okay? I don't have Chromium code access right now, but my best > > > guess is that there is no chrome/ dependencies in content/ atm. > > > > I tried to put codes (sys_trace_agent.*) in chromeos/. However, this > introduces > > components/arc dependencies in chromeos/, which causes a circular dependency > > (chromeos/ is in components/arc dependencies now). > > > > I'm still seeking solutions for this. Any suggestion? > > components/arc/DEPS only depends on a few chromeos/ sub directories and header > files. So if you move them to chromeos/<new_directory>/ (e.g. chromeos/tracing/ > ?) and add DEPS (with +components/arc) to the new directory, that won't > introduce a circular dependency. WDYT? You'd probably want to ask one of the > chromeos/OWNERS though. If the issue is because you want to register it in the ArcServiceManager, you can actually add it here to avoid the circular dependency: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_service_...
https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:50: Category(const std::string& name, const std::string& full_name) Can you use the uniform initialization syntax to avoid this explicit constructor? Category c{"name", "full_name"}; https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:59: // List of available categories name. Each item is a pair of string. The This comment is now out of date.
https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:11: StartTracing@0(array<string> categories) => (bool success); Is there any chance the TraceInstance will want to call any method of the TraceHost in the future? If so, it might be a good idea to declare Init@0(TraceHost host); just like the other interfaces. If there is exactly 0% chance, let's get rid of TraceHost altogether.
shunhsingou@chromium.org changed reviewers: + achuith@chromium.org, dsinclair@chromium.org
Add achuith@ (owner of /chromeos) and dsinclair (owner of /content/browser/trace) to see if there is any suggestion. Hi Achuith, Dan, Nat, I'm trying to enable tracing in arc from chrome://tracing, but see some dependency issues. The story is that I need to run code from components/arc, or chrome/browser/chromeos/arc as suggested by Luis in the comment, in /content/browser/trace. Both dependencies seem not allowed by the definition of "content module". Currently /content/browser depends on /chromeos, so I also tried to add components/arc or chrome/browser/chromeos/arc into /chromeos/BUILD.gn, which is not working due to circular dependency. I've also tried the suggestion from Yusuke. Creating /chromeos/tracing failed to solve the problem because there is "+chromeos" in components/arc/BUILD.gn, so we cannot add "+components/arc" in chromeos/BUILD.gn. Adding DEPS in /chrome/tracing is not enough. I'm a complete newbie of chromium source tree. Do you have any suggestion that I can try for this?
dsinclair@chromium.org changed reviewers: + oysteine@chromium.org, primiano@chromium.org
Adding oysteine@ and primiano@ as they maybe able to help work out the layering requirements.
shunhsingou, did you get any suggestion? If you haven't, how about this:
* Move the SysTraceAgent class to chromeos/, and add a 'Delegate' inner class to
the class:
// Singleton
class SysTraceAgent : public base::trace_event::TracingAgent {
public:
...
class Delegate {
...
};
...
void SetDelegate(Delegate* delegate);
...
private:
Delegate* const delegate_;
...
};
* Change sys_trace_agent.cc so each SysTraceAgent method just calls into its
|delegate_|.
std::string SysTraceAgent::GetTracingAgentName() { return
delegate_->GetTracingAgentName(); }
* Implement the Delegate interface in chrome/browser/chromeos/trace/.
// Also singleton.
class SysTraceAgentDelegate : public SysTraceAgent::Delegate {
...
};
* Set the delegate singleton to the SysTraceAgent singleton instance in e.g.
chrome/browser/chromeos/chrome_browser_main_chromeos.cc.
This way, you can keep content's and chromeos' DEPS files unchanged.
FYI, chromeos/login_event_recorder.h does almost exactly the same.
[chromeos/login_event_recorder.h]
class CHROMEOS_EXPORT LoginEventRecorder {
public:
class Delegate {
[chrome/browser/chromeos/boot_times_recorder.h]
class BootTimesRecorder : public content::NotificationObserver,
public LoginEventRecorder::Delegate {
public:
...
// LoginEventRecorder::Delegate override.
void AddLoginTimeMarker(const std::string& marker_name,
bool send_to_uma) override;
void RecordAuthenticationSuccess() override;
void RecordAuthenticationFailure() override;
[chrome/browser/chromeos/chrome_browser_main_chromeos.cc]
LoginEventRecorder::Get()->SetDelegate(BootTimesRecorder::Get());
WDYT?
https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:55: base::WeakPtrFactory<ArcTraceBridge> weak_ptr_factory_; weak_ptr_factory_ must be the last member variable of the class (so that weak_ptr_factory_ is destructed before other member variables in this class are.) Can you run git cl try once you finish reorganizing the CL? Clang Linux builder should have been able to catch this. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:62: std::vector<Category> categories_; This does not seem to follow the class formatting rule. private: // private member functions here... // private member variables here... DISALLOW_COPY_AND_ASSIGN(...); }; https://google.github.io/styleguide/cppguide.html#Class_Format
On 2016/10/27 04:09:10, Yusuke Sato wrote:
> shunhsingou, did you get any suggestion? If you haven't, how about this:
>
> * Move the SysTraceAgent class to chromeos/, and add a 'Delegate' inner class
to
> the class:
>
> // Singleton
> class SysTraceAgent : public base::trace_event::TracingAgent {
> public:
> ...
> class Delegate {
> ...
> };
> ...
> void SetDelegate(Delegate* delegate);
> ...
> private:
> Delegate* const delegate_;
> ...
> };
>
> * Change sys_trace_agent.cc so each SysTraceAgent method just calls into its
> |delegate_|.
>
> std::string SysTraceAgent::GetTracingAgentName() { return
> delegate_->GetTracingAgentName(); }
>
> * Implement the Delegate interface in chrome/browser/chromeos/trace/.
>
> // Also singleton.
> class SysTraceAgentDelegate : public SysTraceAgent::Delegate {
> ...
> };
>
> * Set the delegate singleton to the SysTraceAgent singleton instance in e.g.
> chrome/browser/chromeos/chrome_browser_main_chromeos.cc.
>
> This way, you can keep content's and chromeos' DEPS files unchanged.
>
>
> FYI, chromeos/login_event_recorder.h does almost exactly the same.
>
> [chromeos/login_event_recorder.h]
>
> class CHROMEOS_EXPORT LoginEventRecorder {
> public:
> class Delegate {
>
>
> [chrome/browser/chromeos/boot_times_recorder.h]
>
> class BootTimesRecorder : public content::NotificationObserver,
> public LoginEventRecorder::Delegate {
> public:
> ...
> // LoginEventRecorder::Delegate override.
> void AddLoginTimeMarker(const std::string& marker_name,
> bool send_to_uma) override;
> void RecordAuthenticationSuccess() override;
> void RecordAuthenticationFailure() override;
>
>
> [chrome/browser/chromeos/chrome_browser_main_chromeos.cc]
>
> LoginEventRecorder::Get()->SetDelegate(BootTimesRecorder::Get());
>
>
> WDYT?
This solution looks good to me. Thanks for the suggestion!
On 2016/10/27 06:18:58, shunhsingou wrote:
> On 2016/10/27 04:09:10, Yusuke Sato wrote:
> > shunhsingou, did you get any suggestion? If you haven't, how about this:
> >
> > * Move the SysTraceAgent class to chromeos/, and add a 'Delegate' inner
class
> to
> > the class:
> >
> > // Singleton
> > class SysTraceAgent : public base::trace_event::TracingAgent {
> > public:
> > ...
> > class Delegate {
> > ...
> > };
> > ...
> > void SetDelegate(Delegate* delegate);
> > ...
> > private:
> > Delegate* const delegate_;
> > ...
> > };
> >
> > * Change sys_trace_agent.cc so each SysTraceAgent method just calls into its
> > |delegate_|.
> >
> > std::string SysTraceAgent::GetTracingAgentName() { return
> > delegate_->GetTracingAgentName(); }
> >
> > * Implement the Delegate interface in chrome/browser/chromeos/trace/.
> >
> > // Also singleton.
> > class SysTraceAgentDelegate : public SysTraceAgent::Delegate {
> > ...
> > };
> >
> > * Set the delegate singleton to the SysTraceAgent singleton instance in e.g.
> > chrome/browser/chromeos/chrome_browser_main_chromeos.cc.
> >
> > This way, you can keep content's and chromeos' DEPS files unchanged.
> >
> >
> > FYI, chromeos/login_event_recorder.h does almost exactly the same.
> >
> > [chromeos/login_event_recorder.h]
> >
> > class CHROMEOS_EXPORT LoginEventRecorder {
> > public:
> > class Delegate {
> >
> >
> > [chrome/browser/chromeos/boot_times_recorder.h]
> >
> > class BootTimesRecorder : public content::NotificationObserver,
> > public LoginEventRecorder::Delegate {
> > public:
> > ...
> > // LoginEventRecorder::Delegate override.
> > void AddLoginTimeMarker(const std::string& marker_name,
> > bool send_to_uma) override;
> > void RecordAuthenticationSuccess() override;
> > void RecordAuthenticationFailure() override;
> >
> >
> > [chrome/browser/chromeos/chrome_browser_main_chromeos.cc]
> >
> > LoginEventRecorder::Get()->SetDelegate(BootTimesRecorder::Get());
> >
> >
> > WDYT?
>
> This solution looks good to me. Thanks for the suggestion!
A quick update for this change sets:
I'm working on a new solution for
https://chrome-internal-review.googlesource.com/#/c/272648/. The original CL
creates some security issues on Chrome OS side. So some major revision is also
required for this.
dsinclair@chromium.org changed reviewers: - dsinclair@chromium.org
On 2016/10/27 06:20:45, shunhsingou wrote:
> On 2016/10/27 06:18:58, shunhsingou wrote:
> > On 2016/10/27 04:09:10, Yusuke Sato wrote:
> > > shunhsingou, did you get any suggestion? If you haven't, how about this:
> > >
> > > * Move the SysTraceAgent class to chromeos/, and add a 'Delegate' inner
> class
> > to
> > > the class:
> > >
> > > // Singleton
> > > class SysTraceAgent : public base::trace_event::TracingAgent {
> > > public:
> > > ...
> > > class Delegate {
> > > ...
> > > };
> > > ...
> > > void SetDelegate(Delegate* delegate);
> > > ...
> > > private:
> > > Delegate* const delegate_;
> > > ...
> > > };
> > >
> > > * Change sys_trace_agent.cc so each SysTraceAgent method just calls into
its
> > > |delegate_|.
> > >
> > > std::string SysTraceAgent::GetTracingAgentName() { return
> > > delegate_->GetTracingAgentName(); }
> > >
> > > * Implement the Delegate interface in chrome/browser/chromeos/trace/.
> > >
> > > // Also singleton.
> > > class SysTraceAgentDelegate : public SysTraceAgent::Delegate {
> > > ...
> > > };
> > >
> > > * Set the delegate singleton to the SysTraceAgent singleton instance in
e.g.
>
> > > chrome/browser/chromeos/chrome_browser_main_chromeos.cc.
> > >
> > > This way, you can keep content's and chromeos' DEPS files unchanged.
> > >
> > >
> > > FYI, chromeos/login_event_recorder.h does almost exactly the same.
> > >
> > > [chromeos/login_event_recorder.h]
> > >
> > > class CHROMEOS_EXPORT LoginEventRecorder {
> > > public:
> > > class Delegate {
> > >
> > >
> > > [chrome/browser/chromeos/boot_times_recorder.h]
> > >
> > > class BootTimesRecorder : public content::NotificationObserver,
> > > public LoginEventRecorder::Delegate {
> > > public:
> > > ...
> > > // LoginEventRecorder::Delegate override.
> > > void AddLoginTimeMarker(const std::string& marker_name,
> > > bool send_to_uma) override;
> > > void RecordAuthenticationSuccess() override;
> > > void RecordAuthenticationFailure() override;
> > >
> > >
> > > [chrome/browser/chromeos/chrome_browser_main_chromeos.cc]
> > >
> > > LoginEventRecorder::Get()->SetDelegate(BootTimesRecorder::Get());
> > >
> > >
> > > WDYT?
> >
> > This solution looks good to me. Thanks for the suggestion!
>
> A quick update for this change sets:
> I'm working on a new solution for
> https://chrome-internal-review.googlesource.com/#/c/272648/. The original CL
> creates some security issues on Chrome OS side. So some major revision is also
> required for this.
Any update on this? This feature would be useful for doing b/32070337 and I'd
like to know ETA.
On 2016/11/02 23:16:58, Yusuke Sato wrote:
> On 2016/10/27 06:20:45, shunhsingou wrote:
> > On 2016/10/27 06:18:58, shunhsingou wrote:
> > > On 2016/10/27 04:09:10, Yusuke Sato wrote:
> > > > shunhsingou, did you get any suggestion? If you haven't, how about this:
> > > >
> > > > * Move the SysTraceAgent class to chromeos/, and add a 'Delegate' inner
> > class
> > > to
> > > > the class:
> > > >
> > > > // Singleton
> > > > class SysTraceAgent : public base::trace_event::TracingAgent {
> > > > public:
> > > > ...
> > > > class Delegate {
> > > > ...
> > > > };
> > > > ...
> > > > void SetDelegate(Delegate* delegate);
> > > > ...
> > > > private:
> > > > Delegate* const delegate_;
> > > > ...
> > > > };
> > > >
> > > > * Change sys_trace_agent.cc so each SysTraceAgent method just calls into
> its
> > > > |delegate_|.
> > > >
> > > > std::string SysTraceAgent::GetTracingAgentName() { return
> > > > delegate_->GetTracingAgentName(); }
> > > >
> > > > * Implement the Delegate interface in chrome/browser/chromeos/trace/.
> > > >
> > > > // Also singleton.
> > > > class SysTraceAgentDelegate : public SysTraceAgent::Delegate {
> > > > ...
> > > > };
> > > >
> > > > * Set the delegate singleton to the SysTraceAgent singleton instance in
> e.g.
> >
> > > > chrome/browser/chromeos/chrome_browser_main_chromeos.cc.
> > > >
> > > > This way, you can keep content's and chromeos' DEPS files unchanged.
> > > >
> > > >
> > > > FYI, chromeos/login_event_recorder.h does almost exactly the same.
> > > >
> > > > [chromeos/login_event_recorder.h]
> > > >
> > > > class CHROMEOS_EXPORT LoginEventRecorder {
> > > > public:
> > > > class Delegate {
> > > >
> > > >
> > > > [chrome/browser/chromeos/boot_times_recorder.h]
> > > >
> > > > class BootTimesRecorder : public content::NotificationObserver,
> > > > public LoginEventRecorder::Delegate {
> > > > public:
> > > > ...
> > > > // LoginEventRecorder::Delegate override.
> > > > void AddLoginTimeMarker(const std::string& marker_name,
> > > > bool send_to_uma) override;
> > > > void RecordAuthenticationSuccess() override;
> > > > void RecordAuthenticationFailure() override;
> > > >
> > > >
> > > > [chrome/browser/chromeos/chrome_browser_main_chromeos.cc]
> > > >
> > > > LoginEventRecorder::Get()->SetDelegate(BootTimesRecorder::Get());
> > > >
> > > >
> > > > WDYT?
> > >
> > > This solution looks good to me. Thanks for the suggestion!
> >
> > A quick update for this change sets:
> > I'm working on a new solution for
> > https://chrome-internal-review.googlesource.com/#/c/272648/. The original CL
> > creates some security issues on Chrome OS side. So some major revision is
also
> > required for this.
>
> Any update on this? This feature would be useful for doing b/32070337 and I'd
> like to know ETA.
We see some problem when trying to reduce the security impact, so things are
still under discussion. I think we still need 1 ~ 2 weeks before we can have a
conclusion on how to design it.
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1516244 (Android framework/base) - http://ag/1516604 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) BUG=653795 TEST=1. Start container with CHROMEOS_DEV_MODE=0 2. Open chrome://tracing 3. Start tracing with android.trace.xxx enabled. 4. Confirm there is tracing results. ========== to ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738291 (Android framework/base) - http://ag/1738271 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) - https://chromium-review.googlesource.com/#/c/422750 (ChromiumOS platform2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
On 2016/11/03 02:14:30, shunhsingou wrote:
> On 2016/11/02 23:16:58, Yusuke Sato wrote:
> > On 2016/10/27 06:20:45, shunhsingou wrote:
> > > On 2016/10/27 06:18:58, shunhsingou wrote:
> > > > On 2016/10/27 04:09:10, Yusuke Sato wrote:
> > > > > shunhsingou, did you get any suggestion? If you haven't, how about
this:
> > > > >
> > > > > * Move the SysTraceAgent class to chromeos/, and add a 'Delegate'
inner
> > > class
> > > > to
> > > > > the class:
> > > > >
> > > > > // Singleton
> > > > > class SysTraceAgent : public base::trace_event::TracingAgent {
> > > > > public:
> > > > > ...
> > > > > class Delegate {
> > > > > ...
> > > > > };
> > > > > ...
> > > > > void SetDelegate(Delegate* delegate);
> > > > > ...
> > > > > private:
> > > > > Delegate* const delegate_;
> > > > > ...
> > > > > };
> > > > >
> > > > > * Change sys_trace_agent.cc so each SysTraceAgent method just calls
into
> > its
> > > > > |delegate_|.
> > > > >
> > > > > std::string SysTraceAgent::GetTracingAgentName() { return
> > > > > delegate_->GetTracingAgentName(); }
> > > > >
> > > > > * Implement the Delegate interface in chrome/browser/chromeos/trace/.
> > > > >
> > > > > // Also singleton.
> > > > > class SysTraceAgentDelegate : public SysTraceAgent::Delegate {
> > > > > ...
> > > > > };
> > > > >
> > > > > * Set the delegate singleton to the SysTraceAgent singleton instance
in
> > e.g.
> > >
> > > > > chrome/browser/chromeos/chrome_browser_main_chromeos.cc.
> > > > >
> > > > > This way, you can keep content's and chromeos' DEPS files unchanged.
> > > > >
> > > > >
> > > > > FYI, chromeos/login_event_recorder.h does almost exactly the same.
> > > > >
> > > > > [chromeos/login_event_recorder.h]
> > > > >
> > > > > class CHROMEOS_EXPORT LoginEventRecorder {
> > > > > public:
> > > > > class Delegate {
> > > > >
> > > > >
> > > > > [chrome/browser/chromeos/boot_times_recorder.h]
> > > > >
> > > > > class BootTimesRecorder : public content::NotificationObserver,
> > > > > public LoginEventRecorder::Delegate {
> > > > > public:
> > > > > ...
> > > > > // LoginEventRecorder::Delegate override.
> > > > > void AddLoginTimeMarker(const std::string& marker_name,
> > > > > bool send_to_uma) override;
> > > > > void RecordAuthenticationSuccess() override;
> > > > > void RecordAuthenticationFailure() override;
> > > > >
> > > > >
> > > > > [chrome/browser/chromeos/chrome_browser_main_chromeos.cc]
> > > > >
> > > > > LoginEventRecorder::Get()->SetDelegate(BootTimesRecorder::Get());
> > > > >
> > > > >
> > > > > WDYT?
> > > >
> > > > This solution looks good to me. Thanks for the suggestion!
> > >
> > > A quick update for this change sets:
> > > I'm working on a new solution for
> > > https://chrome-internal-review.googlesource.com/#/c/272648/. The original
CL
> > > creates some security issues on Chrome OS side. So some major revision is
> also
> > > required for this.
> >
> > Any update on this? This feature would be useful for doing b/32070337 and
I'd
> > like to know ETA.
>
> We see some problem when trying to reduce the security impact, so things are
> still under discussion. I think we still need 1 ~ 2 weeks before we can have a
> conclusion on how to design it.
New patch upload with following changes:
1. Avoid adding dependency to chromeos and content according to above
discussion.
2. Avoid https://chrome-internal-review.googlesource.com/#/c/272648/ for
security.
Will fix other style issue in next patch.
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738291 (Android framework/base) - http://ag/1738271 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) - https://chromium-review.googlesource.com/#/c/422750 (ChromiumOS platform2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738291 (Android framework/base) - http://ag/1738271 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) - https://chromium-review.googlesource.com/#/c/422750 (ChromiumOS platform2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
The CQ bit was checked by shunhsingou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The dependency is fixed as: - Put ArcTraceBridge in chrome/browser/chromeos - Remove SysTraceAgent, and put ArcTraceAgent in chromeos/dbus/. This is because it uses dbus to get report from debugd in new design to avoid exploring ftrace. - Set ArcTraceBridge into ArcTraceAgent in chrome_browser_main_chromeos.cc. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:1204: "trace/sys_trace_agent.cc", On 2016/10/12 05:58:12, Yusuke Sato (ooo Dec 16 to 31) wrote: > move to chromeos/ probably? see my comment in DEPS. Moved according to the comment. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/trace/sys_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.cc:7: #include <string> On 2016/10/12 05:58:12, Yusuke Sato (ooo Dec 16 to 31) wrote: > you can remove this (see my comment below) Done. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.cc:12: #include "base/trace_event/trace_event.h" On 2016/10/12 05:58:12, Yusuke Sato (ooo Dec 16 to 31) wrote: > same Done. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.cc:46: chromeos::DebugDaemonClient* debug_daemon = On 2016/10/12 05:58:12, Yusuke Sato (ooo Dec 16 to 31) wrote: > chromeos:: seems unnecessary (here and elsewhere) Done. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.cc:55: return; On 2016/10/12 05:58:12, Yusuke Sato (ooo Dec 16 to 31) wrote: > This path is taken if the user who hasn't opted into ARC tries to use this > agent, and it's totally normal (hence no LOG()s), correct? If so, can you add > some code comments? Done. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/trace/sys_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.h:14: namespace base { On 2016/10/12 05:58:12, Yusuke Sato (ooo Dec 16 to 31) wrote: > spaces between 14 and 15, 16 and 17 are missing. The forward declaration is now removed. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.h:17: } On 2016/10/12 05:58:12, Yusuke Sato (ooo Dec 16 to 31) wrote: > } // namespace base > > ? Done. https://codereview.chromium.org/2400163003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/trace/sys_trace_agent.h:38: scoped_refptr<base::TaskRunner> task_runner); On 2016/10/12 05:58:12, Yusuke Sato (ooo Dec 16 to 31) wrote: > IWYU: #include for scoped_refptr<> missing? > > (IWYU == include what you use, btw) Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_host_impl.cc (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_host_impl.cc:203: void ArcBridgeHostImpl::OnTraceInstanceReady( On 2016/10/12 05:58:12, Yusuke Sato (ooo Dec 16 to 31) wrote: > sort Removed https://codereview.chromium.org/2400163003/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_host_impl.h (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_host_impl.h:72: void OnTraceInstanceReady(mojom::TraceInstancePtr trace_ptr) override; On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > sort Removed https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:4: On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > // Next MinVersion: 1 Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:7: interface TraceHost { On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > remove line 7-8? I'm not sure defining an empty host makes sense. Removed https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:10: interface TraceInstance { On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > Mind adding comments to the interface and all methods? That would help the > security review etc. Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:11: StartTracing@0(array<string> categories) => (bool success); On 2016/10/12 20:32:07, Luis Héctor Chávez wrote: > Is there any chance the TraceInstance will want to call any method of the > TraceHost in the future? If so, it might be a good idea to declare > > Init@0(TraceHost host); > > just like the other interfaces. If there is exactly 0% chance, let's get rid of > TraceHost altogether. Removed https://codereview.chromium.org/2400163003/diff/120001/components/arc/common/... components/arc/common/trace.mojom:15: QueryAvailableCategories@2() => (array<string> categories); On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > sort Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:7: #include <string> On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > remove. > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > > "However, any includes present in the related header do not need to be included > again in the related cc (i.e., foo.cc can rely on foo.h's includes)." Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:11: #include "base/trace_event/trace_event.h" On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > remove Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:16: namespace { On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > space between line 16 and 17. Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:27: DCHECK(g_arc_trace_bridge); On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > Does this make sense? Your c/b/chromeos/ code has null-checks. remove? Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:60: category, std::string(kCategoryPrefix) + std::string(category))); On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > Converting both to std::string seems redundant. > > kCategoryPrefix + category.get() > > or something alike? Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:61: // Shows the category name in the selection UI. On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > nit: Show (imperative form) Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.cc:78: if (trace_config.IsCategoryGroupEnabled(category.full_name.c_str())) { On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > nit: no {} for one-line if. Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... File components/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:16: #include "components/arc/arc_bridge_service.h" On 2016/10/12 05:58:14, Yusuke Sato (ooo Dec 16 to 31) wrote: > nit: is this #include necessary? > > namespace arc { > > class ArcBridgeService; > > seems sufficient. Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:26: public mojom::TraceHost { On 2016/10/12 05:58:13, Yusuke Sato (ooo Dec 16 to 31) wrote: > Not sure if it makes sense to implement an empty host. remove? Removed https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:34: // Callback when the bridge is ready. On 2016/10/12 05:58:14, Yusuke Sato (ooo Dec 16 to 31) wrote: > // InstanceHolder<mojom::TraceInstance>::Observer overrides: > > ? Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:50: Category(const std::string& name, const std::string& full_name) On 2016/10/12 17:12:40, Luis Héctor Chávez wrote: > Can you use the uniform initialization syntax to avoid this explicit > constructor? > > Category c{"name", "full_name"}; Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:53: mojo::Binding<mojom::TraceHost> binding_; On 2016/10/12 05:58:14, Yusuke Sato (ooo Dec 16 to 31) wrote: > same, remove? Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:55: base::WeakPtrFactory<ArcTraceBridge> weak_ptr_factory_; On 2016/10/27 04:11:24, Yusuke Sato (ooo Dec 16 to 31) wrote: > weak_ptr_factory_ must be the last member variable of the class (so that > weak_ptr_factory_ is destructed before other member variables in this class > are.) > > Can you run git cl try once you finish reorganizing the CL? Clang Linux builder > should have been able to catch this. Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:59: // List of available categories name. Each item is a pair of string. The On 2016/10/12 17:12:40, Luis Héctor Chávez wrote: > This comment is now out of date. Done. https://codereview.chromium.org/2400163003/diff/120001/components/arc/trace/a... components/arc/trace/arc_trace_bridge.h:62: std::vector<Category> categories_; On 2016/10/27 04:11:24, Yusuke Sato (ooo Dec 16 to 31) wrote: > This does not seem to follow the class formatting rule. > > private: > // private member functions here... > > // private member variables here... > > DISALLOW_COPY_AND_ASSIGN(...); > }; > > https://google.github.io/styleguide/cppguide.html#Class_Format Done. https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... File content/browser/tracing/DEPS (right): https://codereview.chromium.org/2400163003/diff/120001/content/browser/tracin... content/browser/tracing/DEPS:11: "+chrome/browser/chromeos", On 2016/10/12 17:07:34, Yusuke Sato (ooo Dec 16 to 31) wrote: > On 2016/10/12 08:03:19, shunhsingou wrote: > > On 2016/10/12 05:58:14, Yusuke Sato wrote: > > > hmm.. is this okay? I don't have Chromium code access right now, but my best > > > guess is that there is no chrome/ dependencies in content/ atm. > > > > I tried to put codes (sys_trace_agent.*) in chromeos/. However, this > introduces > > components/arc dependencies in chromeos/, which causes a circular dependency > > (chromeos/ is in components/arc dependencies now). > > > > I'm still seeking solutions for this. Any suggestion? > > components/arc/DEPS only depends on a few chromeos/ sub directories and header > files. So if you move them to chromeos/<new_directory>/ (e.g. chromeos/tracing/ > ?) and add DEPS (with +components/arc) to the new directory, that won't > introduce a circular dependency. WDYT? You'd probably want to ask one of the > chromeos/OWNERS though. Changed according to the comment.
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738291 (Android framework/base) - http://ag/1738271 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) - https://chromium-review.googlesource.com/#/c/422750 (ChromiumOS platform2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738292 (Android framework/base) - http://ag/1738271 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) - https://chromium-review.googlesource.com/#/c/422750 (ChromiumOS platform2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
first round https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:24: } // namcespace nit: s/namcespace/namespace/ https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:33: g_arc_trace_bridge = this; DCHECK(!g_arc_trace_bridge); https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:57: Do you need |categories_.clear();| here? https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:58: virtual void StartTracing( These two functions are overriden, so they should have the 'override' modifier instead of 'virtual'. Also, add // chromeos::ArcTraceAgent::Delegate overrides: before both of them (and you can delete the newline that separates them to be clearer about which functions are the overriden ones). https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:63: base::ThreadChecker thread_checker_; Since you are in chrome/browser/chromeos, the preferred way of checking if it's on the right thread is to add DCHECK_CURRENTLY_ON(BrowserThread::UI); to the beginning of all methods instead of relying on base::ThredChecker. https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:434: // Initialize dbus ArgTraceAgent by setting ArcTraceBridge. nit: s/ArgTraceAgent/ArcTraceAgent/ Also, this comment is just describing the very next line (instead of describing why it's doing so), so maybe drop it? https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:42: // arc_trace_brigde_ may be null if ARC is not enabled on the system. In such s/arc_trace_brigde_/arc_trace_bridge_/ https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:53: base::Bind(&ArcTraceAgent::OnStartMethod, Shouldn't you run |callback| in OnStartMethod instead of unconditionally? https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:57: base::ThreadTaskRunnerHandle::Get()->PostTask( Why can't you just do callback.Run(GetTracingAgentName(), true /* success */); ? if there is a reason, please add a comment about it. Also, why are you returning success unconditionally? https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:63: void ArcTraceAgent::OnStartMethod(dbus::Response* response) { This can be a standalone function in the anonymous namespace. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:72: if (pipe_reader_ != NULL) { Maybe it's better to check if |callback_.is_null()| instead of checking for pipe_reader_. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:77: if (arc_trace_bridge_) { if (!arc_trace_bridge_) return; // rest of the function. also, shouldn't you call the callback with failure if this happens? https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:80: pipe_reader_.reset( nit: pipe_reader_ = base::MakeUnique<PipeReaderForString>(... args ...) https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:85: base::ScopedFD pipe_write_end = pipe_reader_->StartIO(); What thread is this supposed to be run on? Is I/O allowed in that thread? https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:102: // Called when pipe i/o completes; pass data on and delete the instance. nit: be consistent and use I/O all across the board. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:106: callback_.Run(GetTracingAgentName(), GetTraceEventLabel(), nit: base::ResetAndReturn(&callback_).Run(...); https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:108: pipe_reader_.reset(); maybe reset the pipe_reader_ before calling the callback. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:124: stop_agent_tracing_task_runner_ = task_runner; alignment. please run `git cl format` and `git cl lint` before uploading this again. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:127: ArcTraceAgent* ArcTraceAgent::Create() { nit: // static https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:138: : arc_trace_bridge_(NULL), nit: s/NULL/nullptr/ all across the change. Also, you can move those initializations to the header file. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:142: ArcTraceAgent::~ArcTraceAgent() { nit: = default; https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:26: class ArcTraceBridge { This is a delegate, so maybe call it Delegate. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:38: // Overrides TracingAgent functions. // base::trace_event::TracingAgent overrides: (and remove the empty lines between the overridden methods). https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:51: void Init(dbus::Bus* bus) override; // DBusClient overrides: https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:58: DISALLOW_COPY_AND_ASSIGN(ArcTraceAgent); This should be at the very end of a class. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:64: ArcTraceBridge* arc_trace_bridge_; Please note who owns all the raw pointers. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:67: base::WeakPtrFactory<ArcTraceAgent> weak_ptr_factory_; WeakPtrFactories should always be the very last entries of a class (as noted in their documentation: https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?q=base::WeakPtrFa... ) https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:69: StopAgentTracingCallback callback_; nit: stop_callback_
https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:24: } // namcespace On 2016/12/27 19:15:58, Luis Héctor Chávez wrote: > nit: s/namcespace/namespace/ Done. https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:33: g_arc_trace_bridge = this; On 2016/12/27 19:15:58, Luis Héctor Chávez wrote: > DCHECK(!g_arc_trace_bridge); Done. https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:57: On 2016/12/27 19:15:58, Luis Héctor Chávez wrote: > Do you need |categories_.clear();| here? Done. https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:58: virtual void StartTracing( On 2016/12/27 19:15:58, Luis Héctor Chávez wrote: > These two functions are overriden, so they should have the 'override' modifier > instead of 'virtual'. > > Also, add > > // chromeos::ArcTraceAgent::Delegate overrides: > > before both of them (and you can delete the newline that separates them to be > clearer about which functions are the overriden ones). Done. https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:63: base::ThreadChecker thread_checker_; On 2016/12/27 19:15:58, Luis Héctor Chávez wrote: > Since you are in chrome/browser/chromeos, the preferred way of checking if it's > on the right thread is to add > > DCHECK_CURRENTLY_ON(BrowserThread::UI); > > to the beginning of all methods instead of relying on base::ThredChecker. Done. https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2400163003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:434: // Initialize dbus ArgTraceAgent by setting ArcTraceBridge. On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > nit: s/ArgTraceAgent/ArcTraceAgent/ > > Also, this comment is just describing the very next line (instead of describing > why it's doing so), so maybe drop it? Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:42: // arc_trace_brigde_ may be null if ARC is not enabled on the system. In such On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > s/arc_trace_brigde_/arc_trace_bridge_/ Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:53: base::Bind(&ArcTraceAgent::OnStartMethod, On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > Shouldn't you run |callback| in OnStartMethod instead of unconditionally? Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:57: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > Why can't you just do > > callback.Run(GetTracingAgentName(), true /* success */); > > ? if there is a reason, please add a comment about it. Also, why are you > returning success unconditionally? I forget to change. Move it to OnStartMethod. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:63: void ArcTraceAgent::OnStartMethod(dbus::Response* response) { On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > This can be a standalone function in the anonymous namespace. Moved callback call to here. So this function need access to GetTracingAgentName(). https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:72: if (pipe_reader_ != NULL) { On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > Maybe it's better to check if |callback_.is_null()| instead of checking for > pipe_reader_. Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:77: if (arc_trace_bridge_) { On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > if (!arc_trace_bridge_) > return; > > // rest of the function. > > also, shouldn't you call the callback with failure if this happens? Done. Just return empty report here. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:80: pipe_reader_.reset( On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > nit: pipe_reader_ = base::MakeUnique<PipeReaderForString>(... args ...) Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:85: base::ScopedFD pipe_write_end = pipe_reader_->StartIO(); On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > What thread is this supposed to be run on? Is I/O allowed in that thread? It's running on 'stop_agent_tracing_task_runner_', which is set in tracing_controller_impl.cc as a blocking thread. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:102: // Called when pipe i/o completes; pass data on and delete the instance. On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > nit: be consistent and use I/O all across the board. Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:106: callback_.Run(GetTracingAgentName(), GetTraceEventLabel(), On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > nit: base::ResetAndReturn(&callback_).Run(...); Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:108: pipe_reader_.reset(); On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > maybe reset the pipe_reader_ before calling the callback. Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:124: stop_agent_tracing_task_runner_ = task_runner; On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > alignment. please run `git cl format` and `git cl lint` before uploading this > again. Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:127: ArcTraceAgent* ArcTraceAgent::Create() { On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > nit: // static Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:138: : arc_trace_bridge_(NULL), On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > nit: s/NULL/nullptr/ all across the change. > > Also, you can move those initializations to the header file. Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:142: ArcTraceAgent::~ArcTraceAgent() { On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > nit: = default; Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:26: class ArcTraceBridge { On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > This is a delegate, so maybe call it Delegate. Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:38: // Overrides TracingAgent functions. On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > // base::trace_event::TracingAgent overrides: > > (and remove the empty lines between the overridden methods). Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:51: void Init(dbus::Bus* bus) override; On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > // DBusClient overrides: Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:58: DISALLOW_COPY_AND_ASSIGN(ArcTraceAgent); On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > This should be at the very end of a class. Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:64: ArcTraceBridge* arc_trace_bridge_; On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > Please note who owns all the raw pointers. Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:67: base::WeakPtrFactory<ArcTraceAgent> weak_ptr_factory_; On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > WeakPtrFactories should always be the very last entries of a class (as noted in > their documentation: > https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?q=base::WeakPtrFa... > ) Done. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:69: StopAgentTracingCallback callback_; On 2016/12/27 19:16:00, Luis Héctor Chávez wrote: > nit: stop_callback_ Done.
Second round, will now go through the rest of the changes. https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:85: base::ScopedFD pipe_write_end = pipe_reader_->StartIO(); On 2016/12/28 09:15:15, shunhsingou wrote: > On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > > What thread is this supposed to be run on? Is I/O allowed in that thread? > > It's running on 'stop_agent_tracing_task_runner_', which is set in > tracing_controller_impl.cc as a blocking thread. Can you add comments about which methods run on which threads? https://codereview.chromium.org/2400163003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:101: void ArcTraceBridge::OnTraceStart(bool success) { These two methods don't access members and can be moved to the anonymous namespace. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:44: if (delegate_) { nit: if (!delegate_) return; // rest of method. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:52: start_callback_ = callback; nit: You can avoid introducing |start_callback_| by binding the callback: namespace { void OnStarted(const StartAgentTracingCallback& callback, dbus::Response* response) { if (!response) { LOG(ERROR) << "Failed to request start"; } callback.Run(kArcTracingAgentName, response != nullptr); } } // namespace ... base::Bind(&OnStarted, callback); https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:55: void StopAgentTracing(const StopAgentTracingCallback& callback) override { nit: newline before. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:58: LOG(ERROR) << "Busy doing StopAgentTracing"; nit: maybe word this as "Previous StopAgentTracing() invocation is still pending." https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:91: void SetDelegate(Delegate* delegate) { delegate_ = delegate; } This should be marked as 'override'. Also add // ArcTraceAgent overrides: https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:113: } nit: newline between method definitions. Only skip newlines when you are listing override declarations. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:117: pipe_reader_->GetData(&pipe_data); Weren't you calling |pipe_reader_.reset()| before? https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:133: Delegate* delegate_; Who owns this? https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:43: ArcTraceAgent() {} nit: ArcTraceAgent(); and then ArcTraceAgent() = default; in the .cc. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... File chromeos/dbus/fake_arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... chromeos/dbus/fake_arc_trace_agent.cc:37: base::ThreadTaskRunnerHandle::Get()->PostTask( callback.Run(GetTracingAgentName(), true /* success */); (or comment why you need to use PostTask). https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... File chromeos/dbus/fake_arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... chromeos/dbus/fake_arc_trace_agent.h:17: FakeArcTraceAgent() {} nit: only declare the ctor and dtor here. Use = default for both in the .cc https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... chromeos/dbus/fake_arc_trace_agent.h:20: void SetDelegate(Delegate* delegate) override; nit: // ArcTraceAgent overrides: (and no newline between the method declarations).
https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/160001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:85: base::ScopedFD pipe_write_end = pipe_reader_->StartIO(); On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > On 2016/12/28 09:15:15, shunhsingou wrote: > > On 2016/12/27 19:15:59, Luis Héctor Chávez wrote: > > > What thread is this supposed to be run on? Is I/O allowed in that thread? > > > > It's running on 'stop_agent_tracing_task_runner_', which is set in > > tracing_controller_impl.cc as a blocking thread. > > Can you add comments about which methods run on which threads? Done. https://codereview.chromium.org/2400163003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:101: void ArcTraceBridge::OnTraceStart(bool success) { On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > These two methods don't access members and can be moved to the anonymous > namespace. Done. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:44: if (delegate_) { On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > nit: > > if (!delegate_) > return; > > // rest of method. Done, and also pass false to the callback in this situation. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:52: start_callback_ = callback; On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > nit: You can avoid introducing |start_callback_| by binding the callback: > > namespace { > > void OnStarted(const StartAgentTracingCallback& callback, > dbus::Response* response) { > if (!response) { > LOG(ERROR) << "Failed to request start"; > } > callback.Run(kArcTracingAgentName, response != nullptr); > } > > } // namespace > > ... > > base::Bind(&OnStarted, callback); Done. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:55: void StopAgentTracing(const StopAgentTracingCallback& callback) override { On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > nit: newline before. Done. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:58: LOG(ERROR) << "Busy doing StopAgentTracing"; On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > nit: maybe word this as "Previous StopAgentTracing() invocation is still > pending." Done. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:91: void SetDelegate(Delegate* delegate) { delegate_ = delegate; } On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > This should be marked as 'override'. Also add > > // ArcTraceAgent overrides: Done. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:113: } On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > nit: newline between method definitions. Only skip newlines when you are listing > override declarations. Done. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:117: pipe_reader_->GetData(&pipe_data); On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > Weren't you calling |pipe_reader_.reset()| before? Done. Add it back. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:133: Delegate* delegate_; On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > Who owns this? Added comment here. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:43: ArcTraceAgent() {} On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > nit: ArcTraceAgent(); > > and then > > ArcTraceAgent() = default; > > in the .cc. Done. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... File chromeos/dbus/fake_arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... chromeos/dbus/fake_arc_trace_agent.cc:37: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > callback.Run(GetTracingAgentName(), true /* success */); > > (or comment why you need to use PostTask). Added a comment here. I think this is something need to be modified in the tracing_controller_impl.cc. It expects callback to be called after StartAgentTracing() and its caller function returns. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... File chromeos/dbus/fake_arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... chromeos/dbus/fake_arc_trace_agent.h:17: FakeArcTraceAgent() {} On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > nit: only declare the ctor and dtor here. Use = default for both in the .cc Done. https://codereview.chromium.org/2400163003/diff/200001/chromeos/dbus/fake_arc... chromeos/dbus/fake_arc_trace_agent.h:20: void SetDelegate(Delegate* delegate) override; On 2016/12/28 17:21:58, Luis Héctor Chávez wrote: > nit: > > // ArcTraceAgent overrides: > > (and no newline between the method declarations). Done.
Sorry for the delay. Was on vacation. Some cleanup requests below, but overall lg: https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:132: arc_service_manager_->AddService( Can you instead do something like this? auto trace = base::MakeUnique<ArcTraceBridge>(arc_bridge_service)); DBusThreadManager::Get()->GetArcTraceAgent()->SetDelegate(trace.get()); arc_service_manager_->AddService(std::move(trace)); https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:152: // Destroy in the reverse order of the initialization. ..and DBusThreadManager::Get()->GetArcTraceAgent()->SetDelegate(nullptr); here. https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:21: ArcTraceBridge* g_arc_trace_bridge = nullptr; same, please remove if possible. https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:60: auto* trace_instance = arc_bridge_service()->trace()->GetInstanceForMethod( The API for getting an instance has been changed. See https://codereview.chromium.org/2599673005/. https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:9: #include <utility> unnecessary? https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:30: // Gets the singleton of this class. I think you can remove the singleton getter if you set the delegate in ArcServiceLauncher::Initialize(). https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:434: DBusThreadManager::Get()->GetArcTraceAgent()->SetDelegate( Can you move this to ArcServiceLauncher::Initialize()? The current code has a hidden dependency between line 432 and 434, which may incur a maintenance burden. https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:8: #include <utility> unused? https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:13: #include "base/memory/ref_counted.h" remove (see my comment on the header side) https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:55: // delegate_ may be nullptr if ARC is not enabled on the system. In such Apparently delegate_ is not initialized in the ctor? Please do. https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:88: stop_callback_ = callback; Can't we pass the |callback| directly to OnIOComplete at L94? (Is the check at L76 important here?) https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:37: scoped_refptr<base::TaskRunner> task_runner) = 0; #include base/memory/ref_counted.h ? https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:39: static ArcTraceAgent* Create(); What about returning a unique_ptr? Since the object will be held as a unique_ptr anyway, I don't think we need to use a raw pointer here. https://codereview.chromium.org/2400163003/diff/240001/components/arc/arc_bri... File components/arc/arc_bridge_host_impl.cc (right): https://codereview.chromium.org/2400163003/diff/240001/components/arc/arc_bri... components/arc/arc_bridge_host_impl.cc:209: void ArcBridgeHostImpl::OnTraceInstanceReady( same, sort https://codereview.chromium.org/2400163003/diff/240001/components/arc/arc_bri... File components/arc/arc_bridge_host_impl.h (right): https://codereview.chromium.org/2400163003/diff/240001/components/arc/arc_bri... components/arc/arc_bridge_host_impl.h:76: void OnTraceInstanceReady(mojom::TraceInstancePtr trace_ptr) override; please sort https://codereview.chromium.org/2400163003/diff/240001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/240001/components/arc/common/... components/arc/common/trace.mojom:8: remove this line https://codereview.chromium.org/2400163003/diff/240001/components/arc/common/... components/arc/common/trace.mojom:11: // Query available tracing categories in the container. Queries (L11), Starts (L14), and Stops (L17) https://codereview.chromium.org/2400163003/diff/240001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2400163003/diff/240001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:865: DCHECK_EQ(pending_clock_sync_ack_count_, 0); DCHECK_EQ(0, ...);
Please also update the URL for the platform/cheets-scripts patch.
As discussed in https://docs.google.com/document/d/1VhM-VEdAFi6JS96VEj6-MFVGs4bpWELDpqi0lPlYi.... I will try to get rid of dbus (debugd) first.
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738292 (Android framework/base) - http://ag/1738271 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/272648/ (ChromiumOS platform/cheets-scripts) - https://chromium-review.googlesource.com/#/c/422750 (ChromiumOS platform2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/313679/ (ChromiumOS platform/cheets-scripts) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
shunhsingou@google.com changed reviewers: + shunhsingou@google.com
Finally done moving the read end from debugd to the native service in Android. https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:132: arc_service_manager_->AddService( On 2017/01/04 23:55:47, Yusuke Sato wrote: > Can you instead do something like this? > > auto trace = base::MakeUnique<ArcTraceBridge>(arc_bridge_service)); > DBusThreadManager::Get()->GetArcTraceAgent()->SetDelegate(trace.get()); > arc_service_manager_->AddService(std::move(trace)); I move this to the ctor of ArcTraceBridge https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:152: // Destroy in the reverse order of the initialization. On 2017/01/04 23:55:47, Yusuke Sato wrote: > ..and DBusThreadManager::Get()->GetArcTraceAgent()->SetDelegate(nullptr); here. Moved to the dtor of ArcTraceBridge https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:21: ArcTraceBridge* g_arc_trace_bridge = nullptr; On 2017/01/04 23:55:47, Yusuke Sato wrote: > same, please remove if possible. Done. https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:60: auto* trace_instance = arc_bridge_service()->trace()->GetInstanceForMethod( On 2017/01/04 23:55:47, Yusuke Sato wrote: > The API for getting an instance has been changed. See > https://codereview.chromium.org/2599673005/. Done. https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:9: #include <utility> On 2017/01/04 23:55:47, Yusuke Sato wrote: > unnecessary? Done. https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:30: // Gets the singleton of this class. On 2017/01/04 23:55:47, Yusuke Sato wrote: > I think you can remove the singleton getter if you set the delegate in > ArcServiceLauncher::Initialize(). Done. https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2400163003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:434: DBusThreadManager::Get()->GetArcTraceAgent()->SetDelegate( On 2017/01/04 23:55:47, Yusuke Sato wrote: > Can you move this to ArcServiceLauncher::Initialize()? The current code has a > hidden dependency between line 432 and 434, which may incur a maintenance > burden. Moved to the ctor of ArcTraceBridge https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:8: #include <utility> On 2017/01/04 23:55:47, Yusuke Sato wrote: > unused? Done. https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:13: #include "base/memory/ref_counted.h" On 2017/01/04 23:55:47, Yusuke Sato wrote: > remove (see my comment on the header side) It's only used in this cc file now. https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:55: // delegate_ may be nullptr if ARC is not enabled on the system. In such On 2017/01/04 23:55:47, Yusuke Sato wrote: > Apparently delegate_ is not initialized in the ctor? Please do. Done. https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.cc:88: stop_callback_ = callback; On 2017/01/04 23:55:47, Yusuke Sato wrote: > Can't we pass the |callback| directly to OnIOComplete at L94? (Is the check at > L76 important here?) Done. https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... File chromeos/dbus/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:37: scoped_refptr<base::TaskRunner> task_runner) = 0; On 2017/01/04 23:55:47, Yusuke Sato wrote: > #include base/memory/ref_counted.h ? This method is removed from here. https://codereview.chromium.org/2400163003/diff/240001/chromeos/dbus/arc_trac... chromeos/dbus/arc_trace_agent.h:39: static ArcTraceAgent* Create(); On 2017/01/04 23:55:47, Yusuke Sato wrote: > What about returning a unique_ptr? Since the object will be held as a unique_ptr > anyway, I don't think we need to use a raw pointer here. I use singleton.h here now https://codereview.chromium.org/2400163003/diff/240001/components/arc/arc_bri... File components/arc/arc_bridge_host_impl.cc (right): https://codereview.chromium.org/2400163003/diff/240001/components/arc/arc_bri... components/arc/arc_bridge_host_impl.cc:209: void ArcBridgeHostImpl::OnTraceInstanceReady( On 2017/01/04 23:55:47, Yusuke Sato wrote: > same, sort Done. https://codereview.chromium.org/2400163003/diff/240001/components/arc/arc_bri... File components/arc/arc_bridge_host_impl.h (right): https://codereview.chromium.org/2400163003/diff/240001/components/arc/arc_bri... components/arc/arc_bridge_host_impl.h:76: void OnTraceInstanceReady(mojom::TraceInstancePtr trace_ptr) override; On 2017/01/04 23:55:48, Yusuke Sato wrote: > please sort Done. https://codereview.chromium.org/2400163003/diff/240001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/240001/components/arc/common/... components/arc/common/trace.mojom:8: On 2017/01/04 23:55:48, Yusuke Sato wrote: > remove this line Done. https://codereview.chromium.org/2400163003/diff/240001/components/arc/common/... components/arc/common/trace.mojom:11: // Query available tracing categories in the container. On 2017/01/04 23:55:48, Yusuke Sato wrote: > Queries (L11), Starts (L14), and Stops (L17) Done.
https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:30: #include "chromeos/trace/arc_trace_agent.h" This seems to be unused. https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit: s/2016/2017/ in all files. https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.cc:31: void StartAgentTracing(const base::trace_event::TraceConfig& trace_config, Can you add DCHECK_CURRENTLY_ON(content::BrowserThread::UI); on all methods? Otherwise the access to |delegate_| would be racy. https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:24: const base::Callback<void(bool)>& callback) = 0; nit: Can you add something like using StartTracingCallback = base::Callback<void(bool)>; ? same for the one in StopTracing. https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... components/arc/common/trace.mojom:17: StopTracing@2() => (string report); What happens if the user lets the collection proceed for a very long time? Would there be any issues returning an arbitrarily long report? Will the other side have a circular buffer and start dropping events? Is it difficult to have StartTracing() return a handle to a pipe that streams the JSON chunks and have Chrome read from that pipe and hand over the chunks to the tracing controller? In any case, the comment should explain a bit more about what |report| contains.
https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:29: arc_bridge_service()->trace()->RemoveObserver(this); swap L29 and L30 to do the removal in the reversed order. https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:69: return; Shouldn't we run the |callback| with false in this case? https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:86: return; same https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:13: #include "base/threading/thread_checker.h" unused? https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:52: void OnCategoriesReady(const std::vector<std::string>& categories); const QueryAvailableCategoriesCallback& ? I think trace.mojom.h has the typedef. https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.cc:68: // This allows constructor and destructor to be private and usable only Then can you make them private: ? https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:21: public: add virtual ~Delegate() = default; to force the derived class(es)' destructor(s) to be virtual? https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:31: add virtual ~ArcTraceAgent() = default; ? https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:32: virtual void SetDelegate(Delegate* delegate) = 0; Please add a function document. What happens if |delegate| is nullptr, for example? https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... components/arc/common/arc_bridge.mojom:118: // Notifies Chrome that the TraceInstance interface is ready. Move to L108.
https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:30: #include "chromeos/trace/arc_trace_agent.h" On 2017/01/17 19:20:32, Luis Héctor Chávez wrote: > This seems to be unused. Done. https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:29: arc_bridge_service()->trace()->RemoveObserver(this); On 2017/01/17 22:17:20, Yusuke Sato wrote: > swap L29 and L30 to do the removal in the reversed order. Done. https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:69: return; On 2017/01/17 22:17:20, Yusuke Sato wrote: > Shouldn't we run the |callback| with false in this case? Done. https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:86: return; On 2017/01/17 22:17:20, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.h (right): https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/17 19:20:32, Luis Héctor Chávez wrote: > nit: s/2016/2017/ in all files. Done. https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:13: #include "base/threading/thread_checker.h" On 2017/01/17 22:17:21, Yusuke Sato wrote: > unused? removed https://codereview.chromium.org/2400163003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.h:52: void OnCategoriesReady(const std::vector<std::string>& categories); On 2017/01/17 22:17:21, Yusuke Sato wrote: > const QueryAvailableCategoriesCallback& ? I think trace.mojom.h has the typedef. This is the implementation of QueryAvailableCategoriesCallback. What do you mean about using QueryAvailableCategoriesCallback? https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.cc (right): https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.cc:31: void StartAgentTracing(const base::trace_event::TraceConfig& trace_config, On 2017/01/17 19:20:32, Luis Héctor Chávez wrote: > Can you add > > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > on all methods? Otherwise the access to |delegate_| would be racy. Add thread_checker here as we don't want to add dependency to "content" https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.cc:68: // This allows constructor and destructor to be private and usable only On 2017/01/17 22:17:21, Yusuke Sato wrote: > Then can you make them private: ? Done. https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:21: public: On 2017/01/17 22:17:21, Yusuke Sato wrote: > add > virtual ~Delegate() = default; > to force the derived class(es)' destructor(s) to be virtual? Done. https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:24: const base::Callback<void(bool)>& callback) = 0; On 2017/01/17 19:20:32, Luis Héctor Chávez wrote: > nit: Can you add something like > > using StartTracingCallback = base::Callback<void(bool)>; > > ? same for the one in StopTracing. Done. https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:31: On 2017/01/17 22:17:21, Yusuke Sato wrote: > add > virtual ~ArcTraceAgent() = default; > ? Done. https://codereview.chromium.org/2400163003/diff/280001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:32: virtual void SetDelegate(Delegate* delegate) = 0; On 2017/01/17 22:17:21, Yusuke Sato wrote: > Please add a function document. What happens if |delegate| is nullptr, for > example? Done. https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... components/arc/common/arc_bridge.mojom:118: // Notifies Chrome that the TraceInstance interface is ready. On 2017/01/17 22:17:21, Yusuke Sato wrote: > Move to L108. Done. https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... components/arc/common/trace.mojom:17: StopTracing@2() => (string report); On 2017/01/17 19:20:32, Luis Héctor Chávez wrote: > What happens if the user lets the collection proceed for a very long time? Would > there be any issues returning an arbitrarily long report? Will the other side > have a circular buffer and start dropping events? Is it difficult to have > StartTracing() return a handle to a pipe that streams the JSON chunks and have > Chrome read from that pipe and hand over the chunks to the tracing controller? > > In any case, the comment should explain a bit more about what |report| contains. I added more comment here. Actually I tried to used a handle as following during my developing. // in trace.mojom StopTracing@2(handle report_write_handle) // in arc_trace_bridge.cc int fds[2]; DCHECK_EQ(0, pipe(fds)); ... trace_instance->StopTracing(mojo::WrapPlatformFile(fds[1])); ... So we can write report through the pipe. However, in Android, StopTracing() of trace instance (ArcTraceService) never get called. If I set ArcTraceService to permissive mode than the problem get solved, so I think there is something wrong in my sepolicy settings. However, I could not find any logs about this in logcat and dmesg and don't know how to debug it. Did you see this issue before? In the end I choose to use a string. As mentioned in the comment, the size of the report is limited in ArcTraceService by 16MB.
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - https://chrome-internal-review.googlesource.com/#/c/313679/ (ChromiumOS platform/cheets-scripts) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/280001/components/arc/common/... components/arc/common/trace.mojom:17: StopTracing@2() => (string report); On 2017/01/18 09:13:21, shunhsingou1 wrote: > On 2017/01/17 19:20:32, Luis Héctor Chávez wrote: > > What happens if the user lets the collection proceed for a very long time? > Would > > there be any issues returning an arbitrarily long report? Will the other side > > have a circular buffer and start dropping events? Is it difficult to have > > StartTracing() return a handle to a pipe that streams the JSON chunks and have > > Chrome read from that pipe and hand over the chunks to the tracing controller? > > > > In any case, the comment should explain a bit more about what |report| > contains. > > I added more comment here. > > Actually I tried to used a handle as following during my developing. > > // in trace.mojom > StopTracing@2(handle report_write_handle) > > // in arc_trace_bridge.cc > int fds[2]; > DCHECK_EQ(0, pipe(fds)); > ... > trace_instance->StopTracing(mojo::WrapPlatformFile(fds[1])); > ... > > So we can write report through the pipe. > > However, in Android, StopTracing() of trace instance (ArcTraceService) never get > called. > > If I set ArcTraceService to permissive mode than the problem get solved, so I > think there is something wrong in my sepolicy settings. However, I could not > find any logs about this in logcat and dmesg and don't know how to debug it. > > Did you see this issue before? > > In the end I choose to use a string. As mentioned in the comment, the size of > the report is limited in ArcTraceService by 16MB. Depending on the context in which an SELinux denial happens, it will get reported to different places. Some of them are logged in /var/log/messages instead of the other two locations. Let's follow up offline to fix the underlying issue instead of resorting to workarounds. The reason I'd prefer to have Chrome handle this is to centralize everything and have a potentially simpler implementation, so that events can be bumped out of the circular buffer more or less in order. Otherwise, a good chunk of Chrome-side events will have a very large probability of getting evicted if the 16MB comes at once from Android, which will be very confusing for users since they'll lose some correlation. https://codereview.chromium.org/2400163003/diff/320001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/320001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:22: typedef base::Callback<void(bool)> StartTracingCallback; Is there any reason you're using typedef instead of using like I requested? using is preferred in Chromium code: https://chromium-cpp.appspot.com/#core-whitelist
https://codereview.chromium.org/2400163003/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:91: callback.Run(""); So PostTask is not needed here right? Can you document that? https://codereview.chromium.org/2400163003/diff/320001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/320001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:25: ~Delegate() = default; virtual
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - http://ag/1797970 (Android frameworks/base) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
Done fixing nits. Still trying to send FD to through Mojo so we can deal with circular buffer in Chrome side. https://codereview.chromium.org/2400163003/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:91: callback.Run(""); On 2017/01/19 23:06:18, Yusuke Sato wrote: > So PostTask is not needed here right? Can you document that? Done. https://codereview.chromium.org/2400163003/diff/320001/chromeos/trace/arc_tra... File chromeos/trace/arc_trace_agent.h (right): https://codereview.chromium.org/2400163003/diff/320001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:22: typedef base::Callback<void(bool)> StartTracingCallback; On 2017/01/18 17:31:32, Luis Héctor Chávez wrote: > Is there any reason you're using typedef instead of using like I requested? > using is preferred in Chromium code: > https://chromium-cpp.appspot.com/#core-whitelist Done. Changed to using. https://codereview.chromium.org/2400163003/diff/320001/chromeos/trace/arc_tra... chromeos/trace/arc_trace_agent.h:25: ~Delegate() = default; On 2017/01/19 23:06:19, Yusuke Sato wrote: > virtual Done.
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing This CL creates a mojo interface for Chrome to start tracing in the ARC container in Chrome OS, and read the tracing resuld from debugd. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - http://ag/1797970 (Android frameworks/base) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android framework tracing in chrome://tracing For enabling tracing from chrome://tracing, we create a Mojo interface for host side to trigger tracing in Android. A socket is used for the client to send back the trace event. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - http://ag/1797970 (Android frameworks/base) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
i'm a bit worried about the fact that we're now parsing stuff in Chrome. That'll need special attention from the security reviewers (I won't add them until we're more or less okay with the rest of the design). first round of comments, let me think a bit more about the lifetimes of the various objects. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:307: "arc/trace/arc_trace_reader.cc", I didn't see anything that warrants the reader to be in chrome/browser/chromeos. Can you move it to components/arc/trace? https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:43: void ArcTraceBridge::OnInstanceReady() { nit: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:46: DCHECK_EQ(0, ret); this will be messy on non-debug builds since you'll pass garbage as fds to the rest of the system. It's better to replace this with/add: if (ret < 0) { LOG(ERROR) << "..."; return; } https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:54: trace_instance->SetHostSocket(std::move(handle)); nit: trace_instance->SetHostSocket(mojo::WrapPlatformFile(fds[1])); seems to be more readable and still fits. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:76: categories_.push_back( nit: you can use emplace_back() here. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:126: LOG(WARNING) << "Failed to stop tracing in ARC."; nit: LOG_IF(WARNING, !success) << "..."; https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_reader.cc (right): https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:27: #define ARC_TRACE_MESSAGE_LENGTH (1024 + 512) constexpr size_t kArcTraceMessageLength = 1024 + 512; https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:35: : is_recording_(false), input_fd_(-1), recording_thread_(nullptr) {} these three can be set in the .h, that way you can have ArcTraceReader() = default; also, the convention is to not explicitly initialize unique_ptrs with nullptr. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:96: recording_thread_.reset( How about using base::FileDescriptorWatcher::WatchReadable()[1] instead? That'll remove all the select() code and make everything simpler. https://cs.chromium.org/chromium/src/base/files/file_descriptor_watcher_posix... https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:111: recording_thread_.reset(nullptr); nit: recording_thread_.reset(); https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:113: // TODO(shunshingou): run the following in a separated thread to avoid long This must be done in this CL, unfortunately. Worst case scenario, you can just close the pipe in this end and make sure you block SIGPIPE on the other end. Btw, by using the WatchReadable() and by streaming events as they come, you won't need to join a thread at all! https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:153: // Bad data, shouldn't happen as the data is prepared by ourself. nit: ourselves. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:158: current_data_size_ += json.size(); Why do you still need this? Why can't you just send it to the tracing processor and let it deal with buffers? Doing it this way will effectively double the amount of memory needed. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:167: void ArcTraceReader::ParseAndSaveData(std::string data) { This might fail since |data| might contain multiple lines if you're not reading fast enough. Even worse, you might end up with one line plus a chunk of a second line, and the next entry will be read incorrectly. You'll need to add some buffering to ensure that you're reading this line by line. https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... components/arc/common/trace.mojom:15: SetHostSocket@3(handle sock); Why not send this in StartTracing()?
https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_reader.cc (right): https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:113: // TODO(shunshingou): run the following in a separated thread to avoid long On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > This must be done in this CL, unfortunately. Worst case scenario, you can just > close the pipe in this end and make sure you block SIGPIPE on the other end. D'Oh, this is a socket, not a pipe. Ignore the SIGPIPE part. > > Btw, by using the WatchReadable() and by streaming events as they come, you > won't need to join a thread at all!
https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_reader.cc (right): https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:66: ret = TEMP_FAILURE_RETRY(read(input_fd_.get(), buf, sizeof(buf) - 1)); in any case, you should try to limit the amount of raw UNIX calls. Try to use the very nice classes in base/ :) https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:96: recording_thread_.reset( On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > How about using base::FileDescriptorWatcher::WatchReadable()[1] instead? That'll > remove all the select() code and make everything simpler. > > https://cs.chromium.org/chromium/src/base/files/file_descriptor_watcher_posix... You can also use something similar to https://cs.chromium.org/chromium/src/chromeos/dbus/pipe_reader.h, which seems to be a bit simpler to use. in any case, you should try to use the browser's blocking pool (probably passed in as a class parameter if you end up moving it to components/arc) https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:158: current_data_size_ += json.size(); On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > Why do you still need this? Why can't you just send it to the tracing processor > and let it deal with buffers? Doing it this way will effectively double the > amount of memory needed. Ignore the above comment, since it's the way the API is designed. I should have read the code for TracingAgent instead of relying on my (flawed) understanding of what the trace does just based on the UI. Apologies for any confusion. Having said that, can you please try to use https://cs.chromium.org/chromium/src/base/trace_event/trace_buffer.h instead of rolling your own ring buffer?
Description was changed from ========== arc: enable Android framework tracing in chrome://tracing For enabling tracing from chrome://tracing, we create a Mojo interface for host side to trigger tracing in Android. A socket is used for the client to send back the trace event. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - http://ag/1797970 (Android frameworks/base) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android tracing in verified-boot mode In verified-boot mode, ftrace trace_marker is not allowed for writting due to security concern. As an altenative, we use a socket for processes in Android to write trace events, and send back the trace data via Mojo handle. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - http://ag/1797970 (Android frameworks/base) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
Description was changed from ========== arc: enable Android tracing in verified-boot mode In verified-boot mode, ftrace trace_marker is not allowed for writting due to security concern. As an altenative, we use a socket for processes in Android to write trace events, and send back the trace data via Mojo handle. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - http://ag/1797970 (Android frameworks/base) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android tracing in verified-boot mode In verified-boot mode, ftrace trace_marker is not allowed for writting due to security concern. As an altenative, we use a socket for processes in Android to write trace events, and send back the trace data via Mojo handle. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - http://ag/1797970 (Android frameworks/base) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
Move the tracing trigger part to crrev.com/2699833003 so we can work on that first. The code for tracing in verified-boot mode is left here.
Removing self as reviewer as there are plenty of eyes on this
https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_reader.cc (right): https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:113: // TODO(shunshingou): run the following in a separated thread to avoid long On 2017/02/10 17:48:03, Luis Héctor Chávez wrote: > On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > > This must be done in this CL, unfortunately. Worst case scenario, you can just > > close the pipe in this end and make sure you block SIGPIPE on the other end. > > D'Oh, this is a socket, not a pipe. Ignore the SIGPIPE part. > > > > > Btw, by using the WatchReadable() and by streaming events as they come, you > > won't need to join a thread at all! > The comment here is for the processing of stringstream below. It may takes some time to pop the buffer and generate the report if the buffer is pretty large. (but in currently size (16MB) it's usually quite fast) For the above join(), the worst case take about 1s (blocked by select()), but it's true that I should use WatchReadable or other tools under base/ to avoid many system calls here. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:158: current_data_size_ += json.size(); On 2017/02/10 23:16:51, Luis Héctor Chávez wrote: > On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > > Why do you still need this? Why can't you just send it to the tracing > processor > > and let it deal with buffers? Doing it this way will effectively double the > > amount of memory needed. > > Ignore the above comment, since it's the way the API is designed. I should have > read the code for TracingAgent instead of relying on my (flawed) understanding > of what the trace does just based on the UI. Apologies for any confusion. > > Having said that, can you please try to use > https://cs.chromium.org/chromium/src/base/trace_event/trace_buffer.h instead of > rolling your own ring buffer? trace_buffer seems to be designed for Chrome tracing itself (used by TraceLog), not for TracingAgent. For example, it has chunk design that is not really useful in our case. Other TracingAgent, e.g. etw_tracing_agent_win, power_tracing_agent, also maintain their own buffer. (Although they do not use ring buffer) https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:167: void ArcTraceReader::ParseAndSaveData(std::string data) { On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > This might fail since |data| might contain multiple lines if you're not reading > fast enough. Even worse, you might end up with one line plus a chunk of a second > line, and the next entry will be read incorrectly. > > You'll need to add some buffering to ensure that you're reading this line by > line. The data would only have one trace event since we're using SEQPACKET socket. https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... components/arc/common/trace.mojom:15: SetHostSocket@3(handle sock); On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > Why not send this in StartTracing()? I think we don't need to recreate the socket between host and client every time we start tracing. It's only called one time after the bridge is ready.
On 2017/02/16 09:56:46, Earl Ou wrote: > https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/trace/arc_trace_reader.cc (right): > > https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:113: // TODO(shunshingou): > run the following in a separated thread to avoid long > On 2017/02/10 17:48:03, Luis Héctor Chávez wrote: > > On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > > > This must be done in this CL, unfortunately. Worst case scenario, you can > just > > > close the pipe in this end and make sure you block SIGPIPE on the other end. > > > > D'Oh, this is a socket, not a pipe. Ignore the SIGPIPE part. > > > > > > > > Btw, by using the WatchReadable() and by streaming events as they come, you > > > won't need to join a thread at all! > > > > The comment here is for the processing of stringstream below. It may takes some > time to pop the buffer and generate the report if the buffer is pretty large. > (but in currently size (16MB) it's usually quite fast) > > For the above join(), the worst case take about 1s (blocked by select()), but > it's true that I should use WatchReadable or other tools under base/ to avoid > many system calls here. > > https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:158: current_data_size_ += > json.size(); > On 2017/02/10 23:16:51, Luis Héctor Chávez wrote: > > On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > > > Why do you still need this? Why can't you just send it to the tracing > > processor > > > and let it deal with buffers? Doing it this way will effectively double the > > > amount of memory needed. > > > > Ignore the above comment, since it's the way the API is designed. I should > have > > read the code for TracingAgent instead of relying on my (flawed) understanding > > of what the trace does just based on the UI. Apologies for any confusion. > > > > Having said that, can you please try to use > > https://cs.chromium.org/chromium/src/base/trace_event/trace_buffer.h instead > of > > rolling your own ring buffer? > > trace_buffer seems to be designed for Chrome tracing itself (used by TraceLog), > not for TracingAgent. For example, it has chunk design that is not really useful > in our case. Other TracingAgent, e.g. etw_tracing_agent_win, > power_tracing_agent, also maintain their own buffer. (Although they do not use > ring buffer) > > https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:167: void > ArcTraceReader::ParseAndSaveData(std::string data) { > On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > > This might fail since |data| might contain multiple lines if you're not > reading > > fast enough. Even worse, you might end up with one line plus a chunk of a > second > > line, and the next entry will be read incorrectly. > > > > You'll need to add some buffering to ensure that you're reading this line by > > line. > > The data would only have one trace event since we're using SEQPACKET socket. > > https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... > File components/arc/common/trace.mojom (right): > > https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... > components/arc/common/trace.mojom:15: SetHostSocket@3(handle sock); > On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > > Why not send this in StartTracing()? > > I think we don't need to recreate the socket between host and client every time > we start tracing. It's only called one time after the bridge is ready. Oops. Thanks for catching this. Uploaded another CL included the fix: http://crrev.com/2749283003
On 2017/03/15 14:04:34, shunhsingou wrote: > On 2017/02/16 09:56:46, Earl Ou wrote: > > > https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... > > File chrome/browser/chromeos/arc/trace/arc_trace_reader.cc (right): > > > > > https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... > > chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:113: // > TODO(shunshingou): > > run the following in a separated thread to avoid long > > On 2017/02/10 17:48:03, Luis Héctor Chávez wrote: > > > On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > > > > This must be done in this CL, unfortunately. Worst case scenario, you can > > just > > > > close the pipe in this end and make sure you block SIGPIPE on the other > end. > > > > > > D'Oh, this is a socket, not a pipe. Ignore the SIGPIPE part. > > > > > > > > > > > Btw, by using the WatchReadable() and by streaming events as they come, > you > > > > won't need to join a thread at all! > > > > > > > The comment here is for the processing of stringstream below. It may takes > some > > time to pop the buffer and generate the report if the buffer is pretty large. > > (but in currently size (16MB) it's usually quite fast) > > > > For the above join(), the worst case take about 1s (blocked by select()), but > > it's true that I should use WatchReadable or other tools under base/ to avoid > > many system calls here. > > > > > https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... > > chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:158: current_data_size_ > += > > json.size(); > > On 2017/02/10 23:16:51, Luis Héctor Chávez wrote: > > > On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > > > > Why do you still need this? Why can't you just send it to the tracing > > > processor > > > > and let it deal with buffers? Doing it this way will effectively double > the > > > > amount of memory needed. > > > > > > Ignore the above comment, since it's the way the API is designed. I should > > have > > > read the code for TracingAgent instead of relying on my (flawed) > understanding > > > of what the trace does just based on the UI. Apologies for any confusion. > > > > > > Having said that, can you please try to use > > > https://cs.chromium.org/chromium/src/base/trace_event/trace_buffer.h instead > > of > > > rolling your own ring buffer? > > > > trace_buffer seems to be designed for Chrome tracing itself (used by > TraceLog), > > not for TracingAgent. For example, it has chunk design that is not really > useful > > in our case. Other TracingAgent, e.g. etw_tracing_agent_win, > > power_tracing_agent, also maintain their own buffer. (Although they do not use > > ring buffer) > > > > > https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... > > chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:167: void > > ArcTraceReader::ParseAndSaveData(std::string data) { > > On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > > > This might fail since |data| might contain multiple lines if you're not > > reading > > > fast enough. Even worse, you might end up with one line plus a chunk of a > > second > > > line, and the next entry will be read incorrectly. > > > > > > You'll need to add some buffering to ensure that you're reading this line by > > > line. > > > > The data would only have one trace event since we're using SEQPACKET socket. > > > > > https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... > > File components/arc/common/trace.mojom (right): > > > > > https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... > > components/arc/common/trace.mojom:15: SetHostSocket@3(handle sock); > > On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > > > Why not send this in StartTracing()? > > > > I think we don't need to recreate the socket between host and client every > time > > we start tracing. It's only called one time after the bridge is ready. > > Oops. Thanks for catching this. > > Uploaded another CL included the fix: http://crrev.com/2749283003 BTW, is there any bot I can try to avoid such error.
> > Oops. Thanks for catching this. > > > > Uploaded another CL included the fix: http://crrev.com/2749283003 > > BTW, is there any bot I can try to avoid such error. Hmm... please ignore #65 and #66. I'm commenting on the wrong one...
https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:307: "arc/trace/arc_trace_reader.cc", On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > I didn't see anything that warrants the reader to be in chrome/browser/chromeos. > Can you move it to components/arc/trace? There is no arc_trace_reader.* now. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:43: void ArcTraceBridge::OnInstanceReady() { On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > nit: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Done. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:46: DCHECK_EQ(0, ret); On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > this will be messy on non-debug builds since you'll pass garbage as fds to the > rest of the system. > > It's better to replace this with/add: > > if (ret < 0) { > LOG(ERROR) << "..."; > return; > } Done in arc_tracing_agent.cc now. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:54: trace_instance->SetHostSocket(std::move(handle)); On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > nit: trace_instance->SetHostSocket(mojo::WrapPlatformFile(fds[1])); > > seems to be more readable and still fits. Done. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:76: categories_.push_back( On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > nit: you can use emplace_back() here. Done. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_bridge.cc:126: LOG(WARNING) << "Failed to stop tracing in ARC."; On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > nit: LOG_IF(WARNING, !success) << "..."; Done. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/trace/arc_trace_reader.cc (right): https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:27: #define ARC_TRACE_MESSAGE_LENGTH (1024 + 512) On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > constexpr size_t kArcTraceMessageLength = 1024 + 512; Done in Android side. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:35: : is_recording_(false), input_fd_(-1), recording_thread_(nullptr) {} On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > these three can be set in the .h, that way you can have ArcTraceReader() = > default; > > also, the convention is to not explicitly initialize unique_ptrs with nullptr. Done in Android side. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:66: ret = TEMP_FAILURE_RETRY(read(input_fd_.get(), buf, sizeof(buf) - 1)); On 2017/02/10 23:16:51, Luis Héctor Chávez wrote: > in any case, you should try to limit the amount of raw UNIX calls. Try to use > the very nice classes in base/ :) This is moved to the Android side. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:96: recording_thread_.reset( On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > How about using base::FileDescriptorWatcher::WatchReadable()[1] instead? That'll > remove all the select() code and make everything simpler. > > https://cs.chromium.org/chromium/src/base/files/file_descriptor_watcher_posix... Done in arc_tracing_agent.cc https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:111: recording_thread_.reset(nullptr); On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > nit: recording_thread_.reset(); No reading thread now as suggested. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:113: // TODO(shunshingou): run the following in a separated thread to avoid long On 2017/02/16 09:56:45, Earl Ou wrote: > On 2017/02/10 17:48:03, Luis Héctor Chávez wrote: > > On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > > > This must be done in this CL, unfortunately. Worst case scenario, you can > just > > > close the pipe in this end and make sure you block SIGPIPE on the other end. > > > > D'Oh, this is a socket, not a pipe. Ignore the SIGPIPE part. > > > > > > > > Btw, by using the WatchReadable() and by streaming events as they come, you > > > won't need to join a thread at all! > > > > The comment here is for the processing of stringstream below. It may takes some > time to pop the buffer and generate the report if the buffer is pretty large. > (but in currently size (16MB) it's usually quite fast) > > For the above join(), the worst case take about 1s (blocked by select()), but > it's true that I should use WatchReadable or other tools under base/ to avoid > many system calls here. > Now this is running in IO thread with WatchReadable. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:153: // Bad data, shouldn't happen as the data is prepared by ourself. On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > nit: ourselves. Done on the Android side. https://codereview.chromium.org/2400163003/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/trace/arc_trace_reader.cc:158: current_data_size_ += json.size(); On 2017/02/16 09:56:45, Earl Ou wrote: > On 2017/02/10 23:16:51, Luis Héctor Chávez wrote: > > On 2017/02/10 17:43:54, Luis Héctor Chávez wrote: > > > Why do you still need this? Why can't you just send it to the tracing > > processor > > > and let it deal with buffers? Doing it this way will effectively double the > > > amount of memory needed. > > > > Ignore the above comment, since it's the way the API is designed. I should > have > > read the code for TracingAgent instead of relying on my (flawed) understanding > > of what the trace does just based on the UI. Apologies for any confusion. > > > > Having said that, can you please try to use > > https://cs.chromium.org/chromium/src/base/trace_event/trace_buffer.h instead > of > > rolling your own ring buffer? > > trace_buffer seems to be designed for Chrome tracing itself (used by TraceLog), > not for TracingAgent. For example, it has chunk design that is not really useful > in our case. Other TracingAgent, e.g. etw_tracing_agent_win, > power_tracing_agent, also maintain their own buffer. (Although they do not use > ring buffer) I'm using TraceBuffer in the new patch now. https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... File components/arc/common/trace.mojom (right): https://codereview.chromium.org/2400163003/diff/380001/components/arc/common/... components/arc/common/trace.mojom:15: SetHostSocket@3(handle sock); On 2017/02/16 09:56:46, Earl Ou wrote: > On 2017/02/10 17:43:55, Luis Héctor Chávez wrote: > > Why not send this in StartTracing()? > > I think we don't need to recreate the socket between host and client every time > we start tracing. It's only called one time after the bridge is ready. Move this to StartTracing now.
Description was changed from ========== arc: enable Android tracing in verified-boot mode In verified-boot mode, ftrace trace_marker is not allowed for writting due to security concern. As an altenative, we use a socket for processes in Android to write trace events, and send back the trace data via Mojo handle. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) - http://ag/1797970 (Android frameworks/base) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android tracing in verified-boot mode In verified-boot mode, ftrace trace_marker is not allowed for writting due to security concern. As an altenative, we use a socket for processes in Android to write trace events, and send back the trace data via Mojo handle. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
first round https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:34: constexpr int kArcTraceMessageLength = 1024 + 512; nit: size_t https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:39: constexpr int kTraceEventChunks = nit: size_t https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:42: bool CreateSocketPair(base::ScopedFD* one, base::ScopedFD* two) { I see that you copied this from base/posix/unix_domain_socket_linux.cc . Can you try to declare it in the .h file (in another change) to avoid repetition? https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:71: CreateSocketPair(&read_fd, &write_fd); Make this fail if CreateSocketPair() returns false. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:72: data_file_.reset(new base::File(read_fd.release())); nit: data_file_ = base::MakeUnique<base::File>(read_fd.release()); https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:90: callback_ = callback; You don't need to do this. You can instead do delegate_->StopTracing( base::Bind(&ArcTracingAgentImpl::OnArcTracingStopped, weak_ptr_factory_.GetWeakPtr(), callback)); and add the |const StopAgentTracingCallback& callback| param to OnArcTracingStopped() as the first parameter. Do the same thing for StopTracingOnIOThread(). https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:91: if (delegate_) nit: you cannot elide braces since the body spans more than one line. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:119: chunk_.reset(nullptr); nit: chunk_.reset(); https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:123: void OnTraceDataAvailable() { Can you move all the buffer-related stuff to another class? Make all the methods of that class have DCHECK_CURRENTLY_ON(BrowserThread::IO) so that way it's easier to reason about what data / methods must live in which threads. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:127: data_file_->ReadAtCurrentPosNoBestEffort(buf, kArcTraceMessageLength); I'm a bit worried that we might need to use recv() instead of read() here in case of zero-length datagrams. Can you use base::UnixDomainSocket::RecvMsg() instead of this? https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:128: if (n == 0) What about errors? https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:132: char* data = new char[n + 1]; you're leaking this :( https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:141: // Here we don't actually parse the data, which should already be a valid This is a dangerous assumption :( What happens if somebody manages to push corrupt JSON data across this interface?
https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:180: close(read_fd_.release()); nit: read_fd_.reset(); https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:182: fd_watcher_.reset(nullptr); nit: fd_watcher_.reset();
https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:133: memcpy(data, buf, n + 1); You need to add if (n > kArcTraceMessageLength) { LOG(ERROR) << "..."; return; } prior to this memcpy.
https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:34: constexpr int kArcTraceMessageLength = 1024 + 512; On 2017/03/22 17:04:58, Luis Héctor Chávez wrote: > nit: size_t Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:39: constexpr int kTraceEventChunks = On 2017/03/22 17:04:57, Luis Héctor Chávez wrote: > nit: size_t Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:42: bool CreateSocketPair(base::ScopedFD* one, base::ScopedFD* two) { On 2017/03/22 17:04:57, Luis Héctor Chávez wrote: > I see that you copied this from base/posix/unix_domain_socket_linux.cc . Can you > try to declare it in the .h file (in another change) to avoid repetition? Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:71: CreateSocketPair(&read_fd, &write_fd); On 2017/03/22 17:04:58, Luis Héctor Chávez wrote: > Make this fail if CreateSocketPair() returns false. Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:72: data_file_.reset(new base::File(read_fd.release())); On 2017/03/22 17:04:57, Luis Héctor Chávez wrote: > nit: data_file_ = base::MakeUnique<base::File>(read_fd.release()); This data member is removed. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:90: callback_ = callback; On 2017/03/22 17:04:57, Luis Héctor Chávez wrote: > You don't need to do this. You can instead do > > delegate_->StopTracing( > base::Bind(&ArcTracingAgentImpl::OnArcTracingStopped, > weak_ptr_factory_.GetWeakPtr(), > callback)); > > and add the |const StopAgentTracingCallback& callback| param to > OnArcTracingStopped() as the first parameter. Do the same thing for > StopTracingOnIOThread(). Done. My original thought is that we still need another boolean flag, e.g., is_stopping, to check if we're currently stopping tracing, and it can be done by callback_.is_null(). But good to remove it as we it's more clear. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:91: if (delegate_) On 2017/03/22 17:04:58, Luis Héctor Chávez wrote: > nit: you cannot elide braces since the body spans more than one line. Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:119: chunk_.reset(nullptr); On 2017/03/22 17:04:58, Luis Héctor Chávez wrote: > nit: chunk_.reset(); Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:123: void OnTraceDataAvailable() { On 2017/03/22 17:04:58, Luis Héctor Chávez wrote: > Can you move all the buffer-related stuff to another class? Make all the methods > of that class have DCHECK_CURRENTLY_ON(BrowserThread::IO) so that way it's > easier to reason about what data / methods must live in which threads. Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:127: data_file_->ReadAtCurrentPosNoBestEffort(buf, kArcTraceMessageLength); On 2017/03/22 17:04:58, Luis Héctor Chávez wrote: > I'm a bit worried that we might need to use recv() instead of read() here in > case of zero-length datagrams. Can you use base::UnixDomainSocket::RecvMsg() > instead of this? Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:128: if (n == 0) On 2017/03/22 17:04:58, Luis Héctor Chávez wrote: > What about errors? Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:132: char* data = new char[n + 1]; On 2017/03/22 17:04:57, Luis Héctor Chávez wrote: > you're leaking this :( Done. It's now free in StopTracing. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:133: memcpy(data, buf, n + 1); On 2017/03/22 18:23:04, Luis Héctor Chávez wrote: > You need to add > > if (n > kArcTraceMessageLength) { > LOG(ERROR) << "..."; > return; > } > > prior to this memcpy. Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:141: // Here we don't actually parse the data, which should already be a valid On 2017/03/22 17:04:57, Luis Héctor Chávez wrote: > This is a dangerous assumption :( What happens if somebody manages to push > corrupt JSON data across this interface? Done add a validation check here. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:180: close(read_fd_.release()); On 2017/03/22 18:09:34, Luis Héctor Chávez wrote: > nit: read_fd_.reset(); Done. https://codereview.chromium.org/2400163003/diff/420001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:182: fd_watcher_.reset(nullptr); On 2017/03/22 18:09:34, Luis Héctor Chávez wrote: > nit: fd_watcher_.reset(); Done.
https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:53: void StartTracing(int read_fd) { As mentioned below, make this a ScopedFD. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:68: char buf[kArcTraceMessageLength + 1]; nit: You don't need the +1 here. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:77: LOG(WARNING) << "Unexpected error while reading trace from client: " nit: PLOG(WARNING) << "Unexpected error while reading trace from client."; PLOG already prints out strerror(errno). https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:84: if (n > (ssize_t)kArcTraceMessageLength) { Avoid C-style casts. static_cast<ssize_t>(kArcTraceMessageLength) https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:89: char* data = new char[n + 1]; avoid doing this as much as possible. Instead do: std::unique_ptr<char[]> data = base::MakeUnique<char[]>(n + 1); https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:105: LOG(WARNING) << "Get incorrect trace data from container: " << data; I'd probably not log the data because it might be too large. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:145: delete[] event->name(); Eek. What happens if the chunk is evicted because the size exceeds the buffer size? (I'm guessing memory leaks). https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:148: callback.Run(base::RefCountedString::TakeString(&data)); Keep in mind that this will make OnTracingReaderStopped be run in the IO thread, which is probably not what you want. Please post a task in the UI thread to run the callback correctly. Alternatively, try to do: base::RefCountedString StopTracing() { // ... return base::RefCountedString::TakeString(&data); } // ... bool PostTaskAndReplyWithResult( // the IO thread runner, FROM_HERE, base::Bind(&ArcTracingReader::StopTracing, reader_->GetWeakPtr()), base::Bind(&ArcTracingAgentImpl::OnTracingReaderStopped, weak_ptr_factory_.GetWeakPtr(), callback))); https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:180: bool success = delegate_ != nullptr; Creating a socket pair when ARC is not enabled seems wasteful. Please keep the old semantics: if (!delegate_) { base::Thread... return; } base::ScopedFD write_fd, read_fd; if (!CreateSocketPair(&read_fd, &write_fd)) { base::Thread... return; } https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:196: read_fd.release())); you can do base::Passed(std::move(read_fd)) instead of read_fd.release(). Thay way StartTracing can receive a base::ScopedFD instead of a raw int. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:237: bool success) { DCHECK_CURRENTLY_ON(BrowserThread::UI);? https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:255: const scoped_refptr<base::RefCountedString>& data) { DCHECK_CURRENTLY_ON(BrowserThread::UI);? https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:261: std::unique_ptr<ArcTracingReader> reader_ = nit: move the initialization to the constructor.
https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:53: void StartTracing(int read_fd) { On 2017/04/04 17:28:35, Luis Héctor Chávez wrote: > As mentioned below, make this a ScopedFD. Done. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:77: LOG(WARNING) << "Unexpected error while reading trace from client: " On 2017/04/04 17:28:35, Luis Héctor Chávez wrote: > nit: PLOG(WARNING) << "Unexpected error while reading trace from client."; > > PLOG already prints out strerror(errno). Done. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:84: if (n > (ssize_t)kArcTraceMessageLength) { On 2017/04/04 17:28:35, Luis Héctor Chávez wrote: > Avoid C-style casts. static_cast<ssize_t>(kArcTraceMessageLength) Done. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:89: char* data = new char[n + 1]; On 2017/04/04 17:28:34, Luis Héctor Chávez wrote: > avoid doing this as much as possible. Instead do: > > std::unique_ptr<char[]> data = base::MakeUnique<char[]>(n + 1); Done. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:105: LOG(WARNING) << "Get incorrect trace data from container: " << data; On 2017/04/04 17:28:34, Luis Héctor Chávez wrote: > I'd probably not log the data because it might be too large. Done. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:145: delete[] event->name(); On 2017/04/04 17:28:35, Luis Héctor Chávez wrote: > Eek. What happens if the chunk is evicted because the size exceeds the buffer > size? (I'm guessing memory leaks). Right. It leaks here. I added a checking code when we get reused event. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:148: callback.Run(base::RefCountedString::TakeString(&data)); On 2017/04/04 17:28:34, Luis Héctor Chávez wrote: > Keep in mind that this will make OnTracingReaderStopped be run in the IO thread, > which is probably not what you want. Please post a task in the UI thread to run > the callback correctly. Alternatively, try to do: > > base::RefCountedString StopTracing() { > // ... > return base::RefCountedString::TakeString(&data); > } > > // ... > > bool PostTaskAndReplyWithResult( > // the IO thread runner, > FROM_HERE, > base::Bind(&ArcTracingReader::StopTracing, reader_->GetWeakPtr()), > base::Bind(&ArcTracingAgentImpl::OnTracingReaderStopped, > weak_ptr_factory_.GetWeakPtr(), callback))); Done. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:180: bool success = delegate_ != nullptr; On 2017/04/04 17:28:34, Luis Héctor Chávez wrote: > Creating a socket pair when ARC is not enabled seems wasteful. Please keep the > old semantics: > > if (!delegate_) { > base::Thread... > return; > } > > base::ScopedFD write_fd, read_fd; > if (!CreateSocketPair(&read_fd, &write_fd)) { > base::Thread... > return; > } It should be short-circuited. Fixed the statement here. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:196: read_fd.release())); On 2017/04/04 17:28:34, Luis Héctor Chávez wrote: > you can do base::Passed(std::move(read_fd)) instead of read_fd.release(). Thay > way StartTracing can receive a base::ScopedFD instead of a raw int. Done. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:237: bool success) { On 2017/04/04 17:28:35, Luis Héctor Chávez wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI);? Done. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:255: const scoped_refptr<base::RefCountedString>& data) { On 2017/04/04 17:28:34, Luis Héctor Chávez wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI);? Done. https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:261: std::unique_ptr<ArcTracingReader> reader_ = On 2017/04/04 17:28:34, Luis Héctor Chávez wrote: > nit: move the initialization to the constructor. Done.
https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/480001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:68: char buf[kArcTraceMessageLength + 1]; On 2017/04/04 17:28:35, Luis Héctor Chávez wrote: > nit: You don't need the +1 here. Done.
shunhsingou@chromium.org changed reviewers: + dcheng@chromium.org, dsinclair@chromium.org
+dsinclair for changes in content/browser/tracing/* +dcheng for changes in components/arc/common/tracing.mojom
I think oystein@ is in a better position to review the tracing bits then myself.
https://codereview.chromium.org/2400163003/diff/500001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:63: categories_.emplace_back(Category({category, kCategoryPrefix + category})); Does categories_.emplace_back({...}) work? Otherwise categories_.emplace_back(Category{...}) should do the trick. If you elide the type name from the uniform initialization syntax, it will attempt to deduce it. This effectively constructs a Category (the {category, ...} part), and then pass that into another Category (the Category(...) part) using the move constructor. Finally it will add it to the vector using yet _another_ move constructor (thanks to emplace_back). https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:94: // |TraceEvent| is designed for Chrome trace. There are many restricion in nit: s/restricion/restrictions/ https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:250: base::Unretained(this), callback)); IIUC there's nothing that guarantees that |this| will still be alive by the time this is called. I *think* you still need to have the weak_ptr_factory_. I'm also a bit unsure about L248. I'll take a look tomorrow again, but please make sure you read the docs about WeakPtrFactory to see if it's legal to call reader_.GetWeakPtr() in the UI thread and check it in the IO thread. https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:263: bool is_stopping = false; nit: |is_stopping_|
https://codereview.chromium.org/2400163003/diff/500001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc (right): https://codereview.chromium.org/2400163003/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc:63: categories_.emplace_back(Category({category, kCategoryPrefix + category})); On 2017/04/06 00:59:08, Luis Héctor Chávez wrote: > Does categories_.emplace_back({...}) work? Otherwise > categories_.emplace_back(Category{...}) should do the trick. > > If you elide the type name from the uniform initialization syntax, it will > attempt to deduce it. This effectively constructs a Category (the {category, > ...} part), and then pass that into another Category (the Category(...) part) > using the move constructor. Finally it will add it to the vector using yet > _another_ move constructor (thanks to emplace_back). The second one works. :) https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:94: // |TraceEvent| is designed for Chrome trace. There are many restricion in On 2017/04/06 00:59:08, Luis Héctor Chávez wrote: > nit: s/restricion/restrictions/ Done. https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:250: base::Unretained(this), callback)); On 2017/04/06 00:59:08, Luis Héctor Chávez wrote: > IIUC there's nothing that guarantees that |this| will still be alive by the time > this is called. I *think* you still need to have the weak_ptr_factory_. > > I'm also a bit unsure about L248. I'll take a look tomorrow again, but please > make sure you read the docs about WeakPtrFactory to see if it's legal to call > reader_.GetWeakPtr() in the UI thread and check it in the IO thread. Hmm. True. weak_ptr_factory_ is required as |this| might be invalid. The problem is that we can't bind weak_ptr with function with return value. So I change this back to PostTask instead of PostTaskAndReplyWithResult now. For the thread issue, as the weak_ptr doc says: "Weak pointers may be passed safely between threads, but must always be dereferenced and invalidated on the same SequencedTaskRunner otherwise checking the pointer would be racey." If what I understand is correct, all the usage of weak_ptr here is by BrowserThread::IO, so it should be safe to do this. https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:263: bool is_stopping = false; On 2017/04/06 00:59:08, Luis Héctor Chávez wrote: > nit: |is_stopping_| Done.
Description was changed from ========== arc: enable Android tracing in verified-boot mode In verified-boot mode, ftrace trace_marker is not allowed for writting due to security concern. As an altenative, we use a socket for processes in Android to write trace events, and send back the trace data via Mojo handle. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/1737591 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android tracing in verified-boot mode In verified-boot mode, ftrace trace_marker is not allowed for writting due to security concern. As an altenative, we use a socket for processes in Android to write trace events, and send back the trace data via Mojo handle. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/2028075 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ==========
https://codereview.chromium.org/2400163003/diff/520001/components/arc/common/... File components/arc/common/tracing.mojom (right): https://codereview.chromium.org/2400163003/diff/520001/components/arc/common/... components/arc/common/tracing.mojom:16: StartTracing@1(array<string> categories, handle sock) => (bool success); Nit: sock => socket https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:103: std::unique_ptr<base::Value> value = base::JSONReader::Read(data.get()); We shouldn't be parsing JSON from less trusted processes in the browser process. Is it critical to do this check here? https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:118: delete[] event->name(); The manual management of event->name() is really hard to follow here and feels really fragile. Is it possible to have something external to TraceEvent keep it live? https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:204: base::Passed(std::move(read_fd)))); Nit: base::Passed(&read_fd) https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:253: base::RefCountedString::TakeString(&no_data)); A default constructed base::RefCountedString is the same thing and a little bit simpler. https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:676: json_string = events_str_ptr->data(); This copies the data string at least 2x now for Windows rather than just 1x. Maybe just keep the original code, and add an additional else if branch for ARC++.
https://codereview.chromium.org/2400163003/diff/520001/components/arc/common/... File components/arc/common/tracing.mojom (right): https://codereview.chromium.org/2400163003/diff/520001/components/arc/common/... components/arc/common/tracing.mojom:16: StartTracing@1(array<string> categories, handle sock) => (bool success); On 2017/04/06 06:14:14, dcheng wrote: > Nit: sock => socket Done. https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:103: std::unique_ptr<base::Value> value = base::JSONReader::Read(data.get()); On 2017/04/06 06:14:14, dcheng wrote: > We shouldn't be parsing JSON from less trusted processes in the browser process. > Is it critical to do this check here? The JSON data is generated by our own service in Android, so this is just an additional check to make sure the data is valid. If it's not that safe, do you think we can just skip the parsing here? So the data is sent to the trace viewer UI directly (which will run in javascript), and should be safer than this. https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:118: delete[] event->name(); On 2017/04/06 06:14:14, dcheng wrote: > The manual management of event->name() is really hard to follow here and feels > really fragile. Is it possible to have something external to TraceEvent keep it > live? It's a ring buffer so it doesn't make sense to have another place to hold the ownership. The only solution that I can think of is to implement ring buffer on our own so we don't need to fit our data into TraceBuffer. Does it sound better to you or do you have other suggestion? https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:204: base::Passed(std::move(read_fd)))); On 2017/04/06 06:14:14, dcheng wrote: > Nit: base::Passed(&read_fd) Done. https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:253: base::RefCountedString::TakeString(&no_data)); On 2017/04/06 06:14:14, dcheng wrote: > A default constructed base::RefCountedString is the same thing and a little bit > simpler. Done. https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2400163003/diff/520001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:676: json_string = events_str_ptr->data(); On 2017/04/06 06:14:14, dcheng wrote: > This copies the data string at least 2x now for Windows rather than just 1x. > Maybe just keep the original code, and add an additional else if branch for > ARC++. Done.
The latest patch: 1. Remove the parsing of JSON to avoid security issue. 2. Use cc::RingBuffer instead of TraceBuffer so we don't need to hack the data to fit into TraceBuffer.
question for dcheng@: I suggested doing the JSON parsing to more or less spot-check that the Android side won't serve us invalid data (mainly because we're adding these JSON blocks unquoted). Do we want to try to still do this, or is the current code okay? other than that, I only have minor nits at this point. https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:250: base::Unretained(this), callback)); On 2017/04/06 04:49:20, Earl Ou wrote: > On 2017/04/06 00:59:08, Luis Héctor Chávez wrote: > > IIUC there's nothing that guarantees that |this| will still be alive by the > time > > this is called. I *think* you still need to have the weak_ptr_factory_. > > > > I'm also a bit unsure about L248. I'll take a look tomorrow again, but please > > make sure you read the docs about WeakPtrFactory to see if it's legal to call > > reader_.GetWeakPtr() in the UI thread and check it in the IO thread. > > Hmm. True. weak_ptr_factory_ is required as |this| might be invalid. > > The problem is that we can't bind weak_ptr with function with return value. So I > change this back to PostTask instead of PostTaskAndReplyWithResult now. > > For the thread issue, as the weak_ptr doc says: > "Weak pointers may be passed safely between threads, but must always be > dereferenced and invalidated on the same SequencedTaskRunner otherwise > checking the pointer would be racey." > > If what I understand is correct, all the usage of weak_ptr here is by > BrowserThread::IO, so it should be safe to do this. > The destructor of |reader_| is another source of weak pointer invalidation, and that one is definitely not run in the IO thread. Having said that, your class is a Singleton, and its destructor will be run after all the messageloops are destroyed (see base/Singleton.h and base/at_exit.h), there is no way that any live |reader_| (weak) references are live after the IO thread is gone. So yes, this should be safe, but might not be if we ever un-Singleton this class. Please add a comment about this here :D https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:53: base::Unretained(this))); nit: s/base::Unretained(this)/GetWeakPtr())/ https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:79: ring_buffer_.SaveToBuffer(buf); nit: you can still avoid the +1 above: ring_buffer_.SaveToBuffer(std::string(buf, n)); https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:195: new base::RefCountedString); nit: new base::RefCountedString() (see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer... , second note).
mojo lgtm On 2017/04/07 16:17:46, Luis Héctor Chávez wrote: > question for dcheng@: I suggested doing the JSON parsing to more or less > spot-check that the Android side won't serve us invalid data (mainly because > we're adding these JSON blocks unquoted). Do we want to try to still do this, or > is the current code okay? > > other than that, I only have minor nits at this point. We don't want to parse JSON at all in the browser process. If this is important, one alternative is to send over base::Values from ARC++ and then turn it into a JSON with base::JSONWriter, but I guess that would be non-trivial. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:53: base::Unretained(this))); On 2017/04/07 16:17:46, Luis Héctor Chávez wrote: > nit: s/base::Unretained(this)/GetWeakPtr())/ I'm OK with unretained as long as it's explained why this is safe (Using GetWeakPtr() here has the potential to make this class more complex, as we call GetWeakPtr() on multiple threads and it will be a bit harder to understand the thread safety) https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:68: PLOG(WARNING) << "Unexpected error while reading trace from client."; DPLOG? https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:75: LOG(WARNING) << "Unexpected data size when reading trace from client."; DLOG? https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:94: append_comma = true; Nit: move inside the if at 91
https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:34: constexpr size_t kArcTraceMessageLength = 1024 + 512; Can you document why this particular value was chosen? https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:41: class ArcTracingReader { It's not immediately obvious to me why this was factored out into its own class, since the lifespan seems to be identical with that of the parent ArcTracingAgentImpl. Can you put a comment explaining why? https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:84: // Stop fd_watcher_. nit: Not really a helpful comment, as it just states what the line below does. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:92: data.append(","); nit: maybe: if (append_comma) data.append(","); else append_comma = true; instead to avoid assigning to append_comma in every loop iteration. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:131: bool success = delegate_ != nullptr; nit: (delegate_ != nullptr) for readability. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:215: std::unique_ptr<ArcTracingReader> reader_; Why is this a unique_ptr rather than just an inlined member, given that the whole definition of ArcTracingReader is available here?
On 2017/04/07 at 17:58:29, dcheng wrote: > mojo lgtm > > On 2017/04/07 16:17:46, Luis Héctor Chávez wrote: > > question for dcheng@: I suggested doing the JSON parsing to more or less > > spot-check that the Android side won't serve us invalid data (mainly because > > we're adding these JSON blocks unquoted). Do we want to try to still do this, or > > is the current code okay? > > > > other than that, I only have minor nits at this point. > > We don't want to parse JSON at all in the browser process. If this is important, one alternative is to send over base::Values from ARC++ and then turn it into a JSON with base::JSONWriter, but I guess that would be non-trivial. > Another option could be safe_json::SafeJsonParser.
On 2017/04/07 18:35:34, oystein wrote: > On 2017/04/07 at 17:58:29, dcheng wrote: > > mojo lgtm > > > > On 2017/04/07 16:17:46, Luis Héctor Chávez wrote: > > > question for dcheng@: I suggested doing the JSON parsing to more or less > > > spot-check that the Android side won't serve us invalid data (mainly because > > > we're adding these JSON blocks unquoted). Do we want to try to still do > this, or > > > is the current code okay? > > > > > > other than that, I only have minor nits at this point. > > > > We don't want to parse JSON at all in the browser process. If this is > important, one alternative is to send over base::Values from ARC++ and then turn > it into a JSON with base::JSONWriter, but I guess that would be non-trivial. > > > > Another option could be safe_json::SafeJsonParser. That is an option, but that's another process hop + another callback.
https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/500001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:250: base::Unretained(this), callback)); On 2017/04/07 16:17:46, Luis Héctor Chávez wrote: > On 2017/04/06 04:49:20, Earl Ou wrote: > > On 2017/04/06 00:59:08, Luis Héctor Chávez wrote: > > > IIUC there's nothing that guarantees that |this| will still be alive by the > > time > > > this is called. I *think* you still need to have the weak_ptr_factory_. > > > > > > I'm also a bit unsure about L248. I'll take a look tomorrow again, but > please > > > make sure you read the docs about WeakPtrFactory to see if it's legal to > call > > > reader_.GetWeakPtr() in the UI thread and check it in the IO thread. > > > > Hmm. True. weak_ptr_factory_ is required as |this| might be invalid. > > > > The problem is that we can't bind weak_ptr with function with return value. So > I > > change this back to PostTask instead of PostTaskAndReplyWithResult now. > > > > For the thread issue, as the weak_ptr doc says: > > "Weak pointers may be passed safely between threads, but must always be > > dereferenced and invalidated on the same SequencedTaskRunner otherwise > > checking the pointer would be racey." > > > > If what I understand is correct, all the usage of weak_ptr here is by > > BrowserThread::IO, so it should be safe to do this. > > > > The destructor of |reader_| is another source of weak pointer invalidation, and > that one is definitely not run in the IO thread. Having said that, your class is > a Singleton, and its destructor will be run after all the messageloops are > destroyed (see base/Singleton.h and base/at_exit.h), there is no way that any > live |reader_| (weak) references are live after the IO thread is gone. So yes, > this should be safe, but might not be if we ever un-Singleton this class. > > Please add a comment about this here :D Done adding the comments above the declaration of |reader_|. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:34: constexpr size_t kArcTraceMessageLength = 1024 + 512; On 2017/04/07 18:31:40, oystein wrote: > Can you document why this particular value was chosen? Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:41: class ArcTracingReader { On 2017/04/07 18:31:40, oystein wrote: > It's not immediately obvious to me why this was factored out into its own class, > since the lifespan seems to be identical with that of the parent > ArcTracingAgentImpl. Can you put a comment explaining why? Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:53: base::Unretained(this))); On 2017/04/07 17:58:28, dcheng wrote: > On 2017/04/07 16:17:46, Luis Héctor Chávez wrote: > > nit: s/base::Unretained(this)/GetWeakPtr())/ > > I'm OK with unretained as long as it's explained why this is safe > > (Using GetWeakPtr() here has the potential to make this class more complex, as > we call GetWeakPtr() on multiple threads and it will be a bit harder to > understand the thread safety) Yes, I think we should use |Unretained| here to avoid weak ptr in different runner. Added a comment for this. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:68: PLOG(WARNING) << "Unexpected error while reading trace from client."; On 2017/04/07 17:58:28, dcheng wrote: > DPLOG? Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:75: LOG(WARNING) << "Unexpected data size when reading trace from client."; On 2017/04/07 17:58:28, dcheng wrote: > DLOG? Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:79: ring_buffer_.SaveToBuffer(buf); On 2017/04/07 16:17:46, Luis Héctor Chávez wrote: > nit: you can still avoid the +1 above: > > ring_buffer_.SaveToBuffer(std::string(buf, n)); Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:84: // Stop fd_watcher_. On 2017/04/07 18:31:41, oystein wrote: > nit: Not really a helpful comment, as it just states what the line below does. Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:92: data.append(","); On 2017/04/07 18:31:41, oystein wrote: > nit: maybe: > > if (append_comma) > data.append(","); > else > append_comma = true; > > instead to avoid assigning to append_comma in every loop iteration. Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:94: append_comma = true; On 2017/04/07 17:58:28, dcheng wrote: > Nit: move inside the if at 91 Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:131: bool success = delegate_ != nullptr; On 2017/04/07 18:31:40, oystein wrote: > nit: (delegate_ != nullptr) for readability. Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:195: new base::RefCountedString); On 2017/04/07 16:17:46, Luis Héctor Chávez wrote: > nit: new base::RefCountedString() > > (see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer... > , second note). Done. https://codereview.chromium.org/2400163003/diff/580001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:215: std::unique_ptr<ArcTracingReader> reader_; On 2017/04/07 18:31:41, oystein wrote: > Why is this a unique_ptr rather than just an inlined member, given that the > whole definition of ArcTracingReader is available here? Done.
https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... File components/arc/common/tracing.mojom (right): https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... components/arc/common/tracing.mojom:5: // Next MinVersion: 1 Next MinVersion: 2 https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... components/arc/common/tracing.mojom:16: StartTracing@1(array<string> categories, handle socket) => (bool success); Oh! I almost forgot! [MinVersion=1] handle socket https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:7: #include <string.h> do you still need this? https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:8: #include <sys/socket.h> Do you still need this? https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:46: // from |ArcTracingAgentImpl| to isolate logics that runs on browser's IO nit: s/logics/the logic/ https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:61: // fd_watcher_ is always destructed before |this| got destructed. nit: |fd_watcher_|. s/got destructed/is destroyed/. Also s/destructed/destroyed/ in all the change since that spelling is way more common: chromium/src$ ag '//.*destructed' chrome | wc -l 50 chromium/src$ ag '//.*destroyed' chrome | wc -l 972 https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:228: // destruction is always performed after all messageloop destructed, and thus nit: all MessageLoops are destructed. https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:229: // no racing condition in such situation. nit: "and thus there are no race conditions in such situation".
https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... File components/arc/common/tracing.mojom (right): https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... components/arc/common/tracing.mojom:16: StartTracing@1(array<string> categories, handle socket) => (bool success); On 2017/04/13 21:37:52, Luis Héctor Chávez wrote: > Oh! I almost forgot! > > [MinVersion=1] handle socket This would be a mojom compiling error: Exception: Non-nullable fields are only allowed in version 0 of a struct. TracingInstance_StartTracing_Params.socket is defined with [MinVersion=1] Do you think I should do something like: DeprecatedStartTracing@1(array<string> categories) => (bool success); [MinVersion=1] StartTracing@3(array<string> categories, handle socket) => (bool success);
https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... File components/arc/common/tracing.mojom (right): https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... components/arc/common/tracing.mojom:16: StartTracing@1(array<string> categories, handle socket) => (bool success); On 2017/04/14 22:37:38, Earl Ou wrote: > On 2017/04/13 21:37:52, Luis Héctor Chávez wrote: > > Oh! I almost forgot! > > > > [MinVersion=1] handle socket > > This would be a mojom compiling error: > Exception: Non-nullable fields are only allowed in version 0 of a struct. > TracingInstance_StartTracing_Params.socket is defined with [MinVersion=1] > > Do you think I should do something like: > > DeprecatedStartTracing@1(array<string> categories) => (bool success); > [MinVersion=1] StartTracing@3(array<string> categories, handle socket) => (bool > success); no, that's what the versions are for :) i messed up, it should have been [MinVersion=1] handle? socket
https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... File components/arc/common/tracing.mojom (right): https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... components/arc/common/tracing.mojom:5: // Next MinVersion: 1 On 2017/04/13 21:37:52, Luis Héctor Chávez wrote: > Next MinVersion: 2 Done. https://codereview.chromium.org/2400163003/diff/620001/components/arc/common/... components/arc/common/tracing.mojom:16: StartTracing@1(array<string> categories, handle socket) => (bool success); On 2017/04/15 05:34:19, Luis Héctor Chávez wrote: > On 2017/04/14 22:37:38, Earl Ou wrote: > > On 2017/04/13 21:37:52, Luis Héctor Chávez wrote: > > > Oh! I almost forgot! > > > > > > [MinVersion=1] handle socket > > > > This would be a mojom compiling error: > > Exception: Non-nullable fields are only allowed in version 0 of a struct. > > TracingInstance_StartTracing_Params.socket is defined with [MinVersion=1] > > > > Do you think I should do something like: > > > > DeprecatedStartTracing@1(array<string> categories) => (bool success); > > [MinVersion=1] StartTracing@3(array<string> categories, handle socket) => > (bool > > success); > > no, that's what the versions are for :) > > i messed up, it should have been > > [MinVersion=1] handle? socket Done. That works. :) https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:7: #include <string.h> On 2017/04/13 21:37:52, Luis Héctor Chávez wrote: > do you still need this? Done. https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:8: #include <sys/socket.h> On 2017/04/13 21:37:52, Luis Héctor Chávez wrote: > Do you still need this? Done. https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:46: // from |ArcTracingAgentImpl| to isolate logics that runs on browser's IO On 2017/04/13 21:37:52, Luis Héctor Chávez wrote: > nit: s/logics/the logic/ Done. https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:228: // destruction is always performed after all messageloop destructed, and thus On 2017/04/13 21:37:52, Luis Héctor Chávez wrote: > nit: all MessageLoops are destructed. Done. https://codereview.chromium.org/2400163003/diff/620001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:229: // no racing condition in such situation. On 2017/04/13 21:37:52, Luis Héctor Chávez wrote: > nit: "and thus there are no race conditions in such situation". Done.
*/arc/* lgtm thanks for your hard work!
On 2017/04/17 15:37:27, Luis Héctor Chávez wrote: > */arc/* lgtm > > thanks for your hard work! Thanks a lot for the detailed review!
Thanks! tracing lgtm w/ a tiny nit :) https://codereview.chromium.org/2400163003/diff/660001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/660001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:33: // have additional data field. nit: s/field/fields/
Thanks! https://codereview.chromium.org/2400163003/diff/660001/content/browser/tracin... File content/browser/tracing/arc_tracing_agent.cc (right): https://codereview.chromium.org/2400163003/diff/660001/content/browser/tracin... content/browser/tracing/arc_tracing_agent.cc:33: // have additional data field. On 2017/04/17 21:43:39, oystein wrote: > nit: s/field/fields/ Done.
The CQ bit was checked by shunhsingou@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by shunhsingou@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by shunhsingou@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by shunhsingou@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+clamy for //content Hi Camille, Could you help reviewing the changing of //content/browser/BUILD.gn? Thanks!
On 2017/04/24 20:49:14, Earl Ou wrote: > +clamy for //content > > Hi Camille, > Could you help reviewing the changing of //content/browser/BUILD.gn? Thanks! content/browser/BUILD.gn lgtm. Trusting oysteine's review for content/browser/tracing.
The CQ bit was checked by shunhsingou@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, oysteine@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2400163003/#ps720001 (title: "rebase to tot")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 720001, "attempt_start_ts": 1493628996532850,
"parent_rev": "c362658d2401554189fb6cb79f8e3c4f028fcbee", "commit_rev":
"7092984694bd56fc05488582a93a0fda0c8d7f29"}
Message was sent while issue was closed.
Description was changed from ========== arc: enable Android tracing in verified-boot mode In verified-boot mode, ftrace trace_marker is not allowed for writting due to security concern. As an altenative, we use a socket for processes in Android to write trace events, and send back the trace data via Mojo handle. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/2028075 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) BUG=653795 TEST=Run chrome://tracing and see events from Android. ========== to ========== arc: enable Android tracing in verified-boot mode In verified-boot mode, ftrace trace_marker is not allowed for writting due to security concern. As an altenative, we use a socket for processes in Android to write trace events, and send back the trace data via Mojo handle. All change set includes: - http://crrev.com/2400163003 (Chromium) - http://ag/2028075 (Android system/core) - http://ag/1738271 (Android device/google/cheets2) BUG=653795 TEST=Run chrome://tracing and see events from Android. Review-Url: https://codereview.chromium.org/2400163003 Cr-Commit-Position: refs/heads/master@{#468306} Committed: https://chromium.googlesource.com/chromium/src/+/7092984694bd56fc05488582a93a... ==========
Message was sent while issue was closed.
Committed patchset #37 (id:720001) as https://chromium.googlesource.com/chromium/src/+/7092984694bd56fc05488582a93a... |
