Add GPU fingerprint information to breakpad microdumps.

Add GPU fingerprint information to breakpad microdumps.

Although strictly the GPU fingerprint is defined by the build fingerprint,
there is not currently a straightforward mapping from build fingerprint
to useful GPU / GL driver information.

In order to aid debugging of WebView crashes that occur in GL drivers,
and to better understand the range of drivers and versions for feature
blacklisting purposes, it is useful to have GPU fingerprints in breakpad
microdumps.

Landing this patch on behalf of Tobias Sargeant<tobiasjs@chromium.org>;

BUG=chromium:536769
R=primiano@chromium.org, thestig@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/7ecc65758d808c7c3a954141addadc3c84512a75

[Tobias Sargeant, 2015-09-09 11:33:29]

tobiasjs@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-09-09 18:59:34]

https://codereview.chromium.org/1334473003/diff/1/src/client/linux/handler/mi...
File src/client/linux/handler/minidump_descriptor.h (right):

https://codereview.chromium.org/1334473003/diff/1/src/client/linux/handler/mi...
src/client/linux/handler/minidump_descriptor.h:126: void
SetMicrodumpGPUFingerprint(const char* gpu_fingerprint);
At this point feels like we should just have one method which is:
SetMicrodumpMetadata which takes a struct. So the next time we need something
more (my guts say won't take too long) we don't have to repeat all this plumbing
again (and risk to forget to plumb the NULL assignments). WDYT?

Also this could resolve the todo above.
Welcome to: "since you're changing this, would you mind also..." :)

https://codereview.chromium.org/1334473003/diff/1/src/client/linux/microdump_...
File src/client/linux/microdump_writer/microdump_writer_unittest.cc (right):

https://codereview.chromium.org/1334473003/diff/1/src/client/linux/microdump_...
src/client/linux/microdump_writer/microdump_writer_unittest.cc:113: const string
&expected_product_info) {
would be nice if this was covering also the gpu fingerprint here

[Lei Zhang, 2015-09-09 19:16:38]

(drive by with only a brief look) If we do this, should we do it for minidumps
as well? Then remove the redundant bits in Chrome's breakpad handler.

[Primiano Tucci (use gerrit), 2015-09-09 19:19:45]

On 2015/09/09 19:16:38, Lei Zhang wrote:
> (drive by with only a brief look) If we do this, should we do it for minidumps
> as well? Then remove the redundant bits in Chrome's breakpad handler.

Very interesting point: IIRC we already have some GPU-related info coming from
minidumps. Or is that added by the embedder using the extra mime fields we
support in minidumps (but not in micro)?
Worth double checking how do we get the GPU info in the standard (non-microdump)
case.

[Tobias Sargeant, 2015-09-10 10:32:32]

On 2015/09/09 19:19:45, Primiano Tucci wrote:
> On 2015/09/09 19:16:38, Lei Zhang wrote:
> > (drive by with only a brief look) If we do this, should we do it for
minidumps
> > as well? Then remove the redundant bits in Chrome's breakpad handler.
> 
> Very interesting point: IIRC we already have some GPU-related info coming from
> minidumps. Or is that added by the embedder using the extra mime fields we
> support in minidumps (but not in micro)?
> Worth double checking how do we get the GPU info in the standard
(non-microdump)
> case.

GPU info is set as a series of crash keys in ChromeContentClient::SetGpuInfo(),
which doesn't apply for WebView. I don't think there's any redundant code to
remove here.

[Tobias Sargeant, 2015-09-10 10:45:04]

https://codereview.chromium.org/1334473003/diff/1/src/client/linux/handler/mi...
File src/client/linux/handler/minidump_descriptor.h (right):

https://codereview.chromium.org/1334473003/diff/1/src/client/linux/handler/mi...
src/client/linux/handler/minidump_descriptor.h:126: void
SetMicrodumpGPUFingerprint(const char* gpu_fingerprint);
On 2015/09/09 18:59:34, Primiano Tucci wrote:
> At this point feels like we should just have one method which is:
> SetMicrodumpMetadata which takes a struct. So the next time we need something
> more (my guts say won't take too long) we don't have to repeat all this
plumbing
> again (and risk to forget to plumb the NULL assignments). WDYT?
> 
> Also this could resolve the todo above.
> Welcome to: "since you're changing this, would you mind also..." :)

Done.

https://codereview.chromium.org/1334473003/diff/1/src/client/linux/microdump_...
File src/client/linux/microdump_writer/microdump_writer_unittest.cc (right):

https://codereview.chromium.org/1334473003/diff/1/src/client/linux/microdump_...
src/client/linux/microdump_writer/microdump_writer_unittest.cc:113: const string
&expected_product_info) {
On 2015/09/09 18:59:34, Primiano Tucci wrote:
> would be nice if this was covering also the gpu fingerprint here

Done.

[Primiano Tucci (use gerrit), 2015-09-10 12:38:57]

primiano@chromium.org changed reviewers:
+ thestig@chromium.org

[Primiano Tucci (use gerrit), 2015-09-10 12:38:58]

Looks good, just some final comments and I think you can remove quite some code
at this point.

Also I looked at the current way we report GPU info on classic breakpad crash
reporting and we seem to use crash keys there, not embedding in the minidump
(See chrome/common/crash_keys.h)

IIUC crash keys are not part of the minidump file but it's some kind of
extra-metadata for the uploader.
In which case this approach here in this CL makes sense to me. WDYT Lei?

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
File src/client/linux/handler/microdump_extra_info.h (right):

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
src/client/linux/handler/microdump_extra_info.h:43: MicrodumpExtraInfo(const
char* build_fingerprint_val,
please add a comment clarifying that all the const* char strings here must be
guaranteed to be long-lived (i.e. they should be either literals or strduped
from the callers)

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
File src/client/linux/handler/minidump_descriptor.h (right):

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:78: // TODO(tobiasjs): remove
this ctor and associated struct once the
I think you can already remove this here and fix the code on the chromiuim side
once we roll.
I think we established that microdumps "stick" in chrome and your patch will be
unlikely to get reverted in chrome (which was the reason why I had the setters
here)

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:116: void
SetMicrodumpExtraInfo(const MicrodumpExtraInfo &microdump_extra_info);
Similarly, you could remove this also

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:148: // The product name/version
and build fingerprint that should be appended to
Fix the comment plz, maybe just say
The extra microdump metadata (e.g., product name, version, OS information etc)

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/microd...
File src/client/linux/microdump_writer/microdump_writer_unittest.cc (right):

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer_unittest.cc:220: const char
kGPUFingerprint[] = "bazqux";
you probably want to check here that GPUfinerprint == null doesn't crash and
writes a still valid microdump

[Tobias Sargeant, 2015-09-10 13:18:39]

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
File src/client/linux/handler/microdump_extra_info.h (right):

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
src/client/linux/handler/microdump_extra_info.h:43: MicrodumpExtraInfo(const
char* build_fingerprint_val,
On 2015/09/10 12:38:58, Primiano Tucci wrote:
> please add a comment clarifying that all the const* char strings here must be
> guaranteed to be long-lived (i.e. they should be either literals or strduped
> from the callers)

Done.

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
File src/client/linux/handler/minidump_descriptor.h (right):

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:78: // TODO(tobiasjs): remove
this ctor and associated struct once the
On 2015/09/10 12:38:58, Primiano Tucci wrote:
> I think you can already remove this here and fix the code on the chromiuim
side
> once we roll.
> I think we established that microdumps "stick" in chrome and your patch will
be
> unlikely to get reverted in chrome (which was the reason why I had the setters
> here)

Done.

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:116: void
SetMicrodumpExtraInfo(const MicrodumpExtraInfo &microdump_extra_info);
On 2015/09/10 12:38:58, Primiano Tucci wrote:
> Similarly, you could remove this also

Done.

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:148: // The product name/version
and build fingerprint that should be appended to
On 2015/09/10 12:38:58, Primiano Tucci wrote:
> Fix the comment plz, maybe just say
> The extra microdump metadata (e.g., product name, version, OS information etc)

Done.

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/microd...
File src/client/linux/microdump_writer/microdump_writer_unittest.cc (right):

https://codereview.chromium.org/1334473003/diff/40001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer_unittest.cc:220: const char
kGPUFingerprint[] = "bazqux";
On 2015/09/10 12:38:58, Primiano Tucci wrote:
> you probably want to check here that GPUfinerprint == null doesn't crash and
> writes a still valid microdump

Done.

[Primiano Tucci (use gerrit), 2015-09-10 13:58:31]

LGTM. But plz wait for thestig review.

Also can you make sure that
./configure && make -j40 && make check -j40
is happy?

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
File src/client/linux/handler/microdump_extra_info.h (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
src/client/linux/handler/microdump_extra_info.h:45: MicrodumpExtraInfo(const
char* build_fingerprint_val,
Are you going to use this ctor on the other side or fill the args manually? If
not remove it, if yes nevermind.

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
File src/client/linux/handler/minidump_descriptor.h (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:144: // upfront.  The string
pointers are supposed to be valid through
As you moved this comment to extra info, you could remove the "The string
pointers" part here

[Lei Zhang, 2015-09-10 18:38:19]

lgtm, it's just all nits below

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
File src/client/linux/handler/minidump_descriptor.h (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:57: microdump_extra_info_() {}
probably unnecessary, ditto below

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:109: assert(mode_ ==
kWriteMicrodumpToConsole);
just call IsMicrodumpOnConsole() ?

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
File src/client/linux/microdump_writer/microdump_writer.cc (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer.cc:41: #include
"client/linux/handler/microdump_extra_info.h"
h before l -> move up 1 line

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer.cc:415: const
MicrodumpExtraInfo &microdump_extra_info) {
nit: & followed by space

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
File src/client/linux/microdump_writer/microdump_writer.h (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer.h:58: pid_t crashing_process,
const void* blob, size_t blob_size,
I like 1 param per line

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
File src/client/linux/microdump_writer/microdump_writer_unittest.cc (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer_unittest.cc:56: const
MicrodumpExtraInfo &microdump_extra_info,
& ditto

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer_unittest.cc:111: const
MicrodumpExtraInfo &expected_info) {
& ditto ditto

[Tobias Sargeant, 2015-09-11 09:07:50]

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
File src/client/linux/handler/microdump_extra_info.h (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
src/client/linux/handler/microdump_extra_info.h:45: MicrodumpExtraInfo(const
char* build_fingerprint_val,
On 2015/09/10 13:58:31, Primiano Tucci wrote:
> Are you going to use this ctor on the other side or fill the args manually? If
> not remove it, if yes nevermind.

Done.

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
File src/client/linux/handler/minidump_descriptor.h (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:57: microdump_extra_info_() {}
On 2015/09/10 18:38:19, Lei Zhang wrote:
> probably unnecessary, ditto below

Done.

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:109: assert(mode_ ==
kWriteMicrodumpToConsole);
On 2015/09/10 18:38:19, Lei Zhang wrote:
> just call IsMicrodumpOnConsole() ?

Done.

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/handle...
src/client/linux/handler/minidump_descriptor.h:144: // upfront.  The string
pointers are supposed to be valid through
On 2015/09/10 13:58:31, Primiano Tucci wrote:
> As you moved this comment to extra info, you could remove the "The string
> pointers" part here

Done.

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
File src/client/linux/microdump_writer/microdump_writer.cc (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer.cc:41: #include
"client/linux/handler/microdump_extra_info.h"
On 2015/09/10 18:38:19, Lei Zhang wrote:
> h before l -> move up 1 line

Done.

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer.cc:415: const
MicrodumpExtraInfo &microdump_extra_info) {
On 2015/09/10 18:38:19, Lei Zhang wrote:
> nit: & followed by space

Done.

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
File src/client/linux/microdump_writer/microdump_writer.h (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer.h:58: pid_t crashing_process,
const void* blob, size_t blob_size,
On 2015/09/10 18:38:19, Lei Zhang wrote:
> I like 1 param per line

Done.

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
File src/client/linux/microdump_writer/microdump_writer_unittest.cc (right):

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer_unittest.cc:56: const
MicrodumpExtraInfo &microdump_extra_info,
On 2015/09/10 18:38:19, Lei Zhang wrote:
> & ditto

Done.

https://codereview.chromium.org/1334473003/diff/60001/src/client/linux/microd...
src/client/linux/microdump_writer/microdump_writer_unittest.cc:111: const
MicrodumpExtraInfo &expected_info) {
On 2015/09/10 18:38:19, Lei Zhang wrote:
> & ditto ditto

Done.

[Primiano Tucci (use gerrit), 2015-09-28 12:53:00]

Committed patchset #5 (id:80001) manually as
7ecc65758d808c7c3a954141addadc3c84512a75 (presubmit successful).