777185 - Hook up content process crash reporting for B2G

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[(not currently active) Ted Mielczarek]

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]

https://github.com/mozilla-b2g/gaia/issues/2283 also has some discussion surrounding this.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ted: Who do you think could do this? Could you or bsmedberg?

Assigning to Ted for now.

Comment 3

[(not currently active) Ted Mielczarek]

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]]

Hub, is this something you can take?

Comment 5

[Hubert Figuiere [:hub]]

This is my current priority. Working on it.

Comment 6

[Hubert Figuiere [:hub]]

Attachment: 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]]

Attachment: 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é]

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]]

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]]

Attachment: Bug 777185 - Hookup content process crash reporting.

addressed comments

Comment 11

[[:fabrice] Fabrice Desré]

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 12

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fca49e8a93a

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3fca49e8a93a

Comment 14

[Hubert Figuiere [:hub]]

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

Comment 15

[Robert Kaiser]

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]]

Excellent !!!!