765285 - Include last 3 days of crash IDs in about:support

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Robert Kaiser]

In a lot of cases where people have been crashing, it would be extremely helpful to have some references to the actual crashes in the troubleshooting information people post to support.

We should include a "Recent Crashes" section into the about:support page to help there, listing all crash IDs from the recent 3 days.

Comment 1

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

Where in about:support should this be listed?
Should the crashes be in ascending or descending order?
What kind of information should be shown? Is there a Socorro API to look this up? I haven't read http://socorro.readthedocs.org/en/latest/index.html , its old Google Code mentions Socorro UI, so this is not for general consumption?
Much of toolkit/crashreporter/content/crashes.js will be needed. Basically, it will be the creation of the list with a check for ignoring reports older than 3 days. Should code duplication be avoided (loading it in about:support with a script tag and modify the populate function to accept a parameter for the time limit which will be missing for about:crashes, so the function would have a default value using http://blog.mozilla.org/jorendorff/2012/05/29/rest-arguments-and-default-arguments-in-javascript/
Should the limit of 3 days be configurable by a hidden preference in /modules/libpref/src/init/all.js?

Comment 2

[Robert Kaiser]

(In reply to Archaeopteryx [:aryx] from comment #1)
> Where in about:support should this be listed?

Wherever the about:support maintainers deem it fitting.

> Should the crashes be in ascending or descending order?

I'd do newest-first, ordered by date.

> What kind of information should be shown? Is there a Socorro API to look
> this up?

The linked crash ID and the date, just as in about:crashes - no Socorro API needed, the info is already on the user's computer and code to read it is in use by about:crashes already.

And yes, we should try to reuse about:crashes code as much as possible, IMHO.

We could also link the full about:crashes page as a "Full list of my crashes" link at the end of the list, I guess.

> Should the limit of 3 days be configurable by a hidden preference in
> /modules/libpref/src/init/all.js?

Could be, but doesn't have to. Let's not overdesign this, as about:crashes has the full list anyhow.

Comment 3

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

Attachment: crash reports in about:support, patch, v1

I wrote the changes in the patch into the files of an already compiled build (compiling would take ages on the old machine here, please excuse this). Because this is the first time I use hg's mq extension, please be aware that I could have done something wrong. Thank you.

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 661851 [details] [diff] [review]
crash reports in about:support, patch, v1

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

Seems fine but mostly I'm concerned about putting too much implementation detail in a shared jsm.

::: toolkit/crashreporter/CrashReports.jsm
@@ +9,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +
> +var reportsDir, pendingDir;
> +var reportURL;

These should be declared in the function that creates and uses them rather than globally.

@@ +45,5 @@
> +    event.initEvent("CrashSubmitFailed", true, false);
> +    document.dispatchEvent(event);
> +  },
> +
> +  submitPendingReport: function(event) {

As we're making a shared module here it shouldn't depend on the specifics of how about:crashes works currently and instead the source pages should just use CrashSubmit directly. I know it involves a little bit of code sharing but that's better than having a shared API that only works in these particular cases.

@@ +57,5 @@
> +    event.preventDefault();
> +    return false;
> +  },
> +
> +  findInsertionPoint: function(reports, date) {

This doesn't need to be exposed, just make it a private function instead

@@ +132,5 @@
> +          reports.splice(pos, 0, entry);
> +        }
> +      }
> +    }
> +    return {"reports": reports, "noConfig": false, "reportURL": reportURL};

I'm tempted to say that we should cache this result so multiple visits to about:support and about:crashes are faster, not sure whether it is worth it though.

Comment 5

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

Attachment: crash reports in about:support, patch, v2

The code has been restructured according to your comments, but I didn't do the caching of the crash reports list because newer crash reports can have occurred since the last visit (e.g. child processes like plugin container) and we have to read all reports again (the possibility to read newest -> oldest would be cool).

Comment 6

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

Comment on attachment 662115 [details] [diff] [review]
crash reports in about:support, patch, v2

Removing r?, got bitrotted by 789674.

Comment 7

[Gervase Markham [:gerv]]

Archaeopteryx: you need to ask someone to review your patch :-) This is an awesome idea, and really needed. Let's push it home.

Gerv

Comment 8

[Robert Kaiser]

Archeopteryx: Any progress here?

Comment 9

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

Attachment: crash reports in about:support, patch, v3

Comment 10

[Robert Kaiser]

Comment on attachment 795131 [details] [diff] [review]
crash reports in about:support, patch, v3

I think it does what it should (display the submitted reports of the last 3 days) and the strings and placements look OK but I can't really give feedback on the actual code, we'll need actual review there from someone familiar with this code.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 795131 [details] [diff] [review]
crash reports in about:support, patch, v3

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

In the future please don't include so many unrelated changes, it makes reviewing the patch harder, makes it more likely you may introduce a regression in irrelevant code and complicates figuring out why things changed later on.

r- mainly for the breakage and l10n question but also to ask if there is a way we could test this?

::: toolkit/content/aboutSupport.js
@@ +77,5 @@
> +                                         Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
> +                                         date.getHours(),
> +                                         date.getMinutes(),
> +                                         date.getSeconds());
> +      let formattedDate = datestr + " " + timestr;

This doesn't look to be very well localised. Why not use formatter.FormatDateTime? Is this something we do elsewhere?

::: toolkit/crashreporter/CrashReports.jsm
@@ +63,5 @@
> +      }
> +    }
> +
> +    // Sort reports descending by date
> +    return reports.sort( (a, b) => b.date - a.date);

How slow is this for a few hundred reports?

::: toolkit/crashreporter/content/crashes.js
@@ +139,4 @@
>        file.remove(false);
>      }
>    }
>    entries = reportsDir.directoryEntries;

I'd guess clearReports is broken since you've removed the code that defined reportsDir.

Comment 12

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

Attachment: crash reports in about:support, patch, v4

(In reply to Dave Townsend (:Mossop) from comment #11)
> Comment on attachment 795131 [details] [diff] [review]
> crash reports in about:support, patch, v3
> 
> Review of attachment 795131 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In the future please don't include so many unrelated changes, it makes
> reviewing the patch harder, makes it more likely you may introduce a
> regression in irrelevant code and complicates figuring out why things
> changed later on.
I have remove the Services.*, string method and var -> let conversions from this patch.

> r- mainly for the breakage and l10n question but also to ask if there is a
> way we could test this?
I don't know the testing environment well enough, but basically the ability to copy crash reports from a backup and to modify the lastModifiedTime should be sufficient. Is about:crashes covered by any tests?

> ::: toolkit/content/aboutSupport.js
> @@ +77,5 @@
> > +                                         Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
> > +                                         date.getHours(),
> > +                                         date.getMinutes(),
> > +                                         date.getSeconds());
> > +      let formattedDate = datestr + " " + timestr;
> 
> This doesn't look to be very well localised. Why not use
> formatter.FormatDateTime? Is this something we do elsewhere?
This is similar to about:crashes where two separate table cells are used for date and time. Should this be done here? If yes, it will need extract styling because about:crashes will show them as two separate table cells.
Because the information shall be shared across people from different locales in end user support, formatting it in a locale should be avoided in my opinion.

> ::: toolkit/crashreporter/CrashReports.jsm
> @@ +63,5 @@
> > +      }
> > +    }
> > +
> > +    // Sort reports descending by date
> > +    return reports.sort( (a, b) => b.date - a.date);
> 
> How slow is this for a few hundred reports?
With 296 crash reports, CrashReports.getReports takes between 100-140ms with a mean ~110ms on 6 year old notebook HDD.

> ::: toolkit/crashreporter/content/crashes.js
> @@ +139,4 @@
> >        file.remove(false);
> >      }
> >    }
> >    entries = reportsDir.directoryEntries;
> 
> I'd guess clearReports is broken since you've removed the code that defined
> reportsDir.
Thank you for the catch, CrashReports.jsm now provides pendingDir, reportsDir and submittedDir.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 811698 [details] [diff] [review]
crash reports in about:support, patch, v4

(In reply to Archaeopteryx [:aryx] from comment #12)
> Created attachment 811698 [details] [diff] [review]
> crash reports in about:support, patch, v4
> 
> (In reply to Dave Townsend (:Mossop) from comment #11)
> > ::: toolkit/content/aboutSupport.js
> > @@ +77,5 @@
> > > +                                         Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
> > > +                                         date.getHours(),
> > > +                                         date.getMinutes(),
> > > +                                         date.getSeconds());
> > > +      let formattedDate = datestr + " " + timestr;
> > 
> > This doesn't look to be very well localised. Why not use
> > formatter.FormatDateTime? Is this something we do elsewhere?
> This is similar to about:crashes where two separate table cells are used for
> date and time. Should this be done here? If yes, it will need extract
> styling because about:crashes will show them as two separate table cells.
> Because the information shall be shared across people from different locales
> in end user support, formatting it in a locale should be avoided in my
> opinion.

Interesting point, I'd like to get some l10n feedback on this

Comment 14

[Dave Townsend [:mossop]]

(In reply to Archaeopteryx [:aryx] from comment #12)
> > r- mainly for the breakage and l10n question but also to ask if there is a
> > way we could test this?
> I don't know the testing environment well enough, but basically the ability
> to copy crash reports from a backup and to modify the lastModifiedTime
> should be sufficient. Is about:crashes covered by any tests?

Looks like there are a couple of tests here: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/browser/

Comment 15

[Axel Hecht [:Pike]]

Comment on attachment 811698 [details] [diff] [review]
crash reports in about:support, patch, v4

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

I don't think that about:crashes is doing a good job in date formatting. I'd go for some N minutes, or hours, or days ago instead. Not sure how the plaintext copy should deal with this.

Do we filter by application that crashed? I know that my about:crashes lists all the crashes I had across profiles, I'm not sure about:support should.

Is there value in talking about pending crash reports in about:support? Is there a wording that makes that more human digestible, i.e. makes it clear if that's something actionable for the user or not?

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd
@@ +20,5 @@
> +<!ENTITY aboutSupport.crashes.sendDate "Date Submitted">
> +<!ENTITY aboutSupport.crashes.allReports "All Crash Reports">
> +<!-- LOCALIZATION NOTE (aboutSupport.crashes.noConfig):
> +This is likely the same like noConfig.label in crashes.dtd. -->
> +<!ENTITY aboutSupport.crashes.noConfig "This application has not been configured to display crash reports. The preference <code>breakpad.reportURL</code> must be set.">

The breakpad.reportURL comment seems to be useless advice, even if it's copy and paste.

::: toolkit/locales/en-US/chrome/global/aboutSupport.properties
@@ +4,5 @@
>  
> +# LOCALIZATION NOTE (downloadsTitleFiles): Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# #1 number of days relevant with relevant crash reports
> +crashesTitle=Crash Reports From The Last #1 Day;Crash Reports From The Last #1 Days

I don't think the casing is right here. Also, "Crash Reports for the Last 1 Day" needs English copy.

Comment 16

[Dave Townsend [:mossop]]

(In reply to Axel Hecht [:Pike] from comment #15)

> Do we filter by application that crashed? I know that my about:crashes lists
> all the crashes I had across profiles, I'm not sure about:support should.

Unfortunately all the crash reports for any profile (of the same application) are stored in the same place because the crash reporter can't really decipher which profile you were using easily so that's expected right now and not something we can easily change.

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 811698 [details] [diff] [review]
crash reports in about:support, patch, v4

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

r+ on the current code but address the l10n concerns and spin it past me one last time before landing.

Comment 18

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

Attachment: crash reports in about:support, patch, v5

(In reply to Axel Hecht [:Pike] from comment #15)
> Comment on attachment 811698 [details] [diff] [review]
> crash reports in about:support, patch, v4
> 
> Review of attachment 811698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there value in talking about pending crash reports in about:support? Is
> there a wording that makes that more human digestible, i.e. makes it clear
> if that's something actionable for the user or not?
KaiRo said pending crashes shouldn't be listed on about:support. I show the number of pending ones and a link to about:crashes so it's still possible to see that the application crashed and either user didn't submit the crash or it wasn't submitted because it was a recurring one.

Comment 19

[Robert Kaiser]

(In reply to Archaeopteryx [:aryx] from comment #18)
> KaiRo said pending crashes shouldn't be listed on about:support.

Yes, unsubmitted/pending crashes don't help us when people copy this output to a support page, only submitted ones are helpful there. Also, the crash IDs change when submitting them so if we ask people to go and manually submit them, they still need to give us the new IDs anyhow. Listing the number of pending crashes should be enough to tell us there are some.
Also, the UI on manual crash submissing in about:crashes is lacking anyhow (for one thing, not easy to discover that they are unsubmitted and clicking does submit them, for the other, the page we land on after clicking is usually unhelpful) and I don't want to expose people to that lacking/confusing UI in more places.
Also, I don't think we need to point out too much that users can do something about those, both because of the confusing UI and because if we have any of those, that's IMHO either a user choice (i.e. they decided not to send) or a bug (we should care that we send all we are allowed to), and in the former I don't want to rub it in their face while in the latter, we should care to file, identify and fix that.

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 819440 [details] [diff] [review]
crash reports in about:support, patch, v5

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

One indentation tweak here but otherwise good to go.

::: toolkit/content/aboutSupport.js
@@ +73,5 @@
> +      {
> +        let daysPassed = Math.round(timePassed / (24 * 60 * 60 * 1000));
> +        formattedDate = PluralForm.get(daysPassed,
> +                                       strings.GetStringFromName("crashesTimeDays"))
> +                                              .replace("#1", daysPassed);

This indentation confused me leaving me thinking that it was replacing the result of strings.GetStringFromName rather than the result of PluralForm.get.

Cramming this many operations into a single execution statement might be a mistake. How about something like this:

let string = strings.GetStringFromName("crashesTimeDays");
formattedDate = PluralForm.get(daysPassed, string)
                          .replace("#1", daysPassed);

Ditto for the other cases.

(always thought we should make the get/replace pair one step in PluralForm but there we go)

Comment 21

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

Attachment: crash reports in about:support, patch, v6, r=Mossop

Comment 22

[Ed Morley [:emorley]]

has this had a try push? If so, could you paste the URL? :-)

Comment 23

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

Attachment: crash reports in about:support: fix test, patch, v1

This patch fixes a browser chrome test. Successful Try run: https://tbpl.mozilla.org/?tree=Try&showall=1&rev=b6b2afa3fd78

General Try run with the already r+ patch: https://tbpl.mozilla.org/?tree=Try&showall=1&rev=06f773c2230b

Comment 24

[Justin Dolske [:Dolske]]

Comment on attachment 822791 [details] [diff] [review]
crash reports in about:support: fix test, patch, v1

r+ per IRC.

Comment 25

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

Attachment: crash reports in about:support, patch, v7, r=Mossop,dolske

Thank you for the review, dolske.

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7027f0e1334c, but then that was just the final test fixup without the actual meat of the patch, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c63dfd127858, whole thing landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/f78e6b91ad8d

Comment 27

[Phil Ringnalda (:philor)]

And out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/0add31e02ee5 - the ASan failure in https://tbpl.mozilla.org/php/getParsedLog.php?id=29724475&tree=Mozilla-Inbound says you've got something else that needs to be ifdef crashreporter, since it builds --disable-crashreporter.

Comment 28

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

Attachment: crash reports in about:support, patch, v8, r=Mossop for about:support, r=dolske for previous test change

This change drops the "required" for the crashes property in the test so that it works in builds with and without crash reporter. Pushed to Try a few minutes ago: https://tbpl.mozilla.org/?tree=Try&showall=1&rev=e503311bbe3a

Comment 29

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

Okay with the latest change by edmorely over IRC.

Comment 30

[Ed Morley [:emorley]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/00cba0743231

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/00cba0743231

Comment 32

[Francesco Lodolo [:flod]]

Comment on attachment 823082 [details] [diff] [review]
crash reports in about:support, patch, v8, r=Mossop for about:support, r=dolske for previous test change

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

Not really important for the patch in itself, nitpicking.

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd
@@ +14,5 @@
> +<!ENTITY aboutSupport.crashes.title "Crash Reports">
> +<!-- LOCALIZATION NOTE (aboutSupport.crashes.id):
> +This is likely the same like id.heading in crashes.dtd. -->
> +<!ENTITY aboutSupport.crashes.id "Report ID">
> +<!-- LOCALIZATION NOTE (aboutSupport.crashes.id):

Double comment for aboutSupport.crashes.id

::: toolkit/locales/en-US/chrome/global/aboutSupport.properties
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# LOCALIZATION NOTE (downloadsTitleFiles): Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# #1 number of days relevant with relevant crash reports

I think the first relevant shouldn't be there.

Comment 33

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

Attachment: fix localization comments, patch, v1

This patch addresses flod's comments. Change to the change about "relevant" over IRC.

Comment 34

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

Attachment: fix localization comments, patch, v2

One more change suggested by flod.

Comment 35

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

Attachment: fix localization comments, patch, v3, r=Mossop

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f259397bfb9

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/4f259397bfb9