1322175 - [geckoview] Move crash handling out of GeckoView and into Fennec

Status: RESOLVED DUPLICATE of bug 1433968

(GeckoView :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

Right now, GeckoView has (two?!?) implementations of crash handling (both referencing AppConstants) and the whole App has some complicated crash handling on the Java side.

This ticket tracks doing two things:

1) pushing bits into GeckoInterface (or a new delegate) so that we can extract the Fennec bits from the GeckoView bits;

2) simplifying the whole lot.  For example:

2a) it appears that we have per-thread crash handling, which is unused.

2b) we have this awkward `ensureCrashHandling` method, which the code base implies needs to happen at all entry points.  If I understand correctly, our crash handling is actually per-process.  We can use GeckoApplication (in Fennec!) to simplify this installation for the main process, and handle other processes (remote tabs, updater, etc) independently.

The existing code appears a little crufty and unloved and might be ripe for simplification.  And we can remove the last few references to AppConstants from GeckoView \o/

Comment 1

[Nick Alexander :nalexander [he/him]]

jchen, snorp: can you comment on the per-thread/per-process crash handling?  Am I missing something subtle here?

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Well, CrashHandler was written to be usable by other (possibly independent) modules such as search activity and stumbler, hence the per-thread/per-process options (and also why GeckoAppShell uses a subclass of CrashHandler).

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> Well, CrashHandler was written to be usable by other (possibly independent)
> modules such as search activity and stumbler, hence the
> per-thread/per-process options (and also why GeckoAppShell uses a subclass
> of CrashHandler).

Yeah, as written, I really doubt this will work with different crash handlers.  It's definitely funky, since there's a per-process thing hidden in the GeckoAppShell.CRASH_HANDLER variable.  We should just do this once in GeckoApplication for the main process, and make it clear that the individual processes have separate crash handlers otherwise.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, I support moving all crash handling out of GV and into Fennec. I do think 2b) above is basically what we want to do.

Comment 6

[Nick Alexander :nalexander [he/him]]

jchen: can you make this happen?  There's a lot of stuff here that doesn't make sense for arbitrary GeckoView consumers, and it's getting in the way of improving the Gradle integration.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Is the Gradle problem due to using AppConstants? I think we want to remove AppConstants dependency from CrashHandler/GeckoAppShell, but keep CrashHandler in GeckoView, because,

1) GeckoView still has C++ crash handling, so moving CrashHandler out of GeckoView only matters for a small set of cases such as before Gecko is loaded or in processes where Gecko is not running.

2) The vision is for most of GeckoView to live in a service process where only Gecko/GeckoView code runs. In that case it makes sense to have crash handling for the service process.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> Is the Gradle problem due to using AppConstants? I think we want to remove
> AppConstants dependency from CrashHandler/GeckoAppShell, but keep
> CrashHandler in GeckoView, because,

I've got patches to move AppConstants stuff into BuildConfig, but a lot of that stuff _makes no sense_ for GeckoView.  It's only used by this default CRASH_HANDLER at http://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java#112, which just shouldn't be there.

> 1) GeckoView still has C++ crash handling, so moving CrashHandler out of
> GeckoView only matters for a small set of cases such as before Gecko is
> loaded or in processes where Gecko is not running.
> 
> 2) The vision is for most of GeckoView to live in a service process where
> only Gecko/GeckoView code runs. In that case it makes sense to have crash
> handling for the service process.

I get this, and that makes sense.  What doesn't make sense is for GeckoView to bake in a bunch of Fennec-specific bits; that's what we need to address.

I'll push my patches shortly and you can work on addressing the bits labelled "this doesn't make sense" :)