1317968 - Add the minidump-analyzer tool to Firefox' distribution

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Gabriele Svelto [:gsvelto]]

+++ 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.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: minidump-analyzer-installer.patch

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.

Comment 2

[(not currently active) Ted Mielczarek]

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. :)

Comment 3

[Gabriele Svelto [:gsvelto]]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75fc07efe46f
Package the minidump-analyzer tool r=ted

Comment 5

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

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

Comment 6

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

Hmm, these all appear to have happened on the push AFTER yours, and only on that push, the following pushes are green. Weird.

Comment 7

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

(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...

Comment 8

[Gabriele Svelto [:gsvelto]]

(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.

Comment 9

[Gabriele Svelto [:gsvelto]]

Try is green again, pushing to inbound (with fingers crossed this time).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cddd249a333
Package the minidump-analyzer tool r=ted

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3cddd249a333

Comment 12

[Eric Rahm [:erahm]]

Do we plan on uplifting this to 52?

Comment 13

[Gabriele Svelto [: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).

Comment 14

[Eric Rahm [:erahm]]

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.

Comment 15

[Eric Rahm [:erahm]]

(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 16

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8811241 [details] [diff] [review]
minidump-analyzer-installer.patch

ship minidump-analyzer to get crash stacks in telemetry for aurora52

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f597f21b42f5

Comment 18

[Carsten Book [:Tomcat]]

had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4412341&repo=mozilla-aurora#L14731

Comment 19

[Gabriele Svelto [:gsvelto]]

(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...

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/abc870a3a02d2cf388d3c35c7bb9094bf606be7c

Comment 21

[Eric Rahm [:erahm]]

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.

Comment 22

[Eric Rahm [:erahm]]

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

Comment 23

[Gabriele Svelto [:gsvelto]]

(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.

Comment 24

[Gabriele Svelto [:gsvelto]]

OK, I've requested approval for bug 1322983 so once I have that I'll uplift both together to prevent breakage.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bb61bdb559de