Last Comment Bug 777185 - Hook up content process crash reporting for B2G
: Hook up content process crash reporting for B2G
Status: RESOLVED FIXED
[WebAPI:P0][LOE:S]
: feature
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: ---
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
Depends on:
Blocks: b2g-crash-reporting
  Show dependency treegraph
 
Reported: 2012-07-24 17:39 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2012-10-16 10:33 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed


Attachments
WiP to submit dump (1.82 KB, patch)
2012-10-05 17:28 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Bug 777185 - Hookup content process crash reporting. (1.88 KB, patch)
2012-10-06 15:43 PDT, Hubert Figuiere [:hub]
fabrice: review-
Details | Diff | Splinter Review
Bug 777185 - Hookup content process crash reporting. (1.68 KB, patch)
2012-10-09 00:42 PDT, Hubert Figuiere [:hub]
fabrice: review+
Details | Diff | Splinter Review

Description Ted Mielczarek [:ted.mielczarek] 2012-07-24 17:39:01 PDT
For content process crash reporting, the parent process will have to listen to an observer service notification and submit a report based on the ID it receives.

This code was all hooked up in XUL Fennec, so you can probably just copy what it did.
Comment 1 Robert Kaiser 2012-07-25 05:47:11 PDT
https://github.com/mozilla-b2g/gaia/issues/2283 also has some discussion surrounding this.
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-20 10:40:30 PDT
Ted: Who do you think could do this? Could you or bsmedberg?

Assigning to Ted for now.
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-08-20 10:57:18 PDT
Not me, no. I don't know anything about the B2G chrome and I'm tied up in other work at the moment. It should be mostly copy-paste from the XUL fennec code.
Comment 4 Andrew Overholt [:overholt] 2012-08-22 10:34:37 PDT
Hub, is this something you can take?
Comment 5 Hubert Figuiere [:hub] 2012-09-28 15:36:59 PDT
This is my current priority. Working on it.
Comment 6 Hubert Figuiere [:hub] 2012-10-05 17:28:46 PDT
Created attachment 668686 [details] [diff] [review]
WiP to submit dump

It seems that we don't get a dumpID in case of abnormal termination. The dump file are there though.

I'm on PTO from now until Oct 19th. I will possibly have downtime to look at it, but if you really want to finish the patch before that, please, re-assign.
Comment 7 Hubert Figuiere [:hub] 2012-10-06 15:43:20 PDT
Created attachment 668844 [details] [diff] [review]
Bug 777185 - Hookup content process crash reporting.

Fixed and working patch.

I don't think I'll be able to commit this before Monday. I'll do my best, luckily being on Paris it should be easier.
Comment 8 [:fabrice] Fabrice Desré 2012-10-08 17:30:06 PDT
Comment on attachment 668844 [details] [diff] [review]
Bug 777185 - Hookup content process crash reporting.

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

I want to be sure of the potential QI issue.

::: b2g/chrome/content/shell.js
@@ +85,4 @@
>      try {
> +      if (crashID == undefined || crashID == "")
> +        crashID = Cc["@mozilla.org/xre/app-info;1"]
> +                  .getService(Ci.nsIXULRuntime).lastRunCrashID;

Nit: align .getService() with [

@@ +786,3 @@
>  (function contentCrashTracker() {
>    Services.obs.addObserver(function(aSubject, aTopic, aData) {
>        if(aTopic == "ipc:content-shutdown") {

That test is useless since we only register for this topic. Let's remove it.

@@ +786,5 @@
>  (function contentCrashTracker() {
>    Services.obs.addObserver(function(aSubject, aTopic, aData) {
>        if(aTopic == "ipc:content-shutdown") {
> +        let cs = Cc["@mozilla.org/consoleservice;1"]
> +                 .getService(Ci.nsIConsoleService);

Nit: indentation

@@ +791,5 @@
> +        cs.logStringMessage("content-shutdown has been caught");
> +
> +        if (aSubject.QueryInterface(Ci.nsIPropertyBag2).hasKey("abnormal")) {
> +          cs.logStringMessage("abnormal shutdown");
> +          if (aSubject.hasKey("dumpID")) {

don't you need to QI here also? Storing aSubject.QueryInterface(Ci.nsIPropertyBag2) in a local would make it clearer to me.
Comment 9 Hubert Figuiere [:hub] 2012-10-09 00:33:26 PDT
Comment on attachment 668844 [details] [diff] [review]
Bug 777185 - Hookup content process crash reporting.

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

::: b2g/chrome/content/shell.js
@@ +791,5 @@
> +        cs.logStringMessage("content-shutdown has been caught");
> +
> +        if (aSubject.QueryInterface(Ci.nsIPropertyBag2).hasKey("abnormal")) {
> +          cs.logStringMessage("abnormal shutdown");
> +          if (aSubject.hasKey("dumpID")) {

It does not seem to be necessary to QI the second time. BUT, I'm ok with use a local. Will update with that change.
Comment 10 Hubert Figuiere [:hub] 2012-10-09 00:42:47 PDT
Created attachment 669427 [details] [diff] [review]
Bug 777185 - Hookup content process crash reporting.

addressed comments
Comment 11 [:fabrice] Fabrice Desré 2012-10-10 15:31:32 PDT
Comment on attachment 669427 [details] [diff] [review]
Bug 777185 - Hookup content process crash reporting.

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

::: b2g/chrome/content/shell.js
@@ +788,5 @@
> +      let cs = Cc["@mozilla.org/consoleservice;1"]
> +                 .getService(Ci.nsIConsoleService);
> +      let props = aSubject.QueryInterface(Ci.nsIPropertyBag2);
> +      if (props.hasKey("abnormal")) {
> +        if (props.hasKey("dumpID")) {

nit: if (props.hasKey("abnormal") && props.hasKey("dumpID")) {
...
}
Comment 13 Ed Morley [:emorley] 2012-10-11 12:05:42 PDT
https://hg.mozilla.org/mozilla-central/rev/3fca49e8a93a
Comment 15 Robert Kaiser 2012-10-16 09:37:00 PDT
So, I think this is working, given those two reports I have seen from unagi devices:
bp-62664bb5-3295-441b-8a04-d379d2121016 and bp-a4035b13-3470-4fb1-b282-aa28b2121016
Comment 16 Hubert Figuiere [:hub] 2012-10-16 10:33:48 PDT
Excellent !!!!

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