Closed Bug 1319071 Opened 3 years ago Closed 3 years ago

CrashManager.pendingDumps() does not recognize Linux minidumps correctly

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

(Whiteboard: [fce-active-legacy])

Attachments

(1 file, 1 obsolete file)

+++ 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.
Whiteboard: [fce-active]
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.
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 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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/28dfd89c8e5d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
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+
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
(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-
Whiteboard: [fce-active] → [fce-active-legacy]
You need to log in before you can comment on or make changes to this bug.