Closed Bug 1317968 Opened 3 years ago Closed 3 years ago

Add the minidump-analyzer tool to Firefox' distribution

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

Attachments

(1 file)

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

While inspecting recent crash pings I've noticed that the average size of the pings hadn't changed, which was suspicious since we should have had stack traces on nightly for a while.

As it turns out the patch for bug 1280477 had a major issue: after building the minidump-analyzer it failed to package it with the installer; thus the analyzer has never been distributed yet :-/

I'm working on a patch to fix this ASAP.
Summary: Store stack traces into the crash.main event file for each browser crash → Add the minidump-analyzer tool to Firefox' distribution
This should add the minidump-analyzer to all the relevant packaging steps. I've tested across the different architectures and it seems to work correctly.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8811241 - Flags: review?(ted)
Comment on attachment 8811241 [details] [diff] [review]
minidump-analyzer-installer.patch

Review of attachment 8811241 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh. I hate our packaging story. What a pain! On the flip side: this is why you should figure out how to write some tests for this. :)
Attachment #8811241 - Flags: review?(ted) → review+
Thanks for the quick review Ted! Yeah, I really need to sit down and write some tests but I've been caught in trying to make all this machinery work first :(

Anyway, the try run is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=340eb4555f499509a779da2a255941e1828a921f
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75fc07efe46f
Package the minidump-analyzer tool r=ted
Hmm, these all appear to have happened on the push AFTER yours, and only on that push, the following pushes are green. Weird.
(In reply to Wes Kocher (:KWierso) from comment #6)
> Hmm, these all appear to have happened on the push AFTER yours, and only on
> that push, the following pushes are green. Weird.

Unless this bustage is only on linux opt/pgo, in which case, the push after yours is the only one so far since you've landed that has run reftests on those build types...
Blocks: 1318758
(In reply to Wes Kocher (:KWierso) from comment #5)
> This appears to have caused some reftest crashes like
> https://treeherder.mozilla.org/logviewer.html#?job_id=39325912&repo=mozilla-
> inbound
> 
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e089bc358c5c

This is mighty weird, if you look at my try run from comment 3 there's no such crashes. Besides this patch hasn't touched any code, it just modified what we package in the Firefox installer/dmg image/tarball. Anyway, here's another try:

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

It's mostly green with a single intermittent that caused a reftest to fail (but not to crash) and which was automatically re-triggered and passed on the second run. I'll wait for all the orange tests to re-run and then land if it looks clean again.
Flags: needinfo?(gsvelto)
Try is green again, pushing to inbound (with fingers crossed this time).
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cddd249a333
Package the minidump-analyzer tool r=ted
https://hg.mozilla.org/mozilla-central/rev/3cddd249a333
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Do we plan on uplifting this to 52?
Flags: needinfo?(gsvelto)
This was meant to be part of bug 1280477 so yeah, it would be nice to have it in 52 (it's enabled on nightly and aurora only anyway).
Flags: needinfo?(gsvelto)
Comment on attachment 8811241 [details] [diff] [review]
minidump-analyzer-installer.patch

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1280477 which landed in 52 missed out on packaging the minidump-analyzer.

[User impact if declined]:

We fix fewer crashes.

[Is this code covered by automated tests?]:

Not sure.

[Has the fix been verified in Nightly?]:

Yes, stacks have been submitted via telemetry.

[Needs manual test from QE? If yes, steps to reproduce]: 

No.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

Low risk.

[Why is the change risky/not risky?]:

Fixes omission from bug 1280477 which landed in 52.

[String changes made/needed]:

None.
Attachment #8811241 - Flags: approval-mozilla-aurora?
(In reply to Gabriele Svelto [:gsvelto] from comment #13)
> This was meant to be part of bug 1280477 so yeah, it would be nice to have
> it in 52 (it's enabled on nightly and aurora only anyway).

Okay requested uplift, in bug 1322735 I hope to enable it on beta and release as well.
Comment on attachment 8811241 [details] [diff] [review]
minidump-analyzer-installer.patch

ship minidump-analyzer to get crash stacks in telemetry for aurora52
Attachment #8811241 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Carsten Book [:Tomcat] from comment #18)
> had to back this out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=4412341&repo=mozilla-
> aurora#L14731

Ouch, I just realized this might be due to the way I'm building it. The executable is being conditionally built but then unconditionally included in the package distribution. This needs a quick fix before it makes its way into beta too...
Flags: needinfo?(gsvelto)
Depends on: 1322983
It looks like this got backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/67635f34bbdc. Gabriele, do we still want to try to land this on aurora before the next uplift? It would be really nice to have on beta.
Flags: needinfo?(gsvelto)
Example failure [1]:

> 17:42:32     INFO -  Error: c:\builds\moz2_slave\m-aurora-w32-00000000000000000\build\src\browser\installer\package-manifest.in:780: Missing file(s): bin/minidump-analyzer.exe

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=65772876&repo=mozilla-aurora&lineNumber=19141
(In reply to Eric Rahm [:erahm] from comment #21)
> It looks like this got backed out in
> https://hg.mozilla.org/releases/mozilla-aurora/rev/67635f34bbdc. Gabriele,
> do we still want to try to land this on aurora before the next uplift? It
> would be really nice to have on beta.

Yes, I'd like to but we'll need to uplift this plus bug 1322983 otherwise the end result will be broken. I'll see if I can do it today.
Flags: needinfo?(gsvelto)
OK, I've requested approval for bug 1322983 so once I have that I'll uplift both together to prevent breakage.
You need to log in before you can comment on or make changes to this bug.