1319071 - CrashManager.pendingDumps() does not recognize Linux minidumps correctly

Status: VERIFIED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #1293656 +++

See bug 1293656 comment 39: for some reason breakpad uses a different filename convention on Linux with an 8-4-4-8-8 arrangement for the GUID instead of the traditional 8-4-4-4-12. This has been in breakpad for ages and I couldn't find the rationale for it.

We should probably fix it. That method is not used on currently used on desktop Firefox from what I could tell but if we intend to give the crash dumps a more rational lifecycle it's better if it just works.

Comment 1

[(not currently active) Ted Mielczarek]

(In reply to Gabriele Svelto [:gsvelto] from comment #0)
> See bug 1293656 comment 39: for some reason breakpad uses a different
> filename convention on Linux with an 8-4-4-8-8 arrangement for the GUID
> instead of the traditional 8-4-4-4-12. This has been in breakpad for ages
> and I couldn't find the rationale for it.

I think this is just a mistake. I'm amazed that nobody noticed it all this time!

> We should probably fix it. That method is not used on currently used on
> desktop Firefox from what I could tell but if we intend to give the crash
> dumps a more rational lifecycle it's better if it just works.

We do use this code. We pass a MinidumpDescriptor to the ExceptionHandler constructor on Linux:
https://dxr.mozilla.org/mozilla-central/rev/2963cf6be7f830c0d2155e2968cfc53585868a76/toolkit/crashreporter/nsExceptionHandler.cpp#1722

which winds up calling UpdatePath, which calls CreateGUID:
https://dxr.mozilla.org/mozilla-central/rev/2963cf6be7f830c0d2155e2968cfc53585868a76/toolkit/crashreporter/google-breakpad/src/client/linux/handler/minidump_descriptor.cc#78

For out-of-process dumps the Linux code calls CreateGUID explicitly:
https://dxr.mozilla.org/mozilla-central/rev/2963cf6be7f830c0d2155e2968cfc53585868a76/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/crash_generation_server.cc#314

Comment 2

[Gabriele Svelto [:gsvelto]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> I think this is just a mistake. I'm amazed that nobody noticed it all this
> time!

I also think that's a mistake but it's been around for so long I was wondering if it's worth filing a bug against Breakpad or not. It's a single-line fix in src/common/linux/guid_creator.h.

> We do use this code.

I meant we don't use CrashManager.pendingDumps() which seems the only piece of code affected by this.

> We pass a MinidumpDescriptor to the ExceptionHandler
> constructor on Linux:
> https://dxr.mozilla.org/mozilla-central/rev/
> 2963cf6be7f830c0d2155e2968cfc53585868a76/toolkit/crashreporter/
> nsExceptionHandler.cpp#1722
> 
> which winds up calling UpdatePath, which calls CreateGUID:
> https://dxr.mozilla.org/mozilla-central/rev/
> 2963cf6be7f830c0d2155e2968cfc53585868a76/toolkit/crashreporter/google-
> breakpad/src/client/linux/handler/minidump_descriptor.cc#78
> 
> For out-of-process dumps the Linux code calls CreateGUID explicitly:
> https://dxr.mozilla.org/mozilla-central/rev/
> 2963cf6be7f830c0d2155e2968cfc53585868a76/toolkit/crashreporter/google-
> breakpad/src/client/linux/crash_generation/crash_generation_server.cc#314

Do you think it's worth pursuing this in Breakpad? If that's the case I'll file a bug right away so that if the fix sticks we can pull it in the next time we sync the code.

Comment 3

[(not currently active) Ted Mielczarek]

It's not that big of a deal but it is a weird correctness thing and the fix is small, so I'd say we might as well fix it.

Comment 4

[(not currently active) Ted Mielczarek]

This was bothering me because it reminded me of some other bug and it took me a while to find it: bug 1138399. I'm 100% positive this is the root cause of that bug. Because of this pending crashes don't show up on Linux builds. :-(

This code in CrashReports.jsm uses a UUID regex and that's what populates the list of pending reports in about:crashes:
https://dxr.mozilla.org/mozilla-central/rev/2963cf6be7f830c0d2155e2968cfc53585868a76/toolkit/crashreporter/CrashReports.jsm#42

Comment 5

[Gabriele Svelto [:gsvelto]]

I'll fix the issue in Breakpad then. BTW while looking at the GUID-generation code I've noticed that on Linux they're getting random data using random() and setting the seed with the infamous srandom(time(NULL)) technique. I wonder if that would merit a fix too since it makes crash GUIDs essentially predictable.

Comment 6

[Gabriele Svelto [:gsvelto]]

Quick update: I've got a working patch in breakpad but it needs some adjustments so that it will build cleanly when using stand-alone breakpad while also playing nice with our build system.

Comment 7

[Gabriele Svelto [:gsvelto]]

I've filed this breakpad bug to fix this: https://bugs.chromium.org/p/google-breakpad/issues/detail?id=727

I've got a patch ready for it but I'm still testing it on our side to be sure it can be uplifted cleanly once it lands in breakpad.

Comment 8

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Fix minidump name generation on Linux

This is a WIP of the patch I wrote for Breakpad that applies to mozilla-central. I've tested in on 64-bit Linux and it's working fine but I have to check Fennec too before pushing it to upstream.

Comment 9

[Gabriele Svelto [:gsvelto]]

I've manually tested the patch on Fennec and it does seem to work fine too. I'll ask for review upstream and then ensure we pull this change into mozilla-central.

Here's a try run, just in case:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef9649e18a2371ec6adcca51fa01dd3b7bda7a1e

Comment 10

[Gabriele Svelto [:gsvelto]]

Upstream patch/review is here: https://chromium-review.googlesource.com/c/440086/

Comment 11

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8835479 [details] [diff] [review]
[PATCH WIP] Fix minidump name generation on Linux

Obsoleting this, I'll replace it with a patch to our forked sources so we don't have to go through upstream.

Comment 12

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1319071 - Make crash minidumps use the same format for filenames on Linux as on other platforms;

This patch forks the breakpad files used for GUID generation and replaces them
with copies living together with the rest of the forked sources. Since other
files in the breakpad sources rely on the original files being present they
haven't been removed but we deliberately define their inclusion definitions in
the build system to prevent them from contributing to the build. This ensures
that we always use our own version of the GUID generator while not touching
the non-forked sources.

Review commit: https://reviewboard.mozilla.org/r/121384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121384/

Comment 13

[Gabriele Svelto [:gsvelto]]

Try run for the new patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fca0976a645e144d7797f5a86b3ef936f7d0a1ba

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 8848467 [details]
Bug 1319071 - Make crash minidumps use the same format for filenames on Linux as on other platforms;

https://reviewboard.mozilla.org/r/121384/#review123902

Such a silly bug to hang around for all these years!

::: toolkit/crashreporter/breakpad-client/linux/moz.build:22
(Diff revision 1)
>      'minidump_writer/linux_dumper.cc',
>      'minidump_writer/linux_ptrace_dumper.cc',
>      'minidump_writer/minidump_writer.cc',
>  ]
>  
> +# On Linux we override the guid_creator.h header and use our own instead

AFAICT the only place outside of breakpad-client that includes this file is:
https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/crashreporter/google-breakpad/src/common/linux/file_id.h#39

...and it doesn't seem to actually use it for anything. I suspect I may have left that as a vestigal remnant when I changed things to support Build IDs and no longer needed the `kGUID...` constants. If we just remove that #include does the build work without that? If so I can just land that fix upstream.

Comment 15

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8848467 [details]
Bug 1319071 - Make crash minidumps use the same format for filenames on Linux as on other platforms;

https://reviewboard.mozilla.org/r/121384/#review123902

Thanks for the quick review!

> AFAICT the only place outside of breakpad-client that includes this file is:
> https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/crashreporter/google-breakpad/src/common/linux/file_id.h#39
> 
> ...and it doesn't seem to actually use it for anything. I suspect I may have left that as a vestigal remnant when I changed things to support Build IDs and no longer needed the `kGUID...` constants. If we just remove that #include does the build work without that? If so I can just land that fix upstream.

I think it does, without the include guards I was seeing only one error because the constants were being redefined so I assume it was because of that file.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28dfd89c8e5d
Make crash minidumps use the same format for filenames on Linux as on other platforms; r=ted

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/28dfd89c8e5d

Comment 18

[Abe - QA (:Abe_LV)]

QA verified as fixed.

Here is the link to test cases and runs: https://public.etherpad-mozilla.org/p/1319071

Please needinfo me or send me an email if you have questions about the document or the testing. Thanks

Comment 20

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8848467 [details]
Bug 1319071 - Make crash minidumps use the same format for filenames on Linux as on other platforms;

Approval Request Comment
[Feature/Bug causing the regression]: Upstream Breakpad bug https://bugs.chromium.org/p/google-breakpad/issues/detail?id=727
[User impact if declined]: Content process crashes are not visible in about:crashes on Linux
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: No
[Why is the change risky/not risky?]: This only changes the minidump file names generated when a crash occurs, it has not impact on the rest of the codebase
[String changes made/needed]: None

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8848467 [details]
Bug 1319071 - Make crash minidumps use the same format for filenames on Linux as on other platforms;

54 is on Beta now.

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8848467 [details]
Bug 1319071 - Make crash minidumps use the same format for filenames on Linux as on other platforms;

Fix an invisible content process crashes issue in about:crashes on Linux. Beta 54+. Should be in 54 beta 1.

Comment 23

[Iris Hsiao [:ihsiao]]

Pushed: https://hg.mozilla.org/releases/mozilla-beta/rev/e0340a68ed92 - Gabriele Svelto - Bug 1319071 - Make crash minidumps use the same format for filenames on Linux as on other platforms; r=ted. a=gchang

Comment 24

[Andrei Vaida [:avaida]]

(In reply to Gabriele Svelto [:gsvelto] from comment #20)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Gabriele's assessment on manual testing needs and the fact that this fix has automated coverage.