Hook up content process crash reporting for B2G

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ted, Assigned: hub)

Tracking

(Blocks: 1 bug, {feature})

unspecified
ARM
Gonk (Firefox OS)
feature

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed)

Details

(Whiteboard: [WebAPI:P0][LOE:S])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

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

Updated

5 years ago
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Ted: Who do you think could do this? Could you or bsmedberg?

Assigning to Ted for now.
Assignee: nobody → ted.mielczarek
(Reporter)

Comment 3

5 years ago
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.
Assignee: ted.mielczarek → nobody
Hub, is this something you can take?
Assignee: nobody → hub
Whiteboard: [WebAPI:P0]
Keywords: feature
(Assignee)

Updated

5 years ago
Whiteboard: [WebAPI:P0] → [WebAPI:P0][LOE:S]
(Assignee)

Comment 5

5 years ago
This is my current priority. Working on it.
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #668686 - Attachment is obsolete: true
Attachment #668844 - Flags: review?(fabrice)
Attachment #668844 - Flags: feedback?(ted.mielczarek)
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.
Attachment #668844 - Flags: review?(fabrice) → review-
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
Created attachment 669427 [details] [diff] [review]
Bug 777185 - Hookup content process crash reporting.

addressed comments
Attachment #668844 - Attachment is obsolete: true
Attachment #668844 - Flags: feedback?(ted.mielczarek)
Attachment #669427 - Flags: review?(fabrice)
Attachment #669427 - Flags: feedback?
(Assignee)

Updated

5 years ago
Attachment #669427 - Flags: feedback? → feedback?(ted.mielczarek)
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")) {
...
}
Attachment #669427 - Flags: review?(fabrice) → review+
(Assignee)

Updated

5 years ago
Attachment #669427 - Flags: feedback?(ted.mielczarek)
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fca49e8a93a
https://hg.mozilla.org/mozilla-central/rev/3fca49e8a93a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd21c60ab0ff
status-firefox18: --- → fixed

Comment 15

5 years ago
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
(Assignee)

Comment 16

5 years ago
Excellent !!!!
You need to log in before you can comment on or make changes to this bug.