Last Comment Bug 773892 - Implement minimal (headless) crash submitter for gonk
: Implement minimal (headless) crash submitter for gonk
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
Depends on: 761909 777145
Blocks: b2g-crash-reporting 774290
  Show dependency treegraph
 
Reported: 2012-07-13 18:00 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2012-09-27 03:59 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
+


Attachments
Bug 773892 - Add crash submission when starting b2g. (1.83 KB, patch)
2012-07-18 16:33 PDT, Hubert Figuiere [:hub]
ted: feedback+
Details | Diff | Review
Bug 773892 - Add crash submission when starting b2g. (1.92 KB, patch)
2012-07-23 10:19 PDT, Hubert Figuiere [:hub]
ted: review+
Details | Diff | Review
Bug 773892 - Part 2: Check for network status and delay until reconnect. (1.18 KB, patch)
2012-08-01 08:42 PDT, Hubert Figuiere [:hub]
21: review+
Details | Diff | Review
Bug 773892 - Part 3: only rely on the event to send the crash report. (754 bytes, patch)
2012-09-25 19:22 PDT, Hubert Figuiere [:hub]
fabrice: review+
kairo: feedback+
Details | Diff | Review

Description Ted Mielczarek [:ted.mielczarek] 2012-07-13 18:00:37 PDT
Split this off from bug 761909. That bug implements just the plumbing for crash reporting. minidumps will get written, but no crashreporter will be launched on crash. On restart, the last crash ID will be exposeod as nsIXULRuntime.lastCrashID, which chrome code can pass to CrashSubmit.jsm's submit method. This bug covers that last bit, hooking up the submission in the chrome code. Per a previous discussion we also want to have a pref somewhere to control automatic submission.
Comment 1 Robert Kaiser (not working on stability any more) 2012-07-13 18:10:10 PDT
I believe having crash reporting working for B2G should block basecamp.
Comment 2 David Bolter [:davidb] 2012-07-17 11:44:05 PDT
Hub agreed to jump on this :)

Ted I lost my IRC log - can you reiterate what this bug basically requires?
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-07-17 11:45:55 PDT
It should all be summarized in comment 0.
Comment 4 Hubert Figuiere [:hub] 2012-07-18 16:33:23 PDT
Created attachment 643665 [details] [diff] [review]
Bug 773892 - Add crash submission when starting b2g.

Here is my proposed fix. Requesting feedback to see if that's what you envisioned.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2012-07-18 17:16:21 PDT
Comment on attachment 643665 [details] [diff] [review]
Bug 773892 - Add crash submission when starting b2g.

Conceptually this is doing all the right things. Making it happen at the right *time* is the hard part.
Comment 6 Hubert Figuiere [:hub] 2012-07-23 10:19:38 PDT
Created attachment 644970 [details] [diff] [review]
Bug 773892 - Add crash submission when starting b2g.

Updated and test patch. Reporting is moved to the event loop in the startup phase.

(needs bug 761909)
Comment 7 Hubert Figuiere [:hub] 2012-07-23 12:27:47 PDT
Crash Reports are left in "pending" because it seems that there is no network access.
Comment 8 Ted Mielczarek [:ted.mielczarek] 2012-07-23 12:35:08 PDT
We need to make sure that works. Can you add a listener for network state and send when the network is online?
Comment 9 Hubert Figuiere [:hub] 2012-07-24 14:49:43 PDT
I don't seem to be able to get the "online" event from the window. My event listener is never called.
Comment 10 Philipp von Weitershausen [:philikon] 2012-07-24 16:10:02 PDT
(In reply to Hub Figuiere [:hub] from comment #9)
> I don't seem to be able to get the "online" event from the window. My event
> listener is never called.

It's true, we don't currently update nsIIOService::offline (which I believe controls navigator.onLine and the online/offline events). I filed bug 777145 for this. Note that if you're in chrome code (this includes shell.js), it's probably better to just use nsIIOService (Services.io) and/or the observer notifications.
Comment 11 Ted Mielczarek [:ted.mielczarek] 2012-07-27 05:47:35 PDT
Comment on attachment 644970 [details] [diff] [review]
Bug 773892 - Add crash submission when starting b2g.

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

This code looks fine, but I think we probably still want to observe the network status and only try to submit if we're online. Also, we may want to consider only sending when on WiFi (although minidumps are fairly small, usually only ~100KB).
Comment 12 Hubert Figuiere [:hub] 2012-07-27 14:02:20 PDT
(In reply to Ted Mielczarek [:ted] from comment #11)
> This code looks fine, but I think we probably still want to observe the
> network status and only try to submit if we're online. Also, we may want to
> consider only sending when on WiFi (although minidumps are fairly small,
> usually only ~100KB).

I'll make this part 2. There are a couple of blockers on the platform that needs to be ironed out. And yes I was wondering if we should only do it over wifi. Will add that to part 2.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-07-28 18:35:42 PDT
https://hg.mozilla.org/mozilla-central/rev/64d7f92c3029
Comment 15 Hubert Figuiere [:hub] 2012-07-31 19:37:10 PDT
ted, I'm getting this on the console when actually submitting:

E/GeckoConsole( 4369): crash-reports.mozilla.com : server does not support RFC 5746, see CVE-2009-3555
Comment 16 Ted Mielczarek [:ted.mielczarek] 2012-08-01 05:08:34 PDT
I don't think that's harmful.
Comment 17 Robert Kaiser (not working on stability any more) 2012-08-01 05:35:39 PDT
Right, I've seen this warning for quite a few servers, it's just a warning, the connection (and therefore submission) works without problems. Actually, you could see that log message as a confirmation that the live server was actually contacted. ;-)
Comment 18 Hubert Figuiere [:hub] 2012-08-01 08:38:50 PDT
I don't see it all the time either. :-)
Comment 19 Hubert Figuiere [:hub] 2012-08-01 08:42:00 PDT
Created attachment 647964 [details] [diff] [review]
Bug 773892 - Part 2: Check for network status and delay until reconnect.

Need the blocking bugs to be fixed to work.
Comment 20 Ted Mielczarek [:ted.mielczarek] 2012-08-01 09:41:10 PDT
Comment on attachment 647964 [details] [diff] [review]
Bug 773892 - Part 2: Check for network status and delay until reconnect.

You probably just want review from a B2G peer on this.
Comment 21 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-01 10:12:59 PDT
Comment on attachment 647964 [details] [diff] [review]
Bug 773892 - Part 2: Check for network status and delay until reconnect.

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

::: b2g/chrome/content/shell.js
@@ +89,5 @@
>          crashID) {
> +
> +      if (!Services.io.offline) {
> +        this.CrashSubmit.submit(crashID);
> +      } else {

Do an early return.

@@ +90,5 @@
> +
> +      if (!Services.io.offline) {
> +        this.CrashSubmit.submit(crashID);
> +      } else {
> +        Services.obs.addObserver(function observer(aSubject, aTopic, aState) {

nit: subject, topic, state

This file does not prefix arguments.
Comment 23 Ed Morley [:emorley] 2012-08-30 03:52:05 PDT
https://hg.mozilla.org/mozilla-central/rev/0bdc5212ba83
Comment 24 Robert Kaiser (not working on stability any more) 2012-08-30 05:17:23 PDT
Hub, does the dependency landing mean that this bug can be resolved fixed? (I know you have trouble testing at least some parts with your device because of bug 778360, but should it work for other devices like otoro now?)
Comment 25 Hubert Figuiere [:hub] 2012-08-30 09:37:24 PDT
It should work for Otoro, so we can resolve it. The other fix is coming as well anyway.
Comment 26 Robert Kaiser (not working on stability any more) 2012-08-30 10:16:12 PDT
OK, marking this way per Hub's comment #25. Thanks a ton for this work!

I'd love someone with an Otoro to test (not me yet, but hopefully soon) and would love to see a Gecko crash report submitted from a device. If that works, we can go pushing forward on the other needed parts (app crashes, etc).
Comment 27 Hubert Figuiere [:hub] 2012-08-30 10:23:45 PDT
Don't forget to check https://bugzilla.mozilla.org/show_bug.cgi?id=761909#c13 before testing. Otherwise you won't have crash reporting.
Comment 28 Robert Kaiser (not working on stability any more) 2012-08-30 11:07:50 PDT
(In reply to Hub Figuiere [:hub] from comment #27)
> Don't forget to check
> https://bugzilla.mozilla.org/show_bug.cgi?id=761909#c13 before testing.
> Otherwise you won't have crash reporting.

Thanks, we need to add that into the mozconfigs for the Nightly builds as well, I mentioned that in bug 785698 now, which already has other releng changes we need.
Comment 29 Robert Kaiser (not working on stability any more) 2012-09-25 10:41:02 PDT
Even though this and all dependencies are fixed now, this doesn't seem to work, the reports keep going to "pending" only and not being submitted. Hub, does it work on your device? Should we reopen here or file a new bug?
Comment 30 Hubert Figuiere [:hub] 2012-09-25 10:48:41 PDT
It has worked on my device in the past - but lot of things changed since. 

Indeed they seem to be stuck now.

I'll have a look.
Comment 31 Hubert Figuiere [:hub] 2012-09-25 13:30:46 PDT
There seems to be something time dependent in the crash submission. Adding some Services.console.logStringMessage() and al make it work.
Comment 32 Hubert Figuiere [:hub] 2012-09-25 15:37:07 PDT
reopening
Comment 33 Hubert Figuiere [:hub] 2012-09-25 18:19:30 PDT
If Services.io.offline is false, we try to submit the crash report immediately. In that case it fails. Tracing it show that Submitter.prototype.submitForm() xhr.status is 0 (we expect 200 on success).
Comment 34 Hubert Figuiere [:hub] 2012-09-25 19:22:30 PDT
Created attachment 664757 [details] [diff] [review]
Bug 773892 - Part 3: only rely on the event to send the crash report.
Comment 35 Robert Kaiser (not working on stability any more) 2012-09-25 19:28:34 PDT
Comment on attachment 664757 [details] [diff] [review]
Bug 773892 - Part 3: only rely on the event to send the crash report.

From what you told me on IRC, this looks good to me, but I technically don't think I can review B2G or gaia patches, so I'll defer to Vivien, given he has reviewed the previous patch here as well.
Comment 36 Hubert Figuiere [:hub] 2012-09-26 15:47:38 PDT
Comment on attachment 664757 [details] [diff] [review]
Bug 773892 - Part 3: only rely on the event to send the crash report.

maybe fabrice can review this. Thanks !
Comment 38 Ed Morley [:emorley] 2012-09-27 03:59:22 PDT
https://hg.mozilla.org/mozilla-central/rev/b2644a3626ec

Note You need to log in before you can comment on or make changes to this bug.