Adding utility methods to retrieve minidump paths from the browser.

Expose the most recent minidump path to the AppCrashException and add utilities to retrieve and symbolize all minidump paths from the browser.

BUG=catapult:#2346

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/5bc1829231e2f15c3b9945af71c9d4b4373abcc7

[eyaich, 2016-06-15 16:42:57]

Description was changed from

==========
WIP: First attempt at minidump paths

5


4


3


minidump2


minidump1

BUG=catapult:#
==========

to

==========
WIP: First attempt at minidump paths.  Not ready for full review.

BUG=catapult:#2346
==========

[eyaich, 2016-06-15 16:42:57]

eyaich@google.com changed reviewers:
+ nednguyen@google.com

[eyaich, 2016-06-15 16:45:44]

Please take a look when you get a chance to see if I am on the right track. 
Thanks!

[nednguyen, 2016-06-15 16:51:53]

nednguyen@google.com changed reviewers:
+ kbr@chromium.org

[nednguyen, 2016-06-15 17:02:24]

This seems to be in the right direction. Though I think we may need to
reconsider whether to add BrowserCrashException. The if-statement in
inspector_backend.py makes it seems like we're doing the wrong thing here :-(

[eyaich, 2016-06-15 17:20:58]

On 2016/06/15 17:02:24, nednguyen wrote:
> This seems to be in the right direction. Though I think we may need to
> reconsider whether to add BrowserCrashException. The if-statement in
> inspector_backend.py makes it seems like we're doing the wrong thing here :-(

I agree, but we need to be able to distinguish somewhere that we are within a
Browser if we want to contain the minidump information in the exception itself. 
 Since browser is the only implementation of app that contains the minidump path
I didn't know where else we would create the BrowserCrashException to make that
distinction.  

My next step was to write some tests to see when these exceptions get generated
in inspector_backend and determine best how to handle the exception.  Let me
know if you have any other thoughts.

[Ken Russell (switch to Gerrit), 2016-06-15 18:24:00]

Thanks for moving this forward. It looks like a good start.

One concern is that Chrome, being a multi-process application, could plausibly
generate more than one minidump per "app crash". A way to enumerate at least all
of the unsymbolized minidumps is needed, if not all of the minidumps. Is that
planned for a follow-on CL? If so, this seems like a good start.

[nednguyen, 2016-06-20 15:39:24]

I think the rest is mostly lg2me, we just need to add some test coverage
asserting the minidump path is non-empty when chrome crash.

https://codereview.chromium.org/2052353003/diff/1/telemetry/telemetry/core/ex...
File telemetry/telemetry/core/exceptions.py (right):

https://codereview.chromium.org/2052353003/diff/1/telemetry/telemetry/core/ex...
telemetry/telemetry/core/exceptions.py:65: self._stack_trace =
trace_output.splitlines()
Hi Emily, after thinking this through. I think the easiest option here is just
add the concept of minidump_path to the app API & set self._minidump_path =
app.GetMinidumpPath() here.

That also means there is no need for BrowserCrashException. This is not ideal in
term of extending the app abstraction but the alternative does not seems pans
out well due to the dependencies. 

This was my bad advice to start with & I am truly sorry for this :-(

[eyaich, 2016-06-24 18:42:38]

On 2016/06/20 15:39:24, nednguyen wrote:
> I think the rest is mostly lg2me, we just need to add some test coverage
> asserting the minidump path is non-empty when chrome crash.
> 
>
https://codereview.chromium.org/2052353003/diff/1/telemetry/telemetry/core/ex...
> File telemetry/telemetry/core/exceptions.py (right):
> 
>
https://codereview.chromium.org/2052353003/diff/1/telemetry/telemetry/core/ex...
> telemetry/telemetry/core/exceptions.py:65: self._stack_trace =
> trace_output.splitlines()
> Hi Emily, after thinking this through. I think the easiest option here is just
> add the concept of minidump_path to the app API & set self._minidump_path =
> app.GetMinidumpPath() here.
> 
> That also means there is no need for BrowserCrashException. This is not ideal
in
> term of extending the app abstraction but the alternative does not seems pans
> out well due to the dependencies. 
> 
> This was my bad advice to start with & I am truly sorry for this :-(

Thanks for taking another look.  I updated the app API and got rid of
BrowserCrashException.  I have uploaded the updates and I will add unit tests
next week.

[eyaich, 2016-06-24 18:43:05]

https://codereview.chromium.org/2052353003/diff/1/telemetry/telemetry/core/ex...
File telemetry/telemetry/core/exceptions.py (right):

https://codereview.chromium.org/2052353003/diff/1/telemetry/telemetry/core/ex...
telemetry/telemetry/core/exceptions.py:65: self._stack_trace =
trace_output.splitlines()
On 2016/06/20 15:39:24, nednguyen wrote:
> Hi Emily, after thinking this through. I think the easiest option here is just
> add the concept of minidump_path to the app API & set self._minidump_path =
> app.GetMinidumpPath() here.
> 
> That also means there is no need for BrowserCrashException. This is not ideal
in
> term of extending the app abstraction but the alternative does not seems pans
> out well due to the dependencies. 
> 
> This was my bad advice to start with & I am truly sorry for this :-(

Done.

[nednguyen, 2016-06-24 21:35:06]

You can add a unittest to
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
that navigate to "chrome://crash" & assert that the minidump paths is not empty

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/app/__init__.py (right):

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/app/__init__.py:44: return
self._app_backend.GetMostRecentMinidumpPath()
We should define def GetMostRecentMinidumpPath(self): ... for app_backend.py
too?

[Ken Russell (switch to Gerrit), 2016-06-24 22:33:29]

Thanks for pushing this forward. It basically looks good to me. Some tests are
needed, as Ned points out.

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/browser/browser.py (right):

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:284: def
GetAllMinidumpPaths(self):
It would be nice to be able to tell which minidumps have already been
symbolized. When an AppCrashException is generated, the associated minidump is
symbolized. Would it be possible to track that in the app backend / browser
backend, and also provide a "GetAllUnsymbolizedMinidumpPaths"? That would allow
users to print stack traces for all of the crashes that haven't already been
displayed in the output.

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:287: def SymbolizeMinidump(self,
minidump_path):
Could you please document the return value from this and the other new
functions?

[eyaich, 2016-07-01 18:37:01]

On 2016/06/24 22:33:29, Ken Russell wrote:
> Thanks for pushing this forward. It basically looks good to me. Some tests are
> needed, as Ned points out.
> 
>
https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
> File telemetry/telemetry/internal/browser/browser.py (right):
> 
>
https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
> telemetry/telemetry/internal/browser/browser.py:284: def
> GetAllMinidumpPaths(self):
> It would be nice to be able to tell which minidumps have already been
> symbolized. When an AppCrashException is generated, the associated minidump is
> symbolized. Would it be possible to track that in the app backend / browser
> backend, and also provide a "GetAllUnsymbolizedMinidumpPaths"? That would
allow
> users to print stack traces for all of the crashes that haven't already been
> displayed in the output.
> 
>
https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
> telemetry/telemetry/internal/browser/browser.py:287: def
SymbolizeMinidump(self,
> minidump_path):
> Could you please document the return value from this and the other new
> functions?

Adding unit tests next

[eyaich, 2016-07-01 18:37:11]

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/app/__init__.py (right):

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/app/__init__.py:44: return
self._app_backend.GetMostRecentMinidumpPath()
On 2016/06/24 21:35:06, nednguyen (ooo til 7-11) wrote:
> We should define def GetMostRecentMinidumpPath(self): ... for app_backend.py
> too?

Done.

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/browser/browser.py (right):

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:284: def
GetAllMinidumpPaths(self):
On 2016/06/24 22:33:29, Ken Russell wrote:
> It would be nice to be able to tell which minidumps have already been
> symbolized. When an AppCrashException is generated, the associated minidump is
> symbolized. Would it be possible to track that in the app backend / browser
> backend, and also provide a "GetAllUnsymbolizedMinidumpPaths"? That would
allow
> users to print stack traces for all of the crashes that haven't already been
> displayed in the output.

Done.

https://codereview.chromium.org/2052353003/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:287: def SymbolizeMinidump(self,
minidump_path):
On 2016/06/24 22:33:29, Ken Russell wrote:
> Could you please document the return value from this and the other new
> functions?

Done.

[nednguyen, 2016-07-01 18:41:20]

since I will be out, lgtm to enable landing this when Ken is happy

[Ken Russell (switch to Gerrit), 2016-07-01 19:35:08]

Looking good overall. Could you please make one semantic change in how
GetAllUnsymbolizedMinidumpPaths works?

Any ideas on how to unit test this? desktop_browser_backend.py doesn't seem to
have an associated unit test.

We could (and probably should) add an integration test into the Chromium
workspace, similar to src/tools/perf/core/stacktrace_unittest.py. It could
deliberately crash Chrome's GPU process twice, make sure there are two minidumps
and that neither is symbolized, symbolize one, and make sure there's only one
remaining unsymbolized minidump.

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(right):

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:529:
minidump_paths.remove(self._most_recent_symbololized_minidump_path)
I think the logic should be different here -- _InternalSymbolizeMinidump should
record the path in a set, and this should remove all entries that are in that
set. If the user calls SymbolizeMinidump and then
GetAllUnsymbolizedMinidumpPaths, the symbolized path shouldn't show up in the
call to GetAllUnsymbolizedMinidumpPaths.

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/browser/browser.py (right):

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:286: """Returns all minidump
paths available in the backend"""
Add period at end of comment.

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:291: symbolized in the stack
trace."""
Simplify to "...that have not already been symbolized."

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:298: and error message when
false"""
Add period at end of comment.

[Ken Russell (switch to Gerrit), 2016-07-01 21:05:33]

BTW, since this is getting close to being ready, could you update the subject
and CL description? Thanks.

[eyaich, 2016-07-06 15:25:02]

Description was changed from

==========
WIP: First attempt at minidump paths.  Not ready for full review.

BUG=catapult:#2346
==========

to

==========
Expose the most recent minidump path to the AppCrashException and add utilities
to retrieve and symbolize all minidump paths from the browser.

BUG=catapult:#2346
==========

[eyaich, 2016-07-06 15:26:06]

Added a unittest to the browser to check for existence of the minidump path.

I can add an integration test to chromium tools in a follow on CL since it is in
the chromium repository.

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(right):

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:529:
minidump_paths.remove(self._most_recent_symbololized_minidump_path)
On 2016/07/01 19:35:08, Ken Russell wrote:
> I think the logic should be different here -- _InternalSymbolizeMinidump
should
> record the path in a set, and this should remove all entries that are in that
> set. If the user calls SymbolizeMinidump and then
> GetAllUnsymbolizedMinidumpPaths, the symbolized path shouldn't show up in the
> call to GetAllUnsymbolizedMinidumpPaths.

Done.

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/browser/browser.py (right):

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:286: """Returns all minidump
paths available in the backend"""
On 2016/07/01 19:35:08, Ken Russell wrote:
> Add period at end of comment.

Done.

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:291: symbolized in the stack
trace."""
On 2016/07/01 19:35:08, Ken Russell wrote:
> Simplify to "...that have not already been symbolized."

Done.

https://codereview.chromium.org/2052353003/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser.py:298: and error message when
false"""
On 2016/07/01 19:35:08, Ken Russell wrote:
> Add period at end of comment.

Done.

[Ken Russell (switch to Gerrit), 2016-07-06 18:53:12]

Looks great. Thanks for persisting on this and looking forward to a robust
integration test for it. LGTM.

https://codereview.chromium.org/2052353003/diff/60001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(right):

https://codereview.chromium.org/2052353003/diff/60001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:513:
return self._GetMostRecentMinidump()
In a follow-on CL I think this method's semantics should be changed, and the
name changed to something like GetMostRecentUnsymbolizedMinidumpPath; see
https://github.com/catapult-project/catapult/issues/2454 for rationale.

[eyaich, 2016-07-07 12:32:44]

The CQ bit was checked by eyaich@google.com

[eyaich, 2016-07-07 12:32:44]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2052353003/#ps60001
(title: "Adding unittest")

[commit-bot: I haz the power, 2016-07-07 12:32:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 12:41:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 12:41:44]

Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[eyaich, 2016-07-07 14:29:33]

The CQ bit was checked by eyaich@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 14:29:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 14:43:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 14:43:01]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[eyaich, 2016-07-07 14:47:14]

The CQ bit was checked by eyaich@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 14:47:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 15:00:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 15:00:11]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[eyaich, 2016-07-07 15:10:26]

The CQ bit was checked by eyaich@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 15:10:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 15:50:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 15:50:07]

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...)

[eyaich, 2016-07-08 13:46:28]

The CQ bit was checked by eyaich@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 13:46:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 14:20:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 14:20:09]

Dry run: This issue passed the CQ dry run.

[eyaich, 2016-07-08 16:17:32]

The CQ bit was checked by eyaich@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 16:17:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 17:08:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 17:08:52]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-07-11 17:53:32]

The refactoring looks great. LGTM

[eyaich, 2016-07-11 17:59:43]

The CQ bit was checked by eyaich@google.com

[eyaich, 2016-07-11 17:59:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2052353003/#ps180001
(title: "Refactoring GetAllMinidumpPaths to check breakpad format when
crashpad_database_util isn't present.")

[commit-bot: I haz the power, 2016-07-11 17:59:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 18:30:33]

Description was changed from

==========
Expose the most recent minidump path to the AppCrashException and add utilities
to retrieve and symbolize all minidump paths from the browser.

BUG=catapult:#2346
==========

to

==========
Expose the most recent minidump path to the AppCrashException and add utilities
to retrieve and symbolize all minidump paths from the browser.

BUG=catapult:#2346

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-07-11 18:30:35]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[commit-bot: I haz the power, 2016-07-11 18:30:38]

CQ bit was unchecked.