Closed Bug 1459349 Opened Last year Closed Last year

Android nightly builds stopped reporting crashes in Build 20180503100147

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 blocking verified
firefox62 blocking verified

People

(Reporter: marcia, Assigned: snorp)

References

Details

(Keywords: regression)

Attachments

(1 file)

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?
Flags: needinfo?(snorp)
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 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).
Attachment #8973546 - Flags: review-
I'm having bug 1433968 backed out so we can respin nightlies and proceed with today's 61b3 go-to-build.
(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
Flags: needinfo?(snorp)
Attachment #8973546 - Flags: review?(esawin) → review?(nchen)
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 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.
Attachment #8973546 - Flags: review?(nchen) → review-
(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 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`
Attachment #8973546 - Flags: review?(nchen) → review-
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?
Flags: needinfo?(snorp)
I'll continue the work back on bug 1433968
Status: NEW → RESOLVED
Closed: Last year
Flags: needinfo?(snorp)
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.