Note: There are a few cases of duplicates in user autocompletion which are being worked on.

CrashManager.pendingDumps() does not recognize Linux minidumps correctly

VERIFIED FIXED in Firefox 54

Status

()

Toolkit
Crash Reporting
VERIFIED FIXED
8 months ago
3 months ago

People

(Reporter: gsvelto, Assigned: gsvelto)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [fce-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

+++ 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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
(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
(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.
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
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
Blocks: 1138399
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.
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.

Updated

6 months ago
Whiteboard: [fce-active]

Updated

6 months ago
Blocks: 1293656
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.
Created attachment 8835479 [details] [diff] [review]
[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.
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
Upstream patch/review is here: https://chromium-review.googlesource.com/c/440086/
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.
Attachment #8835479 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Try run for the new patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fca0976a645e144d7797f5a86b3ef936f7d0a1ba

Comment 14

4 months ago
mozreview-review
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.
Attachment #8848467 - Flags: review?(ted) → review+
(Assignee)

Comment 15

4 months ago
mozreview-review-reply
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

4 months ago
Pushed by gsvelto@mozilla.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28dfd89c8e5d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 18

4 months ago
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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1138399
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
Attachment #8848467 - Flags: approval-mozilla-aurora?
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.
Attachment #8848467 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?

Updated

3 months ago
status-firefox54: --- → affected
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.
Attachment #8848467 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 23

3 months ago
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
status-firefox54: affected → fixed
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.