Closed Bug 801809 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

(blocking-basecamp:+, firefox18 fixed)

VERIFIED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed

People

(Reporter: lco, Assigned: Margaret)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → margaret.leibovic
Shortening the summary to make it easier to read.
Summary: As a user, I want to be informed the first time my FirefoxOS phone has a system crash so that I'm aware that a crash has occurred and I can choose to send a report about it or not. → Show a crash reporter dialog the first time we reboot after a system crash
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #672884 - Flags: feedback?(fabrice)
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).
Summary: Show a crash reporter dialog the first time we reboot after a system crash → Show a crash reporter dialog the first time there's a crash
Blocks: 801810
Blocks: 801925
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
Attachment #672884 - Flags: feedback?(fabrice) → feedback+
Attached patch patch (obsolete) — Splinter Review
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 :)
Attachment #672884 - Attachment is obsolete: true
Attachment #674425 - Flags: review?(fabrice)
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.
Attachment #674425 - Flags: review?(fabrice) → review-
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?
I would even go with a string setting here with explicit names.
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
Attachment #674425 - Attachment is obsolete: true
Attachment #674802 - Flags: review?(fabrice)
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.
Attachment #674802 - Flags: review?(fabrice) → review+
Thanks for the review!

Fabrice, are you an appropriate person to review the gaia changes, or should I flag someone else for that?
(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).
 > 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
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
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.
Attachment #675166 - Flags: review?(21)
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.
Attachment #675166 - Attachment is obsolete: true
Attachment #675166 - Flags: review?(21)
This is a necessary component of the crash reporter UI, which is a P1 feature.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Priority: -- → P1
P1 because this is a feature requirement, and we can't ship without a crash submission UI of some sort (user choice).
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e42b3b43e3

The Gaia part of this will be handled in my PR in bug 801925.
https://hg.mozilla.org/mozilla-central/rev/c0e42b3b43e3
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Verified Fixed on Unagi build #20130103070201
Verified fixed on Unagi build id:20130114073222
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.