Telemetry overwrites --vmodule flag in session_manager when launching browser on CrOS |
|||||||||||||
Issue descriptionI found this problem when testing on CrOS with telemetry. The vmodule parameter added in /etc/chrome_dev.conf is handled in session manager, but telemetry appends another --vmodule flag so overwrites the original one: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/cros_browser_backend.py&l=70 If I read the code correct, telemetry calls EnableChromeTesting() with the overriding --vmodule flag https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/cros_browser_backend.py&l=105 The flag is then passed as extra_args at https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/session_manager_impl.cc#257 when launching the browser. Multiple --vmodule flag should be merged in session_manager before launching the browser
,
May 9 2016
,
May 9 2016
Alternately, it may be cleaner to make Chrome's logging handle multiple --vmodule flags. I'm not sure if that's easy to do, though.
,
May 9 2016
Yes it's cleaner, but base::CommandLine in chrome wasn't designed for multiple values. Doing the change is not hard, but whether the re-design would be accept. (In the past only the last value is remembered, should I remember all occurrence for all flags?) On the other hand, it's not so clean if I only take "--vmodule" as a special case.
,
May 13 2016
,
Jun 2 2016
,
Nov 28 2016
Another idea, handle that in telemetry's GetBrowserStartupArgs(). Since telemetry is hacking the command line argument (get existing args and append more), it is reasonable that it is telemetry's responsibility to handle vmodule correctly. And the code change is probably just simple list and string operations in python.
,
Nov 28 2016
Yes, it could be done in telemetry, but it's simply not the right place. It ought to be handled either by session_manager or chrome.
,
Nov 28 2016
Following achuith's suggestion #8, I modified session_manager to handle multiple vmodule https://chromium-review.googlesource.com/#/c/414660/
,
Nov 28 2016
Thank you Cheng-Yu! Added some comments on the CL.
,
Nov 29 2016
add some folks who might be interested in this
,
Dec 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/ee47b0783cd90cf0b13024db116022622df3967f commit ee47b0783cd90cf0b13024db116022622df3967f Author: Cheng-Yu Lee <cylee@chromium.org> Date: Mon Nov 28 19:21:21 2016 login: Combine multiple --vmodule flags when starting chrome. In the past only one of --vmodule got applied if multiple are given. It causes problem if --vmodule was specified in multiple places (e.g., Telemetry). BUG= chromium:610332 TEST=manually tested to ensure --vmodule from telemetry is merged Change-Id: I583eaf0c292134b76d72543c0a0216ccb0ef2c77 Reviewed-on: https://chromium-review.googlesource.com/414660 Commit-Ready: Cheng-Yu Lee <cylee@chromium.org> Tested-by: Cheng-Yu Lee <cylee@chromium.org> Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> Reviewed-by: Cheng-Yu Lee <cylee@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/ee47b0783cd90cf0b13024db116022622df3967f/login_manager/browser_job_unittest.cc [modify] https://crrev.com/ee47b0783cd90cf0b13024db116022622df3967f/login_manager/browser_job.cc
,
Dec 5 2016
,
Mar 4 2017
,
Apr 17 2017
,
May 30 2017
,
Aug 1 2017
,
Oct 14 2017
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by cylee@chromium.org
, May 9 2016Labels: -OS-Mac OS-Chrome