Removing the .stripped suffix from minidumps produced by crashpad.

Removing the .stripped suffix from minidumps produced by crashpad.  

Some linux platforms still use breakpad so this logic remains for minidumps obtained from breakpad.

BUG=chromium:667475
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/73c66c72e5028d3fc850cc6040bb8dc6a167f588

[eyaich1, 2016-12-01 22:28:41]

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

[Ken Russell (switch to Gerrit), 2016-12-01 22:43:04]

https://codereview.chromium.org/2545933002/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(right):

https://codereview.chromium.org/2545933002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:499:
outfile.write(''.join(infile.read().partition('MDMP')[1:]))
Per mark's comment in
https://bugs.chromium.org/p/chromium/issues/detail?id=667475#c12 , I really
think we should just delete this entire block of logic.

[Mark Mentovai, 2016-12-01 22:46:56]

mark@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2016-12-01 22:46:57]

https://codereview.chromium.org/2545933002/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(right):

https://codereview.chromium.org/2545933002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:540:
return [report[1] for report in reports_list]
Here is where you know you’ve gotten minidumps from Crashpad.

[Mark Mentovai, 2016-12-01 22:47:08]

Description was changed from

==========
Removing the .stripped suffix from minidumps produced on macs.

Still need to investigate if this can be removed from win/linux.

BUG=chromium:667475
==========

to

==========
Removing the .stripped suffix from minidumps produced on macs.

Still need to investigate if this can be removed from win/linux.

BUG=chromium:667475
==========

[Mark Mentovai, 2016-12-01 22:47:09]

mark@chromium.org changed reviewers:
- mark@chromium.org

[nednguyen, 2016-12-01 23:01:28]

lgtm whenever Ken & Mark are happy

[eyaich1, 2016-12-01 23:13:00]

https://codereview.chromium.org/2545933002/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(right):

https://codereview.chromium.org/2545933002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:499:
outfile.write(''.join(infile.read().partition('MDMP')[1:]))
On 2016/12/01 22:43:04, Ken Russell wrote:
> Per mark's comment in
> https://bugs.chromium.org/p/chromium/issues/detail?id=667475#c12 , I really
> think we should just delete this entire block of logic.

Done.

https://codereview.chromium.org/2545933002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:540:
return [report[1] for report in reports_list]
On 2016/12/01 22:46:56, Mark Mentovai wrote:
> Here is where you know you’ve gotten minidumps from Crashpad.

Acknowledged.

[Mark Mentovai, 2016-12-01 23:20:21]

mark@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2016-12-01 23:20:22]

Update the CL description to reflect what this winds up doing.

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(left):

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:498:
outfile.write(''.join(infile.read().partition('MDMP')[1:]))
This still may have utility on Linux using Breakpad.

[Ken Russell (switch to Gerrit), 2016-12-01 23:39:51]

LGTM once Mark's comment is clarified.

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(left):

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:498:
outfile.write(''.join(infile.read().partition('MDMP')[1:]))
On 2016/12/01 23:20:22, Mark Mentovai wrote:
> This still may have utility on Linux using Breakpad.

Could you clarify this more please Mark? Should we leave in this logic on Linux
or not? Is top-of-tree Chromium on Linux using Breakpad or Crashpad? We don't
need to support earlier releases (this logic is mainly useful on the bots, which
are running top-of-tree). Thanks.

[Mark Mentovai, 2016-12-02 00:51:50]

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(left):

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:498:
outfile.write(''.join(infile.read().partition('MDMP')[1:]))
On 2016/12/01 23:39:51, Ken Russell wrote:
> On 2016/12/01 23:20:22, Mark Mentovai wrote:
> > This still may have utility on Linux using Breakpad.
> 
> Could you clarify this more please Mark? Should we leave in this logic on
Linux
> or not? Is top-of-tree Chromium on Linux using Breakpad or Crashpad? We don't
> need to support earlier releases (this logic is mainly useful on the bots,
which
> are running top-of-tree). Thanks.

I’m concerned that this look-for-MDMP-marker thing was added to deal with
https://chromium.googlesource.com/chromium/src/+/cc607cc759e50fbb2dc2d0395952....

We’ve only ever done that kind of thing for Breakpad and only for Linux-based
platforms. We’re not currently using a Crashpad client for any Linux-based
platforms, although I’m working on one right now. The Crashpad client for
Linuxes will be the same as the one for Mac and Windows: it will own its
database, and it’d be erroneous for outside code like this to mess around and
create files in it. But when it writes minidump files in its database and you
retrieve their paths with crashpad_database_util, they’ll be real minidumps, not
some MIMEy files like Chrome has for Breakpad on Linux.

So…

I think that the reason that this block was added is still valid. But this block
is not valid for any Crashpad-using platforms, and it hurts them. It would also
not be necessary for other non-Linux Breakpad-using platforms, but it probably
wouldn’t hurt them either.

That’s why the best thing that I can suggest is to figure out whether you got a
dump from Crashpad based on whether you ran crashpad_database_util (the thing I
pointed out previously). If it came from Crashpad, don’t do this, regardless of
platform. If it came from Breakpad, then do this, or only do it on Linuxish
systems.

This should take care of the Breakpad frankenfiles in use today, be compatible
with Crashpad by not mucking around in its internal database unneccessarily, and
be compatible with Linuxes that switch to Crashpad in the future.

[Ken Russell (switch to Gerrit), 2016-12-02 01:08:23]

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(left):

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:498:
outfile.write(''.join(infile.read().partition('MDMP')[1:]))
On 2016/12/02 00:51:50, Mark Mentovai wrote:
> On 2016/12/01 23:39:51, Ken Russell wrote:
> > On 2016/12/01 23:20:22, Mark Mentovai wrote:
> > > This still may have utility on Linux using Breakpad.
> > 
> > Could you clarify this more please Mark? Should we leave in this logic on
> Linux
> > or not? Is top-of-tree Chromium on Linux using Breakpad or Crashpad? We
don't
> > need to support earlier releases (this logic is mainly useful on the bots,
> which
> > are running top-of-tree). Thanks.
> 
> I’m concerned that this look-for-MDMP-marker thing was added to deal with
>
https://chromium.googlesource.com/chromium/src/+/cc607cc759e50fbb2dc2d0395952....
> 
> We’ve only ever done that kind of thing for Breakpad and only for Linux-based
> platforms. We’re not currently using a Crashpad client for any Linux-based
> platforms, although I’m working on one right now. The Crashpad client for
> Linuxes will be the same as the one for Mac and Windows: it will own its
> database, and it’d be erroneous for outside code like this to mess around and
> create files in it. But when it writes minidump files in its database and you
> retrieve their paths with crashpad_database_util, they’ll be real minidumps,
not
> some MIMEy files like Chrome has for Breakpad on Linux.
> 
> So…
> 
> I think that the reason that this block was added is still valid. But this
block
> is not valid for any Crashpad-using platforms, and it hurts them. It would
also
> not be necessary for other non-Linux Breakpad-using platforms, but it probably
> wouldn’t hurt them either.
> 
> That’s why the best thing that I can suggest is to figure out whether you got
a
> dump from Crashpad based on whether you ran crashpad_database_util (the thing
I
> pointed out previously). If it came from Crashpad, don’t do this, regardless
of
> platform. If it came from Breakpad, then do this, or only do it on Linuxish
> systems.
> 
> This should take care of the Breakpad frankenfiles in use today, be compatible
> with Crashpad by not mucking around in its internal database unneccessarily,
and
> be compatible with Linuxes that switch to Crashpad in the future.

I see. Thanks for the clarification. I missed your comments earlier in the
thread.

Emily, I think it would be safest to change the logic so that this class at
least keeps track of whether it most recently got the minidump from Crashpad or
Breakpad, and run this logic for the Breakpad dumps. Otherwise we run the risk
of breaking symbolized stacks on the Linux bots, which would be just as bad. Of
course, attaching that information to the minidump somehow would be ideal, too
(or keeping a map inside this class with that information).

[eyaich1, 2016-12-02 17:19:31]

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py
(left):

https://codereview.chromium.org/2545933002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py:498:
outfile.write(''.join(infile.read().partition('MDMP')[1:]))
On 2016/12/02 01:08:23, Ken Russell wrote:
> On 2016/12/02 00:51:50, Mark Mentovai wrote:
> > On 2016/12/01 23:39:51, Ken Russell wrote:
> > > On 2016/12/01 23:20:22, Mark Mentovai wrote:
> > > > This still may have utility on Linux using Breakpad.
> > > 
> > > Could you clarify this more please Mark? Should we leave in this logic on
> > Linux
> > > or not? Is top-of-tree Chromium on Linux using Breakpad or Crashpad? We
> don't
> > > need to support earlier releases (this logic is mainly useful on the bots,
> > which
> > > are running top-of-tree). Thanks.
> > 
> > I’m concerned that this look-for-MDMP-marker thing was added to deal with
> >
>
https://chromium.googlesource.com/chromium/src/+/cc607cc759e50fbb2dc2d0395952....
> > 
> > We’ve only ever done that kind of thing for Breakpad and only for
Linux-based
> > platforms. We’re not currently using a Crashpad client for any Linux-based
> > platforms, although I’m working on one right now. The Crashpad client for
> > Linuxes will be the same as the one for Mac and Windows: it will own its
> > database, and it’d be erroneous for outside code like this to mess around
and
> > create files in it. But when it writes minidump files in its database and
you
> > retrieve their paths with crashpad_database_util, they’ll be real minidumps,
> not
> > some MIMEy files like Chrome has for Breakpad on Linux.
> > 
> > So…
> > 
> > I think that the reason that this block was added is still valid. But this
> block
> > is not valid for any Crashpad-using platforms, and it hurts them. It would
> also
> > not be necessary for other non-Linux Breakpad-using platforms, but it
probably
> > wouldn’t hurt them either.
> > 
> > That’s why the best thing that I can suggest is to figure out whether you
got
> a
> > dump from Crashpad based on whether you ran crashpad_database_util (the
thing
> I
> > pointed out previously). If it came from Crashpad, don’t do this, regardless
> of
> > platform. If it came from Breakpad, then do this, or only do it on Linuxish
> > systems.
> > 
> > This should take care of the Breakpad frankenfiles in use today, be
compatible
> > with Crashpad by not mucking around in its internal database unneccessarily,
> and
> > be compatible with Linuxes that switch to Crashpad in the future.
> 
> I see. Thanks for the clarification. I missed your comments earlier in the
> thread.
> 
> Emily, I think it would be safest to change the logic so that this class at
> least keeps track of whether it most recently got the minidump from Crashpad
or
> Breakpad, and run this logic for the Breakpad dumps. Otherwise we run the risk
> of breaking symbolized stacks on the Linux bots, which would be just as bad.
Of
> course, attaching that information to the minidump somehow would be ideal, too
> (or keeping a map inside this class with that information).

Done.

[Mark Mentovai, 2016-12-02 17:21:36]

LGTM

[Mark Mentovai, 2016-12-02 17:21:45]

Update the change description again

[Ken Russell (switch to Gerrit), 2016-12-02 17:23:06]

Awesome. Thanks Emily for being so thorough with this fix. LGTM

(It would be ideal if there were unittests for this)

[eyaich1, 2016-12-02 17:24:18]

Description was changed from

==========
Removing the .stripped suffix from minidumps produced on macs.

Still need to investigate if this can be removed from win/linux.

BUG=chromium:667475
==========

to

==========
Removing the .stripped suffix from minidumps produced by crashpad.  

Some linux platforms still use breakpad so this logic remains for minidumps
obtained from breakpad.

BUG=chromium:667475
==========

[eyaich1, 2016-12-02 17:28:47]

On 2016/12/02 17:23:06, Ken Russell wrote:
> Awesome. Thanks Emily for being so thorough with this fix. LGTM
> 
> (It would be ideal if there were unittests for this)

Yes, I know.  Unittests/integration tests have been notoriously flaky for this. 
It actually doesn't look like we have a unittest for this file at all.  I will
investigate that and file a bug with the correct path.  I still have an
outsanding bug to reenable the flaky integration tests (crbug.com/633761) since
that seems the best way to test this given we don't have access to the binaries
in our telemetry unittests.  

I am on a mission to understand crashpad/breakpad and stack traces/crashes in
general in telemetry and these tests are in the queue once I make sure it is
indeed doing what we want/need.

[eyaich1, 2016-12-02 17:29:01]

The CQ bit was checked by eyaich@chromium.org

[eyaich1, 2016-12-02 17:29:01]

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/2545933002/#ps60001
(title: "Inverse crashpad logic")

[commit-bot: I haz the power, 2016-12-02 17:29:13]

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

[Ken Russell (switch to Gerrit), 2016-12-02 17:34:13]

The CQ bit was unchecked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-12-02 17:35:39]

On 2016/12/02 17:29:13, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Sorry, I unchecked the CQ box to edit the Issue Subject to match the new Issue
Description. Not sure whether that was strictly necessary, but re-CQ'ing again.

[Ken Russell (switch to Gerrit), 2016-12-02 17:35:46]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2016-12-02 17:35:48]

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-12-02 17:47:53]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480700146230860,
"parent_rev": "a631bc329824c6f72f54afa7b747246c6c118f24", "commit_rev":
"73c66c72e5028d3fc850cc6040bb8dc6a167f588"}

[commit-bot: I haz the power, 2016-12-02 17:47:58]

Description was changed from

==========
Removing the .stripped suffix from minidumps produced by crashpad.  

Some linux platforms still use breakpad so this logic remains for minidumps
obtained from breakpad.

BUG=chromium:667475
==========

to

==========
Removing the .stripped suffix from minidumps produced by crashpad.  

Some linux platforms still use breakpad so this logic remains for minidumps
obtained from breakpad.

BUG=chromium:667475

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

[commit-bot: I haz the power, 2016-12-02 17:47:59]

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