Add the minidump-analyzer tool to Firefox' distribution

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gsvelto, Assigned: gsvelto)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
+++ 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.
(Assignee)

Updated

2 years ago
Summary: Store stack traces into the crash.main event file for each browser crash → Add the minidump-analyzer tool to Firefox' distribution
(Assignee)

Comment 1

2 years ago
Created attachment 8811241 [details] [diff] [review]
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.
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+
(Assignee)

Comment 3

2 years ago
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

2 years ago
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75fc07efe46f
Package the minidump-analyzer tool r=ted
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
Flags: needinfo?(gsvelto)
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...

Updated

2 years ago
Blocks: 1318758
(Assignee)

Comment 8

2 years ago
(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)
(Assignee)

Comment 9

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

Comment 10

2 years ago
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cddd249a333
Package the minidump-analyzer tool r=ted

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3cddd249a333
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 12

2 years ago
Do we plan on uplifting this to 52?
Flags: needinfo?(gsvelto)
(Assignee)

Comment 13

2 years ago
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 14

2 years ago
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?

Comment 15

2 years ago
(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+

Comment 17

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f597f21b42f5
status-firefox52: --- → fixed
had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4412341&repo=mozilla-aurora#L14731
status-firefox52: fixed → affected
Flags: needinfo?(gsvelto)
(Assignee)

Comment 19

2 years ago
(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)
(Assignee)

Updated

2 years ago
Depends on: 1322983
status-firefox52: fixed → affected

Comment 21

2 years ago
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)

Comment 22

2 years ago
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
(Assignee)

Comment 23

2 years ago
(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)
(Assignee)

Comment 24

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

Comment 25

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/bb61bdb559de
status-firefox52: affected → fixed
You need to log in before you can comment on or make changes to this bug.