835785 - Don't show additional dump names in about:crashes

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Attachment: Don't show additional dumps/reports

Since we changed how additional dumps are submitted, more then one dump per crash is shown per plugin hang in about:crashes.

E.g.:
  7af96978-6b96-4966-bdd8-8ef4f0f4441e
  7af96978-6b96-4966-bdd8-8ef4f0f4441e-browser
  7af96978-6b96-4966-bdd8-8ef4f0f4441e-flash1
  7af96978-6b96-4966-bdd8-8ef4f0f4441e-flash2

We should only show the "main" dump to avoid confusion.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Sorry for all those CCs - i failed cloning another bug properly.

Comment 2

[(not currently active) Ted Mielczarek]

So uh, related note, if you click-to-submit the main dump from about:crashes, does it successfully submit all the other dumps at the same time?

Comment 3

[Georg Fritzsche [:gfritzsche]]

Yes, it did so independently of which of the entries you clicked.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 707585 [details] [diff] [review]
Don't show additional dumps/reports

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

::: toolkit/crashreporter/content/crashes.js
@@ +131,5 @@
>        var file = entries.getNext().QueryInterface(Ci.nsIFile);
>        var leaf = file.leafName;
> +      var id = leaf.slice(0, -4);
> +      if (leaf.substr(-4) == ".dmp" &&
> +          ['browser', 'flash1', 'flash2'].indexOf(id.split('-').pop()) == -1) {

Hardcoding the dump types here kind of sucks. Can we instead just strip test if the length of the filename (minus extension) is greater than that of a UUID (with dashes included), and skip them in that case?

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Don't show additional dumps/reports

Right, sorry, i was rushing this one between other things :(
Alternative suggestion using a regex test.

Comment 6

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36b5ab89ae64

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/01361f5fda96

https://tbpl.mozilla.org/php/getParsedLog.php?id=19412987&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js | about:crashes lists correct number of crash reports - Got 1, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js | uncaught exception - TypeError: el is null at chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1201
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js | Found a tab after previous test timed out: about:crashes
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/crashreporter/test/browser_aboutCrashesResubmit.js | about:crashes lists correct number of crash reports - Got 1, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/crashreporter/test/browser_aboutCrashesResubmit.js | uncaught exception - TypeError: el is null at chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1201
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/crashreporter/test/browser_aboutCrashesResubmit.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/crashreporter/test/browser_aboutCrashesResubmit.js | Found a tab after previous test timed out: about:crashes

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Don't show additional dumps/reports, v3

Turns out there was a bug in the corresponding head.js (substring() returns the range [indexA,indexB)).
https://tbpl.mozilla.org/?tree=Try&rev=60de3873ca30

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 709730 [details] [diff] [review]
Don't show additional dumps/reports, v3

I'm not sure if that small change in head.js requires another review, so defaulting to requesting one.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 709730 [details] [diff] [review]
Don't show additional dumps/reports, v3

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

I would have been fine with you just fixing that, but r=me.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Thanks, noted for next time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb63dccfe41

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8eb63dccfe41