Closed Bug 1457231 Opened 2 years ago Closed 2 years ago

Crash reporting not working in child process

Categories

(GeckoView :: General, enhancement, P1)

59 Branch
enhancement

Tracking

(firefox60 unaffected, firefox61 wontfix, firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox60 --- unaffected
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: snorp, Assigned: snorp)

Details

Attachments

(1 file)

With the patches from bug 1433968 applied, we should be able to report crashes from the child process correctly, but that doesn't seem to happen. There are basically no messages in the logcat other than the process dies.
Assignee: nobody → snorp
Priority: -- → P1
Comment on attachment 8981980 [details]
Bug 1457231 - Submit content process crash reports in GeckoView

https://reviewboard.mozilla.org/r/248010/#review254124

r- for the observer removal issue.

::: mobile/android/modules/geckoview/GeckoViewContent.jsm:11
(Diff revision 1)
>  
>  var EXPORTED_SYMBOLS = ["GeckoViewContent"];
>  
>  ChromeUtils.import("resource://gre/modules/GeckoViewModule.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");

Put this under `defineLazyModuleGetters`

::: mobile/android/modules/geckoview/GeckoViewContent.jsm:28
(Diff revision 1)
>          "GeckoView:SaveState",
>          "GeckoView:SetActive",
>          "GeckoView:ZoomToInput",
>      ]);
>  
> +    Services.obs.addObserver(this, "ipc:content-shutdown");

This observer is never removed.

::: mobile/android/modules/geckoview/GeckoViewContent.jsm:29
(Diff revision 1)
>          "GeckoView:SetActive",
>          "GeckoView:ZoomToInput",
>      ]);
>  
> +    Services.obs.addObserver(this, "ipc:content-shutdown");
> +    Services.obs.addObserver(this, "oop-frameloader-crashed");

We never handle this?
Attachment #8981980 - Flags: review?(nchen) → review-
Comment on attachment 8981980 [details]
Bug 1457231 - Submit content process crash reports in GeckoView

https://reviewboard.mozilla.org/r/248010/#review254128

::: mobile/android/modules/geckoview/GeckoViewContent.jsm:136
(Diff revision 1)
>    }
> +
> +  // nsIObserver event handler
> +  observe(aSubject, aTopic, aData) {
> +    switch (aTopic) {
> +      case "ipc:content-shutdown": {

Also, if we have multiple sessions, seems like they will all try to handle this at the same time?
Comment on attachment 8981980 [details]
Bug 1457231 - Submit content process crash reports in GeckoView

https://reviewboard.mozilla.org/r/248010/#review254154

::: mobile/android/modules/geckoview/GeckoViewContent.jsm:28
(Diff revision 1)
>          "GeckoView:SaveState",
>          "GeckoView:SetActive",
>          "GeckoView:ZoomToInput",
>      ]);
>  
> +    Services.obs.addObserver(this, "ipc:content-shutdown");

The convention seems to be to not unregister things that are initialized in onInit(). We don't unregister the other listeners here either. That does seem like it would be problematic, but nsIObserverService only owns a weak reference to the listener so this specific case should be ok.

::: mobile/android/modules/geckoview/GeckoViewContent.jsm:29
(Diff revision 1)
>          "GeckoView:SetActive",
>          "GeckoView:ZoomToInput",
>      ]);
>  
> +    Services.obs.addObserver(this, "ipc:content-shutdown");
> +    Services.obs.addObserver(this, "oop-frameloader-crashed");

Whoops, bleed through from the other patches.

::: mobile/android/modules/geckoview/GeckoViewContent.jsm:136
(Diff revision 1)
>    }
> +
> +  // nsIObserver event handler
> +  observe(aSubject, aTopic, aData) {
> +    switch (aTopic) {
> +      case "ipc:content-shutdown": {

Ah, that's a good point. This crash handling bit just needs to be in a singleton. I'll rework.
Comment on attachment 8981980 [details]
Bug 1457231 - Submit content process crash reports in GeckoView

https://reviewboard.mozilla.org/r/248010/#review254154

> The convention seems to be to not unregister things that are initialized in onInit(). We don't unregister the other listeners here either. That does seem like it would be problematic, but nsIObserverService only owns a weak reference to the listener so this specific case should be ok.

The event dispatcher or message manager listeners are tied to the window, and are automatically unregistered on window closing. That's not the case for global observers, so we need to unregister explicitly. `nsIObserverService` owns a strong reference by default unless you request a weak reference.
Comment on attachment 8981980 [details]
Bug 1457231 - Submit content process crash reports in GeckoView

https://reviewboard.mozilla.org/r/248010/#review254244

::: mobile/android/components/geckoview/GeckoViewStartup.js:62
(Diff revision 2)
>            this.setResourceSubstitutions();
>  
>            Services.mm.loadFrameScript(
>                "chrome://geckoview/content/GeckoViewPromptContent.js", true);
> +
> +          ContentCrashHandler.init();

Please use `GeckoViewUtils.addLazyGetter` and load `ContentCrashHandler` only when needed.
Attachment #8981980 - Flags: review?(nchen) → review-
(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> Comment on attachment 8981980 [details]
> Bug 1457231 - Submit content process crash reports in GeckoView
> 
> https://reviewboard.mozilla.org/r/248010/#review254244
> 
> ::: mobile/android/components/geckoview/GeckoViewStartup.js:62
> (Diff revision 2)
> >            this.setResourceSubstitutions();
> >  
> >            Services.mm.loadFrameScript(
> >                "chrome://geckoview/content/GeckoViewPromptContent.js", true);
> > +
> > +          ContentCrashHandler.init();
> 
> Please use `GeckoViewUtils.addLazyGetter`

We use GeckoViewUtils unconditionally in the script startup in order to call initLogging(), so it doesn't seem like it would be lazy at all. Is there some other benefit I'm not aware of?

> and load `ContentCrashHandler`
> only when needed.

Only when native crash reporting is enabled? I think this is going to be more trouble than it's worth. AFAICT, we don't have GeckoRuntimeSettings exposed to JS at all right now. ContentCrashHandler is ~50 lines and simply hooks up a listener, so it doesn't seem that bad to me if we do that all the time. Also, in practice, every app we have planned is going to have native crash reporting enabled anyway.
Flags: needinfo?(nchen)
`GeckoViewUtils.addLazyGetter` lets you load a module only when an observer notification happens. In this case, you would let it watch for "ipc:content-shutdown", and only load `ContentCrashHandler` when a crash happens. Something like,

> GeckoViewUtils.addLazyGetter(this, "ContentCrashHandler", {
>   module: "resource://gre/modules/ContentCrashHandler.jsm",
>   observers: [
>     "ipc:content-shutdown",
>   },
> });
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #9)
> `GeckoViewUtils.addLazyGetter` lets you load a module only when an observer
> notification happens. In this case, you would let it watch for
> "ipc:content-shutdown", and only load `ContentCrashHandler` when a crash
> happens. Something like,
> 
> > GeckoViewUtils.addLazyGetter(this, "ContentCrashHandler", {
> >   module: "resource://gre/modules/ContentCrashHandler.jsm",
> >   observers: [
> >     "ipc:content-shutdown",
> >   },
> > });

Ah, I see we do that for GeckoViewPermissions above as well. I'll fix.
Comment on attachment 8981980 [details]
Bug 1457231 - Submit content process crash reports in GeckoView

https://reviewboard.mozilla.org/r/248010/#review254474

::: mobile/android/modules/geckoview/ContentCrashHandler.jsm:42
(Diff revision 3)
> +    debug `Submitting content process crash, dump ID ${dumpID}`;
> +    CrashSubmit.submit(dumpID, {}).then(crashID => {
> +      debug `Crash submission successful: ${crashID}`;
> +    }, ChromeUtils.reportError);
> +  }
> +}

Semicolon
Attachment #8981980 - Flags: review?(nchen) → review+
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/951bf64cc798
Submit content process crash reports in GeckoView r=jchen
https://hg.mozilla.org/mozilla-central/rev/951bf64cc798
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox for Android → GeckoView
Version: Firefox 59 → 59 Branch
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.