1459349 - Android nightly builds stopped reporting crashes in Build 20180503100147

Status: VERIFIED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Marcia Knous [:marcia]]

Starting with Build 20180503100147, I don't see any Nightly Fennec crashes in Crash Stats. 

Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=87bd488c19f620d726b8363d47c8a320bae9bb7c&tochange=a2d1d4158bb4718d8bb31e1284e133aa99273600 is the regression range

Bug 1433968 landed in that timeframe - :snorp can you please check?

Comment 1

[Marcia Knous [:marcia]]

The other possibility is that something changed on the Socorro side, but I do see crashes for the Desktop nightly builds on those dates.

Comment 2

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

Attachment: Bug 1459349 - Use GeckoRuntime to launch Gecko in Fennec

Review commit: https://reviewboard.mozilla.org/r/241900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241900/

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8973546 [details]
Bug 1459349 - Use GeckoRuntime to launch Gecko in Fennec

https://reviewboard.mozilla.org/r/241900/#review247774

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:485
(Diff revision 1)
>  
>          final List<String> env = getEnvFromExtras(mExtras);
>  
>          // In Gecko, the native crash reporter is enabled by default in opt builds, and
>          // disabled by default in debug builds.
> -        if ((mFlags & FLAG_ENABLE_NATIVE_CRASHREPORTER) == 0 && !BuildConfig.DEBUG_BUILD) {
> +        if (!GeckoAppShell.isFennec() &&

Explain to me why we can't arrange for Fennec to tell GV what to do (a push model) as opposed to having GV know what to do for Fennec (a pull model).

Comment 4

[Ryan VanderMeulen [:RyanVM]]

I'm having bug 1433968 backed out so we can respin nightlies and proceed with today's 61b3 go-to-build.

Comment 5

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

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8973546 [details]
> Bug 1459349 - Make sure crash reporting is turned on in Fennec builds
> 
> https://reviewboard.mozilla.org/r/241900/#review247774
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:485
> (Diff revision 1)
> >  
> >          final List<String> env = getEnvFromExtras(mExtras);
> >  
> >          // In Gecko, the native crash reporter is enabled by default in opt builds, and
> >          // disabled by default in debug builds.
> > -        if ((mFlags & FLAG_ENABLE_NATIVE_CRASHREPORTER) == 0 && !BuildConfig.DEBUG_BUILD) {
> > +        if (!GeckoAppShell.isFennec() &&
> 
> Explain to me why we can't arrange for Fennec to tell GV what to do (a push
> model) as opposed to having GV know what to do for Fennec (a pull model).

I agree that would be better, but currently Fennec uses the default GeckoRuntime[0]. We'd need to do one of the following:

1) Make Fennec use an explicitly created GeckoRuntime instance that configures crash reporting appropriately.
2) Make the default GeckoRuntime use the configuration that Fennec expects.

I think 1) is the option we want to use here but I didn't want to go that route for the fire drill. There are other Fennec-specific things with crash reporting so I don't think this makes things much worse.

[0] https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java#41

Comment 6

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

Comment on attachment 8973546 [details]
Bug 1459349 - Use GeckoRuntime to launch Gecko in Fennec

https://reviewboard.mozilla.org/r/241900/#review247886

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1907
(Diff revision 1)
>          // e.g. "en", "en-US", or "en-US-POSIX".
>          return out.toString();
>      }
> +
> +    private static Boolean sIsFennec;
> +    public static boolean isFennec() {

Whoops, need synchronized here. I'll add that in a new patch.

Comment 7

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

Comment on attachment 8973546 [details]
Bug 1459349 - Use GeckoRuntime to launch Gecko in Fennec

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241900/diff/1-2/

Comment 8

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

Comment on attachment 8973546 [details]
Bug 1459349 - Use GeckoRuntime to launch Gecko in Fennec

https://reviewboard.mozilla.org/r/241900/#review247892

I think I'd rather have `GeckoRuntime.setSettingsForDefaultRuntime` that lets you set the settings used to create the default runtime, than the hack here, unless you think that's too risky.

Comment 9

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

(In reply to Jim Chen [:jchen] [:darchons] from comment #8)
> Comment on attachment 8973546 [details]
> Bug 1459349 - Make sure crash reporting is turned on in Fennec builds
> 
> https://reviewboard.mozilla.org/r/241900/#review247892
> 
> I think I'd rather have `GeckoRuntime.setSettingsForDefaultRuntime` that
> lets you set the settings used to create the default runtime, than the hack
> here, unless you think that's too risky.

I think I would rather do option 1) above than that. If you can set the settings for the default runtime, there isn't really much reason to have the default runtime at all, IMHO.

Comment 10

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

Comment on attachment 8973546 [details]
Bug 1459349 - Use GeckoRuntime to launch Gecko in Fennec

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241900/diff/2-3/

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed1813390ded9c43b0cd0c8f275b1bd9ef33b183

Comment 12

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

Comment on attachment 8973546 [details]
Bug 1459349 - Use GeckoRuntime to launch Gecko in Fennec

https://reviewboard.mozilla.org/r/241900/#review247916

General idea looks good, but need to sort out some `GeckoApp` and `GeckoService` details.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1002
(Diff revision 3)
>              Log.i(LOGTAG, "System locale changed. Restarting.");
>              finishAndShutdown(/* restart */ true);
>              return;
>          }
>  
> -        if (sAlreadyLoaded) {
> +        if (GeckoApplication.getRuntime() != null) {

I think we need to keep `sAlreadyLoaded`, because it's supposed to indicate if `GeckoApp` is already loaded, not if Gecko is already loaded.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
(Diff revision 3)
>              Telemetry.addToHistogram("FENNEC_RESTORING_ACTIVITY", 1);
>  
>          } else {
>              final String action = intent.getAction();
>              final String[] args = GeckoApplication.getDefaultGeckoArgs();
> -            final int flags = ACTION_DEBUG.equals(action) ? GeckoThread.FLAG_DEBUGGING : 0;

jimdb uses this IIRC

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
(Diff revision 3)
>              "Update:Check",
>              "Update:Download",
>              "Update:Install",
>              null);
>  
> -        GeckoThread.launch();

We need to launch after registering listeners above

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:219
(Diff revision 3)
>  
>          mInBackground = false;
>      }
>  
> +    private static GeckoRuntime sGeckoRuntime;
> +    public static synchronized GeckoRuntime getRuntime() {

`GeckoRuntime` is only usable from the UI thread, so we don't need to synchronize

::: mobile/android/base/java/org/mozilla/gecko/GeckoService.java:195
(Diff revision 3)
>          if (!initGecko(intent)) {
>              stopSelf(startId);
>              return Service.START_NOT_STICKY;
>          }
>  
> -        GeckoThread.launch();
> +        GeckoApplication.ensureRuntime(this);

I think this can throw an exception because `GeckoThread` can already be initialized at this point through `GeckoThread.initMainProcessWithProfile`

Comment 13

[Ryan VanderMeulen [:RyanVM]]

The backout is verified to be working for 61 at this point. Not sure if we want to leave this bug open still or fold the work here into the relanding of bug 1433968?

Comment 14

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

I'll continue the work back on bug 1433968