1433968 - Figure out GeckoView crash reporting (Sentry and/or Breakpad?)

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

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

Most apps (even our internal ones) are not going to want to use the Mozilla crash reporting system. Things like Crashlytics are much easier to use. We should probably just disable the Gecko crash reporter and allow the app to use whatever they want. We could try to distribute the Gecko symbols along with the AAR so other crash reporting systems can resolve our symbols.

Comment 2

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

Medium/long-term, I think we want to move all Gecko stuff into its own service (process), and just automatically report crashes -- be it in Java or native code -- as long as the user has consented beforehand. The app is then free to do whatever it wants. But that's not where we are today.

For Klar GeckoView I think we have several feasible options, AFAIK, Focus (Klar) does not use any crash reporting mechanism. They simply reply on the Play Store to find out about crashes. Anything we choose here should be better than that.

1) Use breakpad as it exists in Gecko[View] today, and handle both Klar and GeckoView crashes. This would be close to what Fennec does now, but we may want to reconsider the crash report screen.

2) Crashlytics is the "idiomatic" service for handling crashes on Android. We could try using that for both Klar and Gecko[View] crashes. I'm not sure how well Crashlytics will do with native code. Maybe we can upload symbols somewhere? There is also the issue of (possibly sensitive) user data being stored in a non-Mozilla property.

3) Use Crashlytics (or something else) for Klar crashes, but use breakpad for Gecko[View] crashes. The way I see this working is that GeckoView would have first crack at any unhandled Java exception, and if it's not in a GeckoView package, it can kick it down the road to Klar via some kind of delegate system. Native crashes would always be assumed to be in Gecko. The reporting would be as in 1) for the breakpad case.

Sebastian, do you see other options? Do you like any of these?

Comment 3

[Chris Peterson [:cpeterson]]

Adding `[geckoview:klar]` whiteboard tag because we need crash reporting to ship Klar+GeckoView.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

> AFAIK, Focus (Klar) does not use any crash reporting mechanism. They simply reply on the Play Store to find out about crashes.

That's correct. So far we are using the crash reporting that is integrated into Google Play only.

> Anything we choose here should be better than that.

Yes and no. The current system has advantages too. Primarily the Google Play variant is bad at:

* Search. I can't really search for specific crashes / stack traces.

* Linking bug tracker and crashes. I never know if a specific crash has been reported / fixed / deployed.

However it is pretty good at:

* (Some) Metrics. Because Google Play knows about all installations and app versions / release channels it can calculate crash rates and compare the quality of updates.

* It's opt-in/opt-out at the system level. We do not need to ask the user anything and it just works.


We have been thinking about switching to a different crash reporting solution in the past [1]. Back then the only options we came up with were either using our existing crash reporting service that we use in Fennec and create a library from that. Or use Sentry. Sentry is already in use by the iOS team and Mozilla has its own backend service for it. All other options had either data (third-party servers) or code (closed source) concerns.

Didn't Google buy Crashlytics/Fabric some time ago? I wonder if it is going to be fold into Firebase Crash Reporting. Unfortunately Firebase is using third-party services *and* is closed-source.

I wonder what happens if we do not do anything at all? Will a crash in GeckoView show up in Google Play? Will it contain enough information? Ideally I would like to avoid having to fiddle with crash reporting in the first release completely.

So far native crashes in WebView/Chrome crash our app and show up in Google Play as such. I'll attach a screenshot. In API 26 Google added a new API to get notified whenever the render process dies: https://developer.android.com/reference/android/webkit/WebViewClient.html#onRenderProcessGone(android.webkit.WebView,%20android.webkit.RenderProcessGoneDetail) - Is this something that we could/should mimic?

On a higher level I wonder what we want to achieve here:

* Does the Focus team collect (and own) all crashes and report them to the GeckoView team if needed?

* Or: Does the GeckoView team want to collect GeckoView crash reports independently from the application.

Comparing with WebView again: They collect diagnostics data independently on their own and we can opt-out with a manifest flag.

[1] https://github.com/mozilla-mobile/focus-android/issues/1410

Comment 5

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

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> However it is pretty good at:
> 
> * (Some) Metrics. Because Google Play knows about all installations and app
> versions / release channels it can calculate crash rates and compare the
> quality of updates.

Yeah, that's something crash-stats does some of too, but I think it would need some work to be able to handle Focus directly.

> 
> * It's opt-in/opt-out at the system level. We do not need to ask the user
> anything and it just works.

I didn't know you could opt-out, but it being opt-in by default (and silent) is a big advantage.

> 
> 
> We have been thinking about switching to a different crash reporting
> solution in the past [1]. Back then the only options we came up with were
> either using our existing crash reporting service that we use in Fennec and
> create a library from that. Or use Sentry. Sentry is already in use by the
> iOS team and Mozilla has its own backend service for it. All other options
> had either data (third-party servers) or code (closed source) concerns.
> 
> Didn't Google buy Crashlytics/Fabric some time ago? I wonder if it is going
> to be fold into Firebase Crash Reporting. Unfortunately Firebase is using
> third-party services *and* is closed-source.

Whoops, yes, I think the Firebase crash reporting is in fact Crashlytics. I haven't looked much at Sentry.

> 
> I wonder what happens if we do not do anything at all? Will a crash in
> GeckoView show up in Google Play? Will it contain enough information?
> Ideally I would like to avoid having to fiddle with crash reporting in the
> first release completely.

It should report to the Play Store as it does now, yes. The native stacks just won't be very usable.

> 
> So far native crashes in WebView/Chrome crash our app and show up in Google
> Play as such. I'll attach a screenshot. In API 26 Google added a new API to
> get notified whenever the render process dies:
> https://developer.android.com/reference/android/webkit/WebViewClient.
> html#onRenderProcessGone(android.webkit.WebView,%20android.webkit.
> RenderProcessGoneDetail) - Is this something that we could/should mimic?

We can and should do something similar, yes. I'll file a bug for that. We aren't using e10s at all right now, though (but there are plans).

> 
> On a higher level I wonder what we want to achieve here:
> 
> * Does the Focus team collect (and own) all crashes and report them to the
> GeckoView team if needed?
> 
> * Or: Does the GeckoView team want to collect GeckoView crash reports
> independently from the application.
> 
> Comparing with WebView again: They collect diagnostics data independently on
> their own and we can opt-out with a manifest flag.

I think the ideal scenario is that Gecko[View] crashes go to crash-stats, and the app can do whatever it wants (Play Store, Sentry, etc). This is easier when Gecko is in its own process, but we can probably get pretty close now.

Comment 6

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #5)
> > On a higher level I wonder what we want to achieve here:
> > 
> > * Does the Focus team collect (and own) all crashes and report them to the
> > GeckoView team if needed?
> > 
> > * Or: Does the GeckoView team want to collect GeckoView crash reports
> > independently from the application.
> > 
> > Comparing with WebView again: They collect diagnostics data independently on
> > their own and we can opt-out with a manifest flag.
> 
> I think the ideal scenario is that Gecko[View] crashes go to crash-stats,
> and the app can do whatever it wants (Play Store, Sentry, etc). This is
> easier when Gecko is in its own process, but we can probably get pretty
> close now.

I should add -- I think it would be very nice if the GeckoView and Focus teams don't have to triage each other's bugs.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

I looked at Sentry again and I think we want to use Sentry in Focus. Especially because we could start using it almost immediately.

Other than initially thought we want to see GeckoView-related crashes in our crash reporting too. Even if it is not actionable it is an important signal for us to make release decisions (e.g. if something crashes for a majority users then we need to know).

> I should add -- I think it would be very nice if the GeckoView and Focus teams don't have to triage each other's bugs.

That's true. However as we will need to have all crashes in Sentry you /could/ piggyback on that for now - if you want to.

Comment 8

[Gabriele Svelto [:gsvelto]]

Just my 2p in the hope it's a useful bit of info: I have recently added crash ping support to Fennec (bug 1407557) so you might want to look into that too if you're interested in crash rates. We tend to get a lot more crashes reported via pings than via Socorro so it might be a useful data source. Also those pings now contain the same data as desktop crash pings including raw crash stacks.

Comment 9

[Chris Peterson [:cpeterson]]

Sebastian filed a Klar GitHub issue [1] to track the Klar work to stand up Sentry crash reporting. Can Sentry handle crash reports for the Crow browser, too? Do we still need a Breakpad solution for GeckoView's native crashes?

[1] https://github.com/mozilla-mobile/focus-android/issues/2326

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Gabriele Svelto [:gsvelto] from comment #8)
> Just my 2p in the hope it's a useful bit of info: I have recently added
> crash ping support to Fennec (bug 1407557) so you might want to look into
> that too if you're interested in crash rates. We tend to get a lot more
> crashes reported via pings than via Socorro so it might be a useful data
> source. Also those pings now contain the same data as desktop crash pings
> including raw crash stacks.

Thank you. I'll filed an issue for that in the Focus repository:
https://github.com/mozilla-mobile/focus-android/issues/2348

Comment 11

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

This needs bug 1445420 since crash handling will be a GeckoSetting

Comment 12

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

Attachment: Bug 1433968 - Change how environment variables are passed to GeckoLoader

Right now we pass a bundle to GeckoLoader.setupGeckoEnvironment() with
magic keys representing the environment variables. Instead of this,
simply pass a list of Strings.

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

Comment 13

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

Attachment: Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

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

Comment 14

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

Attachment: Bug 1433968 - Add CrashReporterService for GeckView

The Fennec CrashReporter class is also renamed to
CrashReporterActivity. When running in Fennec, the Activity will be used
which retains what we do today, prompting for comments, email, etc. When
used in standalone GeckoView, we report the crash without user
interaction if the appropriate GeckoRuntimeSetting was set. The app will
want to ask for user permission at least once in order to set this.

We do not collect the URL, email, or logcat with GeckoView crashes.
Logcat and URL would be nice to have, but it's not clear what the API
for those would look like, and they can be addressed in followup
patches.

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

Comment 15

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

Comment on attachment 8970266 [details]
Bug 1433968 - Change how environment variables are passed to GeckoLoader

https://reviewboard.mozilla.org/r/239062/#review244710

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:384
(Diff revision 1)
>              }
>              return INSTANCE.mExtras;
>          }
>      }
>  
> +    private static ArrayList<String> getEnvFromExtras(Bundle extras) {

Return `List<String>`'; `final Bundle`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:387
(Diff revision 1)
>      }
>  
> +    private static ArrayList<String> getEnvFromExtras(Bundle extras) {
> +        ArrayList<String> result = new ArrayList<>();
> +        if (extras != null) {
> +            String env = extras.getString("env0");

We should still log the env vars, in debug mode at least.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:394
(Diff revision 1)
> +                result.add(env);
> +                env = extras.getString("env" + c);
> +            }
> +        }
> +
> +        return result;

Return null if `extras` is null

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:458
(Diff revision 1)
>  
>          if ((mFlags & FLAG_DEBUGGING) != 0) {
>              Log.i(LOGTAG, "RunGecko - args = " + TextUtils.join(" ", args));
>          }
>  
> -        GeckoLoader.setupGeckoEnvironment(context, context.getFilesDir().getPath(), mExtras);
> +        ArrayList<String> env = getEnvFromExtras(mExtras);

`final List<String>`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/GeckoLoader.java:97
(Diff revision 1)
>      }
>  
>      public synchronized static void setupGeckoEnvironment(final Context context,
>                                                            final String profilePath,
> -                                                          final Bundle extras) {
> -        // if we have an intent (we're being launched by an activity)
> +                                                          final Collection<String> env) {
> +        for (String e : env) {

`final String`

Comment 16

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

https://reviewboard.mozilla.org/r/239064/#review244712

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:179
(Diff revision 1)
>      };
>  
>      private static String sAppNotes;
> +    private static CrashHandler sCrashHandler;
>  
>      public static CrashHandler ensureCrashHandling() {

Need to synchronize this with `handleUncaughtException`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:462
(Diff revision 1)
> +            // condition), but currently we have no way of detecting if we are running in a
> +            // debug Gecko build.

You can check `BuildConfig.DEBUG_BUILD`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java:93
(Diff revision 1)
> +
> +        if (!settings.getNativeCrashReportingEnabled()) {
> +            flags |= GeckoThread.FLAG_DISABLE_CRASHREPORTER;
> +        }
> +
> +        if (settings.getJavaCrashReportingEnabled()) {

This needs to be propagated to the content process as well.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:267
(Diff revision 1)
>      }
>  
> +    /**
> +     * Get whether native crash reporting is enabled or not.
> +     *
> +     * @return Whether native crash reporting is enabled or not.

`@return True if native crash reporting is enabled`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:269
(Diff revision 1)
> +    /**
> +     * Get whether native crash reporting is enabled or not.
> +     *
> +     * @return Whether native crash reporting is enabled or not.
> +     */
> +    public boolean getNativeCrashReportingEnabled() { return mNativeCrashReporting; }

Multiple lines

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:274
(Diff revision 1)
> +    public boolean getNativeCrashReportingEnabled() { return mNativeCrashReporting; }
> +
> +    /**
> +     * Get whether Java crash reporting is enabled or not.
> +     *
> +     * @return Whether Java crash reporting is enabled or not.

`@return True if Java crash reporting is enabled`

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:276
(Diff revision 1)
> +    /**
> +     * Get whether Java crash reporting is enabled or not.
> +     *
> +     * @return Whether Java crash reporting is enabled or not.
> +     */
> +    public boolean getJavaCrashReportingEnabled() { return mJavaCrashReporting; }

Multiple lines

Comment 17

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

Comment on attachment 8970268 [details]
Bug 1433968 - Add CrashReporterService for GeckView

https://reviewboard.mozilla.org/r/239066/#review244720

::: mobile/android/base/java/org/mozilla/gecko/CrashReporterActivity.java:1
(Diff revision 1)
> +/* -*- Mode: Java; tab-width: 20; indent-tabs-mode: nil; -*-

Mercurial is not seeing this as a file move.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterService.java:36
(Diff revision 1)
> +import java.util.Enumeration;
> +import java.util.HashMap;
> +import java.util.Map;
> +import java.util.zip.GZIPOutputStream;
> +
> +public class CrashReporterService extends IntentService {

`CrashReporterActivity` should share code with `CrashReporterService`. There's a lot of duplicated code here.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterService.java:69
(Diff revision 1)
> +        }
> +
> +        Class<?> reporterActivityCls = getFennecReporterActivity();
> +        if (reporterActivityCls != null) {
> +            intent.setClass(this, reporterActivityCls);
> +            startActivity(intent);

You probably want `Intent.FLAG_ACTIVITY_NEW_TASK` here

Comment 18

[Eugen Sawin [:esawin]]

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

https://reviewboard.mozilla.org/r/239064/#review244934

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:733
(Diff revision 1)
>  
>          if (sRuntime == null) {
>              final GeckoRuntimeSettings.Builder runtimeSettingsBuilder =
>                  new GeckoRuntimeSettings.Builder();
> -            runtimeSettingsBuilder.arguments(new String[] { "-purgecaches" });
> +            runtimeSettingsBuilder
> +                    .arguments(new String[] { "-purgecaches" })

Indentation

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:102
(Diff revision 1)
> +         * a SEGFAULT handler to be installed, and any crash encountered there will be
> +         * reported to Mozilla. Additionally, an unhandled exception
> +         * will be triggered to allow the application to do additional reporting.
> +         *
> +         * @param enabled A flag determining whether native crash reporting should be enabled.
> +         *                Defaults to false.

We should probably note the defaults for the other settings, too.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:102
(Diff revision 1)
> +         * a SEGFAULT handler to be installed, and any crash encountered there will be
> +         * reported to Mozilla. Additionally, an unhandled exception
> +         * will be triggered to allow the application to do additional reporting.
> +         *
> +         * @param enabled A flag determining whether native crash reporting should be enabled.
> +         *                Defaults to false.

Missing @return.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:116
(Diff revision 1)
> +         * a default unhandled exception handler to be installed, and any exceptions encountered
> +         * will automatically reported to Mozilla.
> +         *
> +         * @param enabled A flag determining whether Java crash reporting should be enabled.
> +         *                Defaults to false.
> +         * @return

Missing description.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:71
(Diff revision 1)
>              final Bundle extras = getIntent().getExtras();
>              if (extras != null) {
>                  runtimeSettingsBuilder.extras(extras);
>              }
>              runtimeSettingsBuilder.useContentProcessHint(useMultiprocess);
> +            runtimeSettingsBuilder.nativeCrashReportingEnabled(true);

The builder supports fluent API.

Comment 19

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

Comment on attachment 8970266 [details]
Bug 1433968 - Change how environment variables are passed to GeckoLoader

https://reviewboard.mozilla.org/r/239062/#review245040

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:394
(Diff revision 1)
> +                result.add(env);
> +                env = extras.getString("env" + c);
> +            }
> +        }
> +
> +        return result;

I'll return an empty list instead of null, that way it's less of a hassle to append the crash reporter env var.

Comment 20

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

https://reviewboard.mozilla.org/r/239064/#review245042

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:733
(Diff revision 1)
>  
>          if (sRuntime == null) {
>              final GeckoRuntimeSettings.Builder runtimeSettingsBuilder =
>                  new GeckoRuntimeSettings.Builder();
> -            runtimeSettingsBuilder.arguments(new String[] { "-purgecaches" });
> +            runtimeSettingsBuilder
> +                    .arguments(new String[] { "-purgecaches" })

This is what Android Studio wants to do. Do you expect 4 spaces here?

Comment 21

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

Comment on attachment 8970266 [details]
Bug 1433968 - Change how environment variables are passed to GeckoLoader

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

Comment 22

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

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

Comment 23

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

Comment on attachment 8970268 [details]
Bug 1433968 - Add CrashReporterService for GeckView

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

Comment 24

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

Comment on attachment 8970266 [details]
Bug 1433968 - Change how environment variables are passed to GeckoLoader

https://reviewboard.mozilla.org/r/239062/#review245398

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:386
(Diff revision 2)
>              }
>              return INSTANCE.mExtras;
>          }
>      }
>  
> +    private static ArrayList<String> getEnvFromExtras(final Bundle extras) {

return `List<String>`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:392
(Diff revision 2)
> +        if (extras == null) {
> +            return new ArrayList<>();
> +        }
> +
> +        ArrayList<String> result = new ArrayList<>();
> +        if (extras != null) {

Already checked that

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:395
(Diff revision 2)
> +
> +        ArrayList<String> result = new ArrayList<>();
> +        if (extras != null) {
> +            String env = extras.getString("env0");
> +            for (int c = 1; env != null; c++) {
> +                if (BuildConfig.DEBUG) {

Should be `(mFlags & FLAG_DEBUGGING) != 0` instead of `BuildConfig.DEBUG`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:467
(Diff revision 2)
>  
>          if ((mFlags & FLAG_DEBUGGING) != 0) {
>              Log.i(LOGTAG, "RunGecko - args = " + TextUtils.join(" ", args));
>          }
>  
> -        GeckoLoader.setupGeckoEnvironment(context, context.getFilesDir().getPath(), mExtras);
> +        final ArrayList<String> env = getEnvFromExtras(mExtras);

`final List<String>`

Comment 25

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

https://reviewboard.mozilla.org/r/239064/#review245400

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:180
(Diff revision 2)
>  
>      private static String sAppNotes;
> +    private static CrashHandler sCrashHandler;
>  
>      public static CrashHandler ensureCrashHandling() {
> -        // Crash handling is automatically enabled when GeckoAppShell is loaded.
> +        synchronized (GeckoAppShell.class) {

Just make the method `synchronized`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:190
(Diff revision 2)
> +
> +        return sCrashHandler;
> +    }
> +
> +    public static boolean isCrashHandlingEnabled() {
> +        synchronized (GeckoAppShell.class) {

Same

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:296
(Diff revision 2)
>          return CrashHandler.getExceptionStackTrace(CrashHandler.getRootException(e));
>      }
>  
>      @WrapForJNI(exceptionMode = "ignore")
>      private static void handleUncaughtException(Throwable e) {
> -        CRASH_HANDLER.uncaughtException(null, e);
> +        synchronized (GeckoAppShell.class) {

Same

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:382
(Diff revision 2)
>      public static @Nullable Bundle getActiveExtras() {
>          synchronized (INSTANCE) {
>              if (!INSTANCE.mInitialized) {
>                  return null;
>              }
> -            return INSTANCE.mExtras;
> +            return new Bundle(INSTANCE.mExtras);

Why not make `EXTRA_JAVA_CRASH_HANDLING` an extra in  the parent process as well, so we don't need this?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoServiceChildProcess.java:47
(Diff revision 2)
>  
>      @Override
>      public void onCreate() {
>          super.onCreate();
>  
>          GeckoAppShell.ensureCrashHandling();

Need to remove this line

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:104
(Diff revision 2)
> +         * reported to Mozilla. Additionally, an unhandled exception
> +         * will be triggered to allow the application to do additional reporting.

Which exception?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:295
(Diff revision 2)
>      public int describeContents() {
>          return 0;
>      }
>  
>      @Override // Parcelable
>      public void writeToParcel(Parcel out, int flags) {

Need to change `writeToParcel` and `readFromParcel` as well

Comment 26

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

https://reviewboard.mozilla.org/r/239064/#review245400

> Why not make `EXTRA_JAVA_CRASH_HANDLING` an extra in  the parent process as well, so we don't need this?

You mean set it in GeckoRuntime? I think it's weird to use that as an internal implementation detail.

> Which exception?

Oops, this was an optimistic comment :)

I'll remove it for now, but I think we probably need to add this if possible. We could also look for the CRASH file in the profile directory on the next start and expose that somehow.

> Need to change `writeToParcel` and `readFromParcel` as well

Weird, I know I wrote this earlier. Must've dropped it in a rebase.

Comment 27

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

https://reviewboard.mozilla.org/r/239064/#review245400

> You mean set it in GeckoRuntime? I think it's weird to use that as an internal implementation detail.

You can set it in GeckoThread, next to the other internal extras that we set.

Comment 28

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

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

Comment 29

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

Comment on attachment 8970268 [details]
Bug 1433968 - Add CrashReporterService for GeckView

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

Comment 30

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

https://reviewboard.mozilla.org/r/239064/#review244934

> Indentation

This is what Studio wants to do. Do you expect 4 spaces or something?

Comment 31

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

Would you both take another look at this one? I changed some things around.

Comment 32

[Eugen Sawin [:esawin]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #20)
> >  
> >          if (sRuntime == null) {
> >              final GeckoRuntimeSettings.Builder runtimeSettingsBuilder =
> >                  new GeckoRuntimeSettings.Builder();
> > -            runtimeSettingsBuilder.arguments(new String[] { "-purgecaches" });
> > +            runtimeSettingsBuilder
> > +                    .arguments(new String[] { "-purgecaches" })
> 
> This is what Android Studio wants to do. Do you expect 4 spaces here?

Yeah, 4 spaces. I don't see why this should be a special case.

Comment 33

[Eugen Sawin [:esawin]]

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

https://reviewboard.mozilla.org/r/239064/#review245904

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:293
(Diff revision 3)
>      @Override // Parcelable
>      public int describeContents() {
>          return 0;
>      }
>  
> +    private static void writeBoolean(Parcel out, boolean val) {

Can we move this to a utility class, it's useful across the codebase.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:297
(Diff revision 3)
>  
> +    private static void writeBoolean(Parcel out, boolean val) {
> +        out.writeByte((byte) (val ? 1 : 0));
> +    }
> +
> +    private static boolean readBoolean(Parcel source) {

Same.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:72
(Diff revision 3)
>              if (extras != null) {
>                  runtimeSettingsBuilder.extras(extras);
>              }
> -            runtimeSettingsBuilder.useContentProcessHint(useMultiprocess);
> +
> +            runtimeSettingsBuilder
> +                    .useContentProcessHint(useMultiprocess)

Indentation.

Comment 34

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

Apparently you can change the indentation for this case to use 4 in the Studio settings. It's labeled "Continuation Indentation" in the Java prefs.

Comment 35

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

(In reply to Eugen Sawin [:esawin] from comment #32)
> > 
> > This is what Android Studio wants to do. Do you expect 4 spaces here?
> 
> Yeah, 4 spaces. I don't see why this should be a special case.

The Android style guide calls for 8 spaces for line wraps [1]. I assume that's what Studio is following.

[1] https://source.android.com/setup/contribute/code-style#use-spaces-for-indentation

Comment 36

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

https://reviewboard.mozilla.org/r/239064/#review245940

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java:487
(Diff revision 3)
> +        // 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) {
> +            env.add(0, "MOZ_CRASHREPORTER_DISABLE=1");
> +        } else if ((mFlags & FLAG_ENABLE_NATIVE_CRASHREPORTER) != 0 && BuildConfig.DEBUG_BUILD) {
> +            env.add(0, "MOZ_CRASHREPORTER_ENABLE=1");

Is this implemented anywhere?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoProcessManager.java:205
(Diff revision 3)
>  
> +        final int flags = GeckoThread.getActiveFlags();
> +
>          boolean started = false;
>          try {
> -            started = child.start(this, args, extras, prefsPfd, ipcPfd, crashPfd,
> +            started = child.start(this, args, extras, flags, prefsPfd, ipcPfd, crashPfd,

Only the `FLAG_ENABLE_JAVA_CRASHREPORTER` flag really makes sense for the child. In particular `FLAG_PRELOAD_CHILD` does not make sense for the child.

::: widget/android/nsAppShell.cpp:403
(Diff revision 3)
> +            GeckoAppShellSupport::Init();
> +            GeckoThreadSupport::Init();

Why `GeckoThreadSupport::Init()`?

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82b765c5a02f
Change how environment variables are passed to GeckoLoader r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/73ec9bb3e17e
Add crash reporting control to GeckoRuntimeSettings r=esawin,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/0812ac0376da
Add CrashReporterService for GeckView r=jchen

Comment 38

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/82b765c5a02f
https://hg.mozilla.org/mozilla-central/rev/73ec9bb3e17e
https://hg.mozilla.org/mozilla-central/rev/0812ac0376da

Comment 39

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/93443d36d4bd
Backed out 3 changesets for causing Bug 1459349. a=backout

Comment 40

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

Attachment: Bug 1433968 - Add support for specifying profile to GeckoRuntime

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

Comment 41

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

Attachment: Bug 1433968 - Support pause-for-debugger in GeckoRuntime

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

Comment 42

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

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

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

Comment 43

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

Comment on attachment 8970266 [details]
Bug 1433968 - Change how environment variables are passed to GeckoLoader

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

Comment 44

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

Comment on attachment 8970267 [details]
Bug 1433968 - Add crash reporting control to GeckoRuntimeSettings

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

Comment 45

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

Comment on attachment 8970268 [details]
Bug 1433968 - Add CrashReporterService for GeckView

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

Comment 46

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

Comment on attachment 8974424 [details]
Bug 1433968 - Support pause-for-debugger in GeckoRuntime

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

Comment 47

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

Comment on attachment 8974425 [details]
Bug 1433968 - Use GeckoRuntime to launch Gecko in Fennec

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

Comment 48

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

Comment on attachment 8974424 [details]
Bug 1433968 - Support pause-for-debugger in GeckoRuntime

https://reviewboard.mozilla.org/r/242760/#review248716

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:151
(Diff revision 2)
> +         *
> +         * @param enabled A flag determining whether there will be a pause early in startup.
> +         *                Defaults to false.
> +         * @return This Builder.
> +         */
> +        public @NonNull Builder debugPause(boolean enabled) {

Maybe `pauseForDebugger`?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:352
(Diff revision 2)
> +    /**
> +     * Gets whether the debug pause is enabled or not.
> +     *
> +     * @return True if the debug pause is enabled.
> +     */
> +    public boolean getDebugPauseEnabled() { return mDebugPause; }

Same, `getPauseForDebugger`?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:360
(Diff revision 2)
>      public int describeContents() {
>          return 0;
>      }
>  
>      @Override // Parcelable
>      public void writeToParcel(Parcel out, int flags) {

Need to change `writeToParcel` and `readFromParcel`

Comment 49

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

Comment on attachment 8974425 [details]
Bug 1433968 - Use GeckoRuntime to launch Gecko in Fennec

https://reviewboard.mozilla.org/r/242762/#review248720

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:59
(Diff revision 2)
> +import org.mozilla.geckoview.GeckoRuntime;
> +import org.mozilla.geckoview.GeckoRuntimeSettings;



::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:73
(Diff revision 2)
>  import java.util.UUID;
>  
>  public class GeckoApplication extends Application
>                                implements HapticFeedbackDelegate {
>      private static final String LOG_TAG = "GeckoApplication";
> +    public static final String ACTION_DEBUG = "org.mozilla.gecko.DEBUG";

private

::: mobile/android/base/java/org/mozilla/gecko/GeckoService.java:173
(Diff revision 2)
>          }
>  
> -        if (!GeckoThread.initMainProcessWithProfile(
> -                profileName, profileDir != null ? new File(profileDir) : null,
> -                GeckoApplication.getDefaultGeckoArgs(), intent.getExtras())) {
> -            Log.w(LOGTAG, "Ignoring due to profile mismatch: " +
> +        if (GeckoApplication.getRuntime() != null) {
> +            // Gecko has already been initialized, make sure it's using the
> +            // expected profile.
> +            return GeckoThread.canUseProfile(profileName,

We should probably keep the warning

::: mobile/android/base/java/org/mozilla/gecko/GeckoService.java:181
(Diff revision 2)
> -            final GeckoProfile profile = GeckoThread.getActiveProfile();
> -            if (profile != null) {
> -                Log.w(LOGTAG, "Current profile is " + profile.getName() +
> -                              " [" + profile.getDir().getAbsolutePath() + ']');
> -            }
> -            return false;
> +
> +        String args;
> +        if (profileDir != null) {
> +            args = "-profile " + profileDir;
> +        } else {
> +            args = "-P " + profileName;

We can have both `profileDir` and `profileName`, in which case we need to specify both `-profile` and `-P`. So the two should be separate.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed1e3af37ba
Change how environment variables are passed to GeckoLoader r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/62f865eed952
Add crash reporting control to GeckoRuntimeSettings r=esawin,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/64cccb490a2a
Add CrashReporterService for GeckView r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/c94c48d76350
Support pause-for-debugger in GeckoRuntime r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8616427aea
Use GeckoRuntime to launch Gecko in Fennec r=jchen

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bba1deb626b5
Fix a typo in GeckoRuntimeSettings r=me

Comment 52

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7ffd532e0a
Backed out 6 changesets for Android failures on GeckoRuntimeSettings on a CLOSED TREE

Comment 53

[Tiberius Oros[:tiberius_oros]]

Backed out 6 changesets (bug 1433968) for Android failures on GeckoRuntimeSettings
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7ffd532e0ac7d7fc52a52f80a2e1b839a7f915

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7a8616427aea19089fffd9423577751deae186e2&selectedJob=177864501

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7a8616427aea19089fffd9423577751deae186e2
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bba1deb626b53d9c044a30bdfce49a15f5c00780

Comment 54

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

Ugh. Try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe1e842585d946c316ef55296b42f6521e8242c5

Comment 55

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

One more time? https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ac6ca8fb57a50b84feacf36b5446d2671d0f485

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efbc296d239f
Change how environment variables are passed to GeckoLoader r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/f934403b4022
Add crash reporting control to GeckoRuntimeSettings r=esawin,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac2124bfed0
Add CrashReporterService for GeckView r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/8524ebbac7f0
Support pause-for-debugger in GeckoRuntime r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e0813df0f65
Use GeckoRuntime to launch Gecko in Fennec r=jchen

Comment 57

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/efbc296d239f
https://hg.mozilla.org/mozilla-central/rev/f934403b4022
https://hg.mozilla.org/mozilla-central/rev/3ac2124bfed0
https://hg.mozilla.org/mozilla-central/rev/8524ebbac7f0
https://hg.mozilla.org/mozilla-central/rev/0e0813df0f65