Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3002793002:
[Telemetry] fix run_benchmark crash on CrOS (Closed)
|
Created:
3 years, 4 months ago by dave.rodgman Modified:
3 years, 4 months ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionTelemetry: fix run_benchmark crash on CrOS
run_benchmark against a remote CrOS target crashes because it tries to
create two connections to obtain trace data - one against the login
screen, one against the instance of Chrome running the benchmark. This
causes the second connection to fail with an error saying "Tracing
already started".
Disable the first trace connection against the login screen, as it is
not needed.
BUG=catapult:#756342
Test: Use run_benchmark to verify that this patch solves the issue;
see crbug.com/756342 for details.
Review-Url: https://codereview.chromium.org/3002793002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/f83558ef7bc609e38b9b6bdb541393ad02209ad8
Patch Set 1 #
Total comments: 2
Patch Set 2 : [Telemetry] fix run_benchmark crash on CrOS #Messages
Total messages: 16 (8 generated)
Description was changed from ========== Telemetry: fix run_benchmark crash on CrOS run_benchmark against a remote CrOS target crashes because it tries to create two connections to obtain trace data - one against the login screen, one against the instance of Chrome running the benchmark. This causes the second connection to fail with an error saying "Tracing already started". Disable the first trace connection against the login screen, as it is not needed. BUG=catapult:#756342 Test: Use run_benchmark to verify that this patch solves the issue; see crbug.com/756342 for details. ========== to ========== Telemetry: fix run_benchmark crash on CrOS run_benchmark against a remote CrOS target crashes because it tries to create two connections to obtain trace data - one against the login screen, one against the instance of Chrome running the benchmark. This causes the second connection to fail with an error saying "Tracing already started". Disable the first trace connection against the login screen, as it is not needed. BUG=catapult:#756342 Test: Use run_benchmark to verify that this patch solves the issue; see crbug.com/756342 for details. ==========
dave.rodgman@arm.com changed reviewers: + perezju@chromium.org
Hi, I've found an issue (crbug.com/756342) in Telemetry related to how it gets trace data on CrOS, which ultimately causes many benchmarks to fail. This patch fixes the issue and allows benchmarks to work again on Cros.
perezju@chromium.org changed reviewers: + nednguyen@google.com
lgtm +ned in case you have any other opinions
Can you explain to me again why CrOS have two browser startup steps? https://codereview.chromium.org/3002793002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py (right): https://codereview.chromium.org/3002793002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:148: if not self._enable_tracing: This is a misusage of abstraction. The name of this property is "supports_tracing", whereas what you want is not to enable tracing even if it's supported.
achuith@chromium.org changed reviewers: + achuith@chromium.org
https://codereview.chromium.org/3002793002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/backends/chrome/cros_browser_backend.py (right): https://codereview.chromium.org/3002793002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome/cros_browser_backend.py:301: self._WaitForBrowserToComeUp(enable_tracing) I think we may be able to get rid of this call, but maybe try that in a follow-up CL.
On 2017/08/17 14:16:24, nednguyen wrote: > Can you explain to me again why CrOS have two browser startup steps? > > https://codereview.chromium.org/3002793002/diff/1/telemetry/telemetry/interna... > File > telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py > (right): > > https://codereview.chromium.org/3002793002/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:148: > if not self._enable_tracing: > This is a misusage of abstraction. The name of this property is > "supports_tracing", whereas what you want is not to enable tracing even if it's > supported. Thanks for the review Ned. The CrOS device boots into a login screen which is running Chrome - first Telemetry waits for this browser to come up. Then, it logs in which launches a new browser instance to run the benchmark in. Whilst looking into addressing your point about misusing supports_tracing, I concluded that rather than avoid creating the first connection, it's a lot simpler and cleaner to simply close the first connection when connecting to the second browser instance - please see updated patch.
On 2017/08/17 16:08:09, dave.rodgman wrote: > On 2017/08/17 14:16:24, nednguyen wrote: > > Can you explain to me again why CrOS have two browser startup steps? > > > > > https://codereview.chromium.org/3002793002/diff/1/telemetry/telemetry/interna... > > File > > > telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py > > (right): > > > > > https://codereview.chromium.org/3002793002/diff/1/telemetry/telemetry/interna... > > > telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:148: > > if not self._enable_tracing: > > This is a misusage of abstraction. The name of this property is > > "supports_tracing", whereas what you want is not to enable tracing even if > it's > > supported. > > Thanks for the review Ned. > > The CrOS device boots into a login screen which is running Chrome - first > Telemetry waits for this browser to come up. Then, it logs in which launches a > new browser instance to run the benchmark in. > > Whilst looking into addressing your point about misusing supports_tracing, I > concluded that rather than avoid creating the first connection, it's a lot > simpler and cleaner to simply close the first connection when connecting to the > second browser instance - please see updated patch. THe new patch makes a lot more sense to me. Thanks! lgtm
The CQ bit was checked by dave.rodgman@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org Link to the patchset: https://codereview.chromium.org/3002793002/#ps20001 (title: "[Telemetry] fix run_benchmark crash on CrOS")
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": 20001, "attempt_start_ts": 1503051819276880,
"parent_rev": "e1241eb99c0a0e68e678878b879e1c1cd10b3352", "commit_rev":
"f83558ef7bc609e38b9b6bdb541393ad02209ad8"}
Message was sent while issue was closed.
Description was changed from ========== Telemetry: fix run_benchmark crash on CrOS run_benchmark against a remote CrOS target crashes because it tries to create two connections to obtain trace data - one against the login screen, one against the instance of Chrome running the benchmark. This causes the second connection to fail with an error saying "Tracing already started". Disable the first trace connection against the login screen, as it is not needed. BUG=catapult:#756342 Test: Use run_benchmark to verify that this patch solves the issue; see crbug.com/756342 for details. ========== to ========== Telemetry: fix run_benchmark crash on CrOS run_benchmark against a remote CrOS target crashes because it tries to create two connections to obtain trace data - one against the login screen, one against the instance of Chrome running the benchmark. This causes the second connection to fail with an error saying "Tracing already started". Disable the first trace connection against the login screen, as it is not needed. BUG=catapult:#756342 Test: Use run_benchmark to verify that this patch solves the issue; see crbug.com/756342 for details. Review-Url: https://codereview.chromium.org/3002793002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
