1024677 - Expose crash data via the SelfSupport API

Status: RESOLVED INCOMPLETE

(Firefox Health Report Graveyard :: Client: Desktop, defect, P4)

Description

[Benjamin Smedberg]

The FHR payload knows about crash counts, but not crash reasons. We want to be able to expose to the support API/about:healthreport more detailed information about recent crashes:

* A list of recent crashes (180 days) including whether each one was submitted to the server
* For submitted crashes, expose the support classification
* For non-submitted crashes, an API to submit the crash

Comment 1

[Biru [:poiru]]

I was thinking of extending MozSelfSupport with `Promise<object> getCrashes()` (feel free to suggest a better name). The promise would be resolved with something like this:

[
  {id: '...', type: '...', date: '...', classification: '...'},
  ...
]

Does that look reasonable?

Comment 2

[Gregory Szorc [:gps]]

I think the proposal looks reasonable. I was about to say it should be an array of objects so some kind of intelligent sorting can be applied before returning. But you've read my mind.

Comment 3

[Biru [:poiru]]

Attachment: Part 1: Add MozSelfSupport.getCrashes

Comment 4

[Biru [:poiru]]

(In reply to Gregory Szorc [:gps] from comment #2)
> I think the proposal looks reasonable. I was about to say it should be an
> array of objects so some kind of intelligent sorting can be applied before
> returning. But you've read my mind.

The patch above does not do any sorting. Perhaps we should sort by crash date?

Comment 5

[Biru [:poiru]]

Attachment: Part 1: Add MozSelfSupport.getCrashes

I went ahead and updated the patch to sort by date.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8478768 [details] [diff] [review]
Part 1: Add MozSelfSupport.getCrashes

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

r+ conditional on fixing getCrash(). I'm surprised this code isn't raising. Is "id" coming from an outer scope?

::: browser/components/selfsupport/test/test_MozSelfSupport.html
@@ +15,5 @@
> +Cu.import("resource://gre/modules/Task.jsm", this);
> +
> +let clearCrashManagerCrashes = Task.async(function* () {
> +  let store = yield Services.crashmanager._getStore();
> +  store._data.crashes.clear();

You may want to put a |yield store.save()| (I think that's the API) in here to avoid race conditions with other tests. Unlikely, but this is how intermittent oranges begin their life.

@@ +79,5 @@
> +    for (let t of TESTS) {
> +      yield t();
> +    }
> +
> +    SimpleTest.finish();

Ugh. Do we not have APIs in chrome tests for easily writing task-basted tests? It is quite silly that every test needs to reinvent this boilerplate.

::: toolkit/components/crashes/CrashManager.jsm
@@ +605,5 @@
>  
> +  getCrash: Task.async(function* () {
> +    let store = yield this._getStore();
> +    return store.getCrash(id);
> +  }),

Where does "id" come from?

Comment 7

[Biru [:poiru]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #0)
> * For non-submitted crashes, an API to submit the crash

bsmedberg, do we want to communicate the result of the submission with this API?

If so, we probably want to return a promise that is:
- resolved with the submission result (one of the SUBMISSION_RESULT constants or alternatively just a bool) if the submission is attempted
- rejected if the submission is not attempted at all (e.g. if the ID is invalid)

However, what about the case where the crash has already been submitted -- should we reject the promise or lie about a successful resolution?

(In reply to Gregory Szorc [:gps] from comment #6)
> ::: toolkit/components/crashes/CrashManager.jsm
> @@ +605,5 @@
> >  
> > +  getCrash: Task.async(function* () {
> > +    let store = yield this._getStore();
> > +    return store.getCrash(id);
> > +  }),
> 
> Where does "id" come from?

That was a mistake, will fix.

Comment 8

[Benjamin Smedberg]

1) the ultimate purpose of this API is to be able to fold about:crashes into about:healthreport so that we can prompt users to submit un-submitted recent crashes. So yes, in order to drive the UI we should have some sort of promise that is resolved with the server-side crash ID.

2) We should never re-submit crashes. I don't have a strong preference for whether we should throw some sort of already-submitted error in that case, or just resolve the promise with the crash report ID that we already have.

Comment 9

[Biru [:poiru]]

Attachment: Part 2: Add CrashManager.getCrash

Comment 10

[Biru [:poiru]]

Attachment: Part 3: Add MozSelfSupport.submitCrashReport

Comment 11

[Biru [:poiru]]

We probably want to have some kind of 'handled' property in the crash metadata plus e.g. MozSelfSupport.markCrashHandled(id) in order for about:healthreport to avoid resuggesting the same action over and over again for a particular crash. Any thoughts?

Comment 12

[Biru [:poiru]]

Attachment: Part 3: Add MozSelfSupport.submitCrashReport

Comment 13

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8478768 [details] [diff] [review]
Part 1: Add MozSelfSupport.getCrashes

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

Late to the party, but I couldn't resist a bit of JS Promise dev evangelism.

::: browser/components/selfsupport/SelfSupportService.js
@@ +71,5 @@
> +  getCrashes() {
> +    return new this._window.Promise((resolve, reject) => {
> +      Services.crashmanager.getCrashes().then((crashes) => {
> +        let results = [];
> +        for (let crash of crashes) {

The Promises spec requires that when you resolve a promise with another promise, the outer promise then resolves/rejects with the result of the inner. You can write this sort of code (where we want to take a library/chrome Promise and wrap it in a Window promise for content) like:

return this._window.Promise.resolve(
          innerPromise.then(action).then(what, ever));

Also, the loop over 'crashes' to produce the structure you want to return to content could be written as an array comprehension:

results = [for (let crash of crashes) { id: crash.id, ... }];

::: browser/components/selfsupport/test/test_MozSelfSupport.html
@@ +15,5 @@
> +Cu.import("resource://gre/modules/Task.jsm", this);
> +
> +let clearCrashManagerCrashes = Task.async(function* () {
> +  let store = yield Services.crashmanager._getStore();
> +  store._data.crashes.clear();

If the save is supposed to be safely async, I'd prefer to see explicit tests that the outward-facing APIs work correctly even if there is a pending or in-progress save on the store.

@@ +79,5 @@
> +    for (let t of TESTS) {
> +      yield t();
> +    }
> +
> +    SimpleTest.finish();

I haven't figured out how to get that working in content mochitests. In browser-chrome mochitests, all this harness is unnecessary; you can use a test file that just contains one or more

add_task(function* test_whatever() {...} );

and optionally a

registerCleanupFunction(function cleanup_func() {...} );

The cleanup function can return a promise if your cleanup stage is asynchronous. Unfortunately MDN is out of date on these test harness improvements.

Comment 14

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8500455 [details] [diff] [review]
Part 2: Add CrashManager.getCrash

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

::: toolkit/components/crashes/CrashManager.jsm
@@ +655,5 @@
>  
> +  getCrash: Task.async(function* (id) {
> +    let store = yield this._getStore();
> +    return store.getCrash(id);
> +  }),

It's overkill to use Task.async here; it adds tons of overhead (among other things, I'm pretty sure we can't JIT compile generator functions yet). The pure Promise implementation is straightforward:

getCrash(id) {
  return this._getStore().then(store => store.getCrash(id));
},

Comment 15

[Biru [:poiru]]

Attachment: Part 1: Add MozSelfSupport.getCrashes

(In reply to :Irving Reid from comment #13)
> The Promises spec requires that when you resolve a promise with another
> promise, the outer promise then resolves/rejects with the result of the
> inner. You can write this sort of code (where we want to take a
> library/chrome Promise and wrap it in a Window promise for content) like:
> 
> return this._window.Promise.resolve(
>           innerPromise.then(action).then(what, ever));
> 
> Also, the loop over 'crashes' to produce the structure you want to return to
> content could be written as an array comprehension:
> 
> results = [for (let crash of crashes) { id: crash.id, ... }];

Done. Thanks for the tips!

> ::: browser/components/selfsupport/test/test_MozSelfSupport.html
> @@ +15,5 @@
> > +Cu.import("resource://gre/modules/Task.jsm", this);
> > +
> > +let clearCrashManagerCrashes = Task.async(function* () {
> > +  let store = yield Services.crashmanager._getStore();
> > +  store._data.crashes.clear();
> 
> If the save is supposed to be safely async, I'd prefer to see explicit tests
> that the outward-facing APIs work correctly even if there is a pending or
> in-progress save on the store.

I'll address this with a separate test in a later patch.

> @@ +79,5 @@
> > +    for (let t of TESTS) {
> > +      yield t();
> > +    }
> > +
> > +    SimpleTest.finish();
> 
> I haven't figured out how to get that working in content mochitests. In
> browser-chrome mochitests, all this harness is unnecessary; you can use a
> test file that just contains one or more
> 
> add_task(function* test_whatever() {...} );

Unfortunately this is a mochitest-chrome test (which is distinct from mochitest-browser-chrome tests). AFAIK mochitest-chrome does not have these niceties.

Comment 16

[Benjamin Smedberg]

Comment on attachment 8500455 [details] [diff] [review]
Part 2: Add CrashManager.getCrash

I'm going to bump all of these to gfritzsche, since I'd like him to be the long-term peer of this code. Georg, please ping me if you need clarification on the overall goal of this.

Comment 17

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8500475 [details] [diff] [review]
Part 3: Add MozSelfSupport.submitCrashReport

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

A few comments as I'm looking over these changes to learn how to fit my own work into Self Support...

::: browser/components/selfsupport/SelfSupportService.js
@@ +94,5 @@
> +    return new this._window.Promise((resolve, reject) => {
> +      Services.crashmanager.getCrash(id).then((crash) => {
> +        if (!crash ||
> +            this._hasSuccessfulSubmission(crash.submissions)) {
> +          reject();

Provide a useful value to the reject() call here and below, so the caller has the option of reacting appropriately to what went wrong.

It looks like the W3C APIs are going the route of rejecting with a 'new DOMException()' object (http://www.w3.org/TR/dom/#domexception, though this is a draft spec) but based on some testing and DXR searching, we can't make DOMException objects in JS code.

Benjamin, would you rather try to enable use of DOMException for code like this and re-use their defined error codes, or create a new Error object for MozSelfSupport and define our own set of error names?

@@ +98,5 @@
> +          reject();
> +          return;
> +        }
> +
> +        CrashSubmit.submit(id, {

This would be a easier if CrashSubmit.submit returned a Promise instead of taking a callback object...

Comment 18

[Benjamin Smedberg]

I think a custom Error would make more sense in this case: whatever happens here is unlikely to actually be a "DOM" exception anyway.

Comment 19

[Benjamin Smedberg]

CrashSubmit.submit predates promises, but I don't see why we couldn't change it.

Comment 20

[Georg Fritzsche [:gfritzsche]]

(In reply to :Irving Reid from comment #17)
> @@ +98,5 @@
> > +          reject();
> > +          return;
> > +        }
> > +
> > +        CrashSubmit.submit(id, {
> 
> This would be a easier if CrashSubmit.submit returned a Promise instead of
> taking a callback object...

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #19)
> CrashSubmit.submit predates promises, but I don't see why we couldn't change
> it.

Indeed - doing a quick check this looks manageable, can you look into this in a blocking bug Birunthan?

Comment 21

[Georg Fritzsche [:gfritzsche]]

Also, sorry for the delays in reviewing this. I started looking into it today and will have to continue tomorrow or friday.

Comment 22

[Biru [:poiru]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> Indeed - doing a quick check this looks manageable, can you look into this
> in a blocking bug Birunthan?

Yep, filed bug 1088125.

Comment 23

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8504180 [details] [diff] [review]
Part 1: Add MozSelfSupport.getCrashes

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

::: browser/components/selfsupport/SelfSupportService.js
@@ +66,5 @@
>    },
> +
> +  getCrashes() {
> +    return this._window.Promise.resolve(
> +      Services.crashmanager.getCrashes().then((crashes) => {

What about handling rejections from getCrashes()? I see a few possible failure scenarios in CrashManager._getStore() which we should not directly leak out of an API.

@@ +67,5 @@
> +
> +  getCrashes() {
> +    return this._window.Promise.resolve(
> +      Services.crashmanager.getCrashes().then((crashes) => {
> +        let results = [for (crash of crashes) {

What about the requirement from comment 0 of only returning "recent crashes (180 days)"?

@@ +75,5 @@
> +          submitted: this._hasSuccessfulSubmission(crash.submissions),
> +          classifications: crash.classifications,
> +        }];
> +
> +        results.sort((a, b) => { return a.date - b.date; });

You don't need neither the return nor curly bracing with arrow functions.

::: browser/components/selfsupport/test/test_MozSelfSupport.html
@@ +72,5 @@
> +];
> +
> +function doTest() {
> +  SimpleTest.waitForExplicitFinish();
> +

We definitely need test cleanup here - multiple mochitests are run in the same environment/profile.
SimpleTest.registerCleanupFunction() should be available in mochitest-chrome and take a promise too.

@@ +75,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  Task.spawn(function () {
> +    for (let t of TESTS) {
> +      yield t();

What happens if t() rejects or throws?

::: dom/webidl/MozSelfSupport.webidl
@@ +44,5 @@
> +   *
> +   * [
> +   *   {
> +   *      id: String,
> +   *      type: String,

What kind of strings is this exposing? Either document possible values or point to where to find them.

@@ +47,5 @@
> +   *      id: String,
> +   *      type: String,
> +   *      date: Date,
> +   *      submitted: Boolean,
> +   *      classifications: Array of strings,

Same documentation issue for the classifications.

@@ +54,5 @@
> +   * ]
> +   *
> +   * The array is sorted by the date field.
> +   *
> +   * @return A promise that is resolved with the crash data.

What about failure here? I expect we want to expose an error from bug 1087388.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8500455 [details] [diff] [review]
Part 2: Add CrashManager.getCrash

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

r+ with Irvings comment fixed.

Comment 25

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8500475 [details] [diff] [review]
Part 3: Add MozSelfSupport.submitCrashReport

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

::: dom/webidl/MozSelfSupport.webidl
@@ +63,5 @@
> +   * Submits a crash report. The resolved object is of the form:
> +   *
> +   * {
> +   *   id: String,
> +   *   submitted: Boolean,

If the promise is resolved, |submitted==true| is implied. If it is rejected, |submitted==false| is implied and it doesn't seem meaningful to return this.

Could we just return the same form from getCrashes() and submitCrashReport() on success?
This might simplify things a lot for the caller - what do you think?

@@ +71,5 @@
> +   *        The crash ID to submit the crash report for. If the ID is invalid
> +   *        or if a crash report for the given ID has already been submitted,
> +   *        the returned promise is rejected.
> +   *
> +   * @return A promise that is resolved with the object described above.

And rejected with a failure of the form in bug 1087388?

Comment 26

[Biru [:poiru]]

Do you know if we still need/want this?

Comment 27

[Benjamin Smedberg]

Yes, we want this. It's not especially useful until we have codesigning, though.

Comment 28

[Biru [:poiru]]

Attachment: Part 1: Add MozSelfSupport.getCrashes

Comment 29

[Georg Fritzsche [:gfritzsche]]

Thanks for picking this up again, i'll only be able to take a look over your requests next week.

Comment 30

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #27)
> Yes, we want this. It's not especially useful until we have codesigning,
> though.

Do we still want this exposed through the current message-based API or only on the MozSelfSupport interface (and wait for code-signing)?

Comment 31

[Benjamin Smedberg]

Only on MozSelfSupport

Comment 32

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8633140 [details] [diff] [review]
Part 1: Add MozSelfSupport.getCrashes

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

Sorry, this dropped off my radar for a bit over Unified Telemetry.

::: browser/components/selfsupport/SelfSupportService.js
@@ +108,5 @@
> +
> +        results.sort((a, b) => a.date - b.date);
> +        return results;
> +      }).catch(() => {
> +        throw new MozSelfSupportError(MozSelfSupportError.INTERNAL,

Why not use something more speficic to the error here as the name?
Say "GetCrashesFailure"?

@@ +114,5 @@
> +      })
> +    );
> +  },
> +
> +  _hasSuccessfulSubmission(submissions) {

This doesn't need to be a method, lets just move it top-level.

::: browser/components/selfsupport/test/test_MozSelfSupport.html
@@ +20,5 @@
> +  yield store.save();
> +});
> +
> +let TESTS = [
> +  Task.async(function* test_getCrashes() {

We have add_task in mochitests now, see bug 1187701, comment 13:
https://dxr.mozilla.org/mozilla-central/source/modules/libjar/test/mochitest/test_bug1173171.html

::: dom/webidl/MozSelfSupport.webidl
@@ +50,5 @@
> +   *      id: String,
> +   *      type: String, // See CRASH_TYPE constants in CrashManager.jsm.
> +   *      date: Date,
> +   *      submitted: Boolean,
> +   *      classifications: Array of strings, // Unused, pending bug 1024672.

If its unused, lets just leave it out for now.
We can always add additional fields later.

Comment 33

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #32)
> ::: dom/webidl/MozSelfSupport.webidl
> @@ +50,5 @@
> > +   *      id: String,
> > +   *      type: String, // See CRASH_TYPE constants in CrashManager.jsm.
> > +   *      date: Date,
> > +   *      submitted: Boolean,
> > +   *      classifications: Array of strings, // Unused, pending bug 1024672.
> 
> If its unused, lets just leave it out for now.
> We can always add additional fields later.

It looks like bug 1024672 is complete - is the comment just outdated? Do we need additional test-coverage for the classifications.

Comment 34

[Biru [:poiru]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #33)
> It looks like bug 1024672 is complete - is the comment just outdated?

Yep.

Comment 35

[Firefox Bug Husbandry Bot]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1497137; component has been deprecated.