801809 - Show a crash reporter dialog the first time there's a crash

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect, P1)

Description

[Larissa Co [:lco]]

See pg. 4 of the spec (URL linked) for the screenshot.

The first time a crash occurs, display the System Crash Reporting prompt if the crash is a system-level crash:

1. The system crash prompt is displayed once the device reboots.
2. Hitting "Send" when the "Always send a report" pref is ON sends the report for the current crash, and continues to send reports from then on.
3. Hitting "Send" when the "Always send a report" pref is OFF sends the report for the current crash only.
4. Hitting "Don't Send" doesn't send the report for the current crash, and is equivalent to asking for permission for sending a report during subsequent crashes.

Comment 1

[:Margaret Leibovic]

Shortening the summary to make it easier to read.

Comment 2

[:Margaret Leibovic]

Attachment: WIP

This patch implements the logic flow from lco's mockups here:
http://people.mozilla.com/~lco/Crash_Reporting_B2G/R1_Crash%20Reports%20v1.pdf

I'm working on the gaia part on a branch here:
https://github.com/leibovic/gaia/tree/crashreporter

I'm not sure how to ask for feedback on progress on github... I guess it's annoying that I have multiple commits in that branch, but if you could take a quick look and let me know if I'm doing anything obviously wrong, that would be appreciated :)

Also, for right now, this will show the same dialog for app/system crashes, but I'll do the app-specific bits in bug 801810.

Comment 3

[:Margaret Leibovic]

Updating the summary to reflect the fact that we only ever want to show this dialog once (not once for a system crash and once for an app crash).

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 672884 [details] [diff] [review]
WIP

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

Looks great!

::: b2g/chrome/content/shell.js
@@ +103,5 @@
>  
> +    dump("\n\nshell.js reportCrash: " + crashID + "\n\n\n");
> +
> +    // Bail if there isn't a valid crashID.
> +    if (!crashID)

nit: {} even for one-liners

Comment 5

[:Margaret Leibovic]

Attachment: patch

I made a pull request for the gaia side of things:
https://github.com/mozilla-b2g/gaia/pull/5967

I decided that we should keep the "app.reportCrashes" pref automatically set to true until the crash reporter settings UI lands (bug 801932). Otherwise, if you decided not to send a crash report, you'd never be able to switch back to automatic crash reporting. This means that this crash reporter dialog won't actually show up for anyone yet, but I figured we should start landing pieces as they're ready.

I included bug numbers to follow-up work in some comments. There's definitely more to do, but keeping the scope of each bug small seems like a good idea :)

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 674425 [details] [diff] [review]
patch

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

I think we should just add a setting listener (in b2g/chrome/content/settings.js) here that controls the pref you are already using, and remove the |case "report-crashes-pref":| path.
This way, your code will be ready even if the gaia side is not done yet.

Comment 7

[:Margaret Leibovic]

I was looking into the settings code, and I wonder if it would be easier to implement the settings UI pieces with a tri-state integer pref rather than this tri-state boolean pref (null/true/false). It looks like the logic that controls boolean prefs expects them to be either true or false.

I'm poking around the settings app trying to find an example of a similar preference, but I'm having trouble finding one. Kaze, do you know if gaia is equipped to handle tri-state prefs? Or do you have a preference for how we do this?

Comment 8

[[:fabrice] Fabrice Desré]

I would even go with a string setting here with explicit names.

Comment 9

[:Margaret Leibovic]

Attachment: patch w/ setting listener

I went with a string setting here, but kept the boolean pref for the backend. I updated the PR [1] as well.

[1] https://github.com/mozilla-b2g/gaia/pull/5967

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 674802 [details] [diff] [review]
patch w/ setting listener

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

Very nice!

::: b2g/chrome/content/shell.js
@@ +125,5 @@
> +      }
> +    }
> +  },
> +
> +  // This function submits a crash when we're on wi-fi.

Nit: s/on wi-fi/online

@@ +129,5 @@
> +  // This function submits a crash when we're on wi-fi.
> +  submitCrash: function shell_submitCrash(aCrashID) {
> +    Services.obs.addObserver(function observer(subject, topic, state) {
> +      if (topic != "network:offline-status-changed") {
> +        return;

That can not happen, right (unles we have serious issues in the observer code)? feel free to remove that block.

Comment 11

[:Margaret Leibovic]

Thanks for the review!

Fabrice, are you an appropriate person to review the gaia changes, or should I flag someone else for that?

Comment 12

[Robert Kaiser]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> > +  // This function submits a crash when we're on wi-fi.
> 
> Nit: s/on wi-fi/online


The spec says we should only actually send reports when we're on wifi (which of course has its own problems in Brazil, but that's a different topic).

Comment 13

[Larissa Co [:lco]]

 > The spec says we should only actually send reports when we're on wifi (which
> of course has its own problems in Brazil, but that's a different topic).

The reason for doing so right now is to alleviate the user's concern that sending these reports will consume their data plan. Fabrice, what do you mean when you say, "that cannot happen"?

To clarify the idea, we can keep the report data on the device until the user is connected to a wi-fi network, then we send the report in the background.

In the future, we could enable the user to choose how to send these reports in settings, if we're concerned about the availability / accessibility of Wi-Fi networks in Brazil

Comment 14

[:Margaret Leibovic]

(In reply to Robert Kaiser (:[e-mail]) from comment #12)
> (In reply to Fabrice Desré [:fabrice] from comment #10)
> > > +  // This function submits a crash when we're on wi-fi.
> > 
> > Nit: s/on wi-fi/online
> 
> 
> The spec says we should only actually send reports when we're on wifi (which
> of course has its own problems in Brazil, but that's a different topic).

This means wi-fi, Fabrice was just being nit-picky about the comment :)

(In reply to Larissa Co from comment #13)
>  > The spec says we should only actually send reports when we're on wifi
> (which
> > of course has its own problems in Brazil, but that's a different topic).
> 
> The reason for doing so right now is to alleviate the user's concern that
> sending these reports will consume their data plan. Fabrice, what do you
> mean when you say, "that cannot happen"?

He meant that we wouldn't end up inside that function if |topic != "network:offline-status-changed"| so that was a useless check.

> To clarify the idea, we can keep the report data on the device until the
> user is connected to a wi-fi network, then we send the report in the
> background.

Yes, that's exactly what we're doing.

> In the future, we could enable the user to choose how to send these reports
> in settings, if we're concerned about the availability / accessibility of
> Wi-Fi networks in Brazil

Yeah, sounds like a good follow-up bug.

Comment 15

[:Margaret Leibovic]

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/5967

The patch in this PR shows the crash reporter dialog when gaia gets a 'crash-dialog' chrome event from gecko.

There's still follow-up work to do (I'm working on that now!), so I linked to bug numbers in comments.

Comment 16

[:Margaret Leibovic]

Comment on attachment 675166 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/5967

A lot of this ended up changing in my patch for bug 801925, so don't worry about reviewing this.

Comment 17

[:Margaret Leibovic]

This is a necessary component of the crash reporter UI, which is a P1 feature.

Comment 18

[Alex Keybl [:akeybl]]

P1 because this is a feature requirement, and we can't ship without a crash submission UI of some sort (user choice).

Comment 19

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e42b3b43e3

The Gaia part of this will be handled in my PR in bug 801925.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0e42b3b43e3

Comment 21

[:Margaret Leibovic]

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

Comment 22

[mdaniloff]

Verified Fixed on Unagi build #20130103070201

Comment 23

[pallavi]

Verified fixed on Unagi build id:20130114073222