|
|
DescriptionFix telemetry's IsBrowserRunning check
This method currently checks to see if there's a process that matches
the _prefix_ 'org.chromium.chrome' and returns true if there is.
However, even if the browser itself is shutdown, there may be other
processes running that match that prefix which can confuse telemetry.
This patch fixes the issue by using GetApplicationPids which uses an
exact match rather than a prefix.
BUG=catapult:#3912
Review-Url: https://chromiumcodereview.appspot.com/3018503002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ae0c06b444b18e43a058e8e833e50c8c548d81d9
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use GetApplicationPids #Messages
Total messages: 23 (16 generated)
The CQ bit was checked by bokan@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: Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
bokan@chromium.org changed reviewers: + perezju@chromium.org
Hi perezju@, I'm wondering if this is the right fix and you look like you've touched this file recently. Basically, I'm running into an issue running telemetry locally on Android. After running the first story, telemetry would crash out without giving much by way of explanation. I finally narrowed it down to: - Story completes. Call browser.close(). Browser on device closes. - Call IsBrowserRunning to make sure the close() call succeeded - Surprisingly this returns true so we try to print an error saying the browser is still running with pid %d An exception is thrown when we try to get the browser's pid at this point since the browser isn't actually running. What IsBrowserRunning was hitting is a pid for 'org.chromium.chrome.tests.support'. I'm not sure what this is but killing it doesn't help as it seems to come back when I run telemetry again. Not sure why it's only a problem locally and not on bots.
Description was changed from ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. Bug: catapult:# ========== to ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. This patch fixes the issue by checking for 'org.chromium.chrome' exactly by appending the EOL character ($). ==========
ping, also, do I have to create a catapult bug? Where should that go if so, crbug?
Thanks for looking into this! You can file a bug at: https://github.com/catapult-project/catapult/issues/ (or crbug.com, that should be fine too) https://codereview.chromium.org/3018503002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/3018503002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:271: '%s$' % (self._backend_settings.package)) I don't think this will work. The process name is not matched as a regex. The fix should be done in IsAppRunning [1], probably replacing the call from GetPids to GetApplicationPids [2]. This change should be safe, but please also check with other callers of IsAppRunning whether this isn't likely to break their expectations. [1]: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... [2]: https://cs.chromium.org/chromium/src/third_party/catapult/devil/devil/android...
Description was changed from ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. This patch fixes the issue by checking for 'org.chromium.chrome' exactly by appending the EOL character ($). ========== to ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. This patch fixes the issue by checking for 'org.chromium.chrome' exactly by appending the EOL character ($). BUG=catapult:3912 ==========
Description was changed from ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. This patch fixes the issue by checking for 'org.chromium.chrome' exactly by appending the EOL character ($). BUG=catapult:3912 ========== to ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. This patch fixes the issue by using GetApplicationPids which uses an exact match rather than a prefix. BUG=catapult:3912 ==========
Description was changed from ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. This patch fixes the issue by using GetApplicationPids which uses an exact match rather than a prefix. BUG=catapult:3912 ========== to ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. This patch fixes the issue by using GetApplicationPids which uses an exact match rather than a prefix. BUG=catapult:#3912 ==========
The CQ bit was checked by bokan@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...
Sorry for the delay (was traveling). New patch using your suggestion is up. https://codereview.chromium.org/3018503002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/3018503002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:271: '%s$' % (self._backend_settings.package)) On 2017/09/19 12:57:01, perezju wrote: > I don't think this will work. The process name is not matched as a regex. > > The fix should be done in IsAppRunning [1], probably replacing the call from > GetPids to GetApplicationPids [2]. > > This change should be safe, but please also check with other callers of > IsAppRunning whether this isn't likely to break their expectations. > > [1]: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > [2]: > https://cs.chromium.org/chromium/src/third_party/catapult/devil/devil/android... D'oh! Sorry, you're right, I missed what `-F` does in grep (I thought I tried it for both running and not-running but I must have been sloppy). Thanks for the solution. I've made the change and confirmed it fixes the issue. It also seems that this is the only user of PlatformBackend.IsAppRunning (https://cs.chromium.org/search/?q=IsAppRunning+file:.*.py$&sq=package:chromiu...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm thanks!
The CQ bit was checked by bokan@chromium.org
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": 1506433091804450, "parent_rev": "0b563bed301ef780e6f0cac7bab670aaffb65fdf", "commit_rev": "ae0c06b444b18e43a058e8e833e50c8c548d81d9"}
Message was sent while issue was closed.
Description was changed from ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. This patch fixes the issue by using GetApplicationPids which uses an exact match rather than a prefix. BUG=catapult:#3912 ========== to ========== Fix telemetry's IsBrowserRunning check This method currently checks to see if there's a process that matches the _prefix_ 'org.chromium.chrome' and returns true if there is. However, even if the browser itself is shutdown, there may be other processes running that match that prefix which can confuse telemetry. This patch fixes the issue by using GetApplicationPids which uses an exact match rather than a prefix. BUG=catapult:#3912 Review-Url: https://chromiumcodereview.appspot.com/3018503002 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... |