Closed Bug 1467461 Opened Last year Closed Last year

Migrate CrashReportingService to JobIntentService

Categories

(Firefox for Android :: General, enhancement)

All
Android
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: petru, Assigned: petru)

References

Details

Attachments

(1 file, 1 obsolete file)

This is related to the work from bug 1407046

Because of the new background limitations imposed by Oreo we should avoid using services to do background work.

CrashReportingService [1] is one such Service which can easily be migrated to JobIntentService.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterService.java
The uploaded patch should resolve the issue (in a perfect world).
Unfortunately, the command used to previously start our service [1],
> /system/bin/am startservice
cannot start a JobIntentService.
To get past this I've created a new Receiver with it's sole purpose being to start the newly migrated to JobIntentService.
The issue with this approach is that apparently a broadcast cannot be started on Oreo anymore from "/system/bin/am"
I tried starting the broadcast using adb shell commands from terminal and it worked.
I suspect that is because adb shell shell ran as root while our app doesn't.

I've investigated this and found that others have also experienced issues with "/system/bin/am" [2] and followed what is actually happening when trying to start a broadcast, action which ultimately fails with "Calling application did not provide package name" [3] as result of a recent Android API change. Same thing happens to us.

An ugly but quick solution to this would be to keep am starting a service, another new small service which should just start the new JobIntentService, similar to what the BroadcastReceiver does. Because the code to start the service is very small there would be no time for Android to kill this service (as part of the background limitations policy), but a service to start a service is not ideal.

Another possible approach would involve refactoring nsExceptionHandler.LaunchCrashReporterActivity [4] to directly delegate a Java method to start the JobIntentService.

Because I don't have experience with this part of cpp code and I don't know why we went with this route in the first place (use /system/bin/am) or how easy would be to refactor this method I'm NI'ing James for directions regarding what he thinks the best approach would be.


[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#843
[2] https://github.com/termux/termux-api/issues/105#issuecomment-362651583
[3] https://pastebin.com/Las0Cv7e
[4] https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#831
Flags: needinfo?(snorp)
Flags: needinfo?(sdaswani)
Is this actually a problem in practice? A crash report is not going to take long at all to send, certainly not minutes (unless the connection is very bad).

The reason we use 'am start' is because we are launching this from a compromised (crashed) process. In many cases, the Dalvik VM is in an unusable state.

I think I'm ok with a stub service if we feel strongly about making CrashReporterService into a JobIntentService.
Flags: needinfo?(snorp)
Thanks!

The documentation is not entirely clear but as per https://developer.android.com/about/versions/oreo/background
> IntentService is a service, and is therefore subject to the new restrictions on background services. 
> As a result, many apps that rely on IntentService do not work properly when targeting Android 8.0 or higher. 
> For this reason, Android Support Library 26.0.0 introduces a new JobIntentService class, which provides the same
> functionality as IntentService but uses jobs instead of services when running on Android 8.0 or higher.  
> ...
> After the system has created the service, the app has five seconds to call the service's startForeground() method 
> to show the new service's user-visible notification. If the app does not call startForeground() within the time limit, 
> the system stops the service and declares the app to be ANR.

I think we need to go with the migration to JobIntentService and if it's ok with you I'll use that stub service in place of the broadcast receiver.
(In reply to Petru-Mugurel Lingurar[:petru] from comment #4)
> Thanks!
> 
> The documentation is not entirely clear but as per
> https://developer.android.com/about/versions/oreo/background
> > IntentService is a service, and is therefore subject to the new restrictions on background services. 
> > As a result, many apps that rely on IntentService do not work properly when targeting Android 8.0 or higher. 
> > For this reason, Android Support Library 26.0.0 introduces a new JobIntentService class, which provides the same
> > functionality as IntentService but uses jobs instead of services when running on Android 8.0 or higher.  
> > ...
> > After the system has created the service, the app has five seconds to call the service's startForeground() method 
> > to show the new service's user-visible notification. If the app does not call startForeground() within the time limit, 
> > the system stops the service and declares the app to be ANR.
> 
> I think we need to go with the migration to JobIntentService and if it's ok
> with you I'll use that stub service in place of the broadcast receiver.

Ah, ok. Interesting. I think that's going to affect how we use e10s with GeckoView too. Also from that page:

"With Android 8.0, there is a complication; the system doesn't allow a background app to create a background service."

That seems to imply that you *must* use startForeground() to start a service from a background app. It looks like we can use 'am start-foreground-service' for this for the crash reporter.
Awesome find!
That is what I was worried about also, although in my previous tests on Oreo the crash reporting service was successfully started using the simple am startservice (before any changes in the cpp).
(After starting to use support library 26 the old play services would throw an error about a missing class when forcing a fx profile sync so atm, for testing, I have a reliable way to start the crash handling process).

Will use 'am start-foreground-service' to be on the safe side and test with that, thanks!
Assignee: nobody → petru.lingurar
I think I have the info needed, clearing the NI from sdaswani
Flags: needinfo?(sdaswani)
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review256562

This is looking pretty good, just a few rough edges to clean up.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterStarterService.java:34
(Diff revision 2)
> + * (We should have started {@link CrashReporterService} a long time before that anyway.)
> + */
> +public class CrashReporterStarterService extends Service {
> +    @Override
> +    public int onStartCommand(Intent intent, int flags, int startId) {
> +        final Class<?> reporterServiceClass = getFennecReporterService();

We'll always have this, no need to use reflection, etc. The thing we do for CrashReporterActivity is because we don't have that in GeckoView.

::: mobile/android/services/src/main/java/org/mozilla/gecko/background/JobIdsConstants.java:1
(Diff revision 2)
> +package org.mozilla.gecko.background;

AFAICT nothing from this file is used in the patch. Leakage from something else?

::: toolkit/crashreporter/nsExceptionHandler.cpp:843
(Diff revision 2)
>    else if (pid == 0) {
>      // Invoke the reportCrash activity using am
>      if (androidUserSerial) {
>        Unused << execlp("/system/bin/am",
>                         "/system/bin/am",
> -                       "startservice",
> +                       "start-foreground-service",

We can't call this unconditionally, because older devices won't have start-foreground-service. You need to make sure we only use this on Android O and higher, and fallback to startservice (or start-service?) otherwise.

::: toolkit/crashreporter/nsExceptionHandler.cpp:853
(Diff revision 2)
>                         "--ez", "minidumpSuccess", aSucceeded ? "true" : "false",
>                         (char*)0);
>      } else {
>        Unused << execlp("/system/bin/am",
>                         "/system/bin/am",
> -                       "startservice",
> +                       "start-foreground-service",

Same as above

::: toolkit/crashreporter/nsExceptionHandler.cpp:1541
(Diff revision 2)
>  #ifdef XP_MACOSX
>    libraryPath = ToNewCString(libraryPath_temp);
>  #endif
>  #endif // XP_WIN32
>  #else
>    // On Android, we launch using the application package name instead of a

Somewhere in this block may be a good place to do the SDK version detection.
Attachment #8984140 - Flags: review-
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review256772

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterStarterService.java:28
(Diff revision 2)
> + * </ul>
> + *
> + * This service is started with <em>am start-foreground-service</em> which let's us start
> + * as a background service.<br>
> + * We do not though call <a href="http://dev.w3.org/html5/webvtt">startForeground(..)</a> immediately
> + * and accept that after the 5 seconds the system would automatically stop us.<br>

To confirm, does this really work in practice on Android O (with the target API set to 26)?

While normal background services (that were started while the app was still in foreground) will indeed merely be stopped, ignoring the "call startForeground" requirement on a service started through startForegroundService leads to either an ANR (if the service was still running), or a crash (if the service was already stopped again before the timeout expired).
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #5)
> Ah, ok. Interesting. I think that's going to affect how we use e10s with
> GeckoView too. Also from that page:
> 
> "With Android 8.0, there is a complication; the system doesn't allow a
> background app to create a background service."
> 
> That seems to imply that you *must* use startForeground() to start a service
> from a background app. It looks like we can use 'am
> start-foreground-service' for this for the crash reporter.

(In reply to Petru-Mugurel Lingurar[:petru] from comment #6)
> Awesome find!
> That is what I was worried about also, although in my previous tests on Oreo
> the crash reporting service was successfully started using the simple am
> startservice (before any changes in the cpp).

I'm not sure where (if at all) that's formally documented, but it could be that after the app is in the background there's still a short grace period where the background restrictions not yet apply. Also, you did test this with the target API set to 26, didn't you?
Attachment #8984140 - Flags: review?(sdaswani)
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review256562

> AFAICT nothing from this file is used in the patch. Leakage from something else?

The last int in this file would be used as the jobId for our JobIntentService.
Each JobIntentService should have a unique id [1] and so I've created that file to centralize them all and help avoid duplicate ids.
One issue though is that this file (JobIdsConstants) is not accessble to classes from geckoview. I've decided to still keep the id here to ensure ids unicity across the app but made it private and added comments to try and explain why I've chosen this route.

> We can't call this unconditionally, because older devices won't have start-foreground-service. You need to make sure we only use this on Android O and higher, and fallback to startservice (or start-service?) otherwise.

Right. Thanks!
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review256772

> To confirm, does this really work in practice on Android O (with the target API set to 26)?
> 
> While normal background services (that were started while the app was still in foreground) will indeed merely be stopped, ignoring the "call startForeground" requirement on a service started through startForegroundService leads to either an ANR (if the service was still running), or a crash (if the service was already stopped again before the timeout expired).

Tested this with `android_target_sdk=26`
Indeed, if running on a 26 device, after those 5 seconds I see that the service is destroyed and immediately after
`E/ActivityManager: ANR in org.mozilla.fennec_petrulingurar:crashreporter`
`PID: 14850`
`Reason: Context.startForegroundService() did not then call Service.startForeground()`
but the crash reporting process continue to work normally - we are already in another service / activity.

The only solution to avoid this would be to post a notification but this would change the overall UX.
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review256772

> Tested this with `android_target_sdk=26`
> Indeed, if running on a 26 device, after those 5 seconds I see that the service is destroyed and immediately after
> `E/ActivityManager: ANR in org.mozilla.fennec_petrulingurar:crashreporter`
> `PID: 14850`
> `Reason: Context.startForegroundService() did not then call Service.startForeground()`
> but the crash reporting process continue to work normally - we are already in another service / activity.
> 
> The only solution to avoid this would be to post a notification but this would change the overall UX.

If it works in practice and remains invisible to the user I guess it's okay, but just to keep in mind that we need to take extra care around this.
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review256898

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterService.java:82
(Diff revision 3)
> +        // If JobScheduler stopped execution before work being completed it should not be restarted.
> +        return false;
> +    }
> +
> +    static void enqueueWork(@NonNull final Context context, @NonNull final Intent intent) {
> +        final int jobId = -14;  // taken from org.mozilla.gecko.background.JobIdsConstants

The same issue I raised in bug 1407046 applies: Since jobIds need to be unique per UID and unfortunately Beta and Release share a common UID, we cannot use the same jobIds for both - one of them needs an additional offset added/subtracted through the build config.
Right, thanks Jan!
Also you can you please sort out your commits again and merge everything back into the one with MozReview-Commit-ID: GATl6Waa9St please?
Attachment #8984818 - Attachment is obsolete: true
Attachment #8984818 - Flags: review?(snorp)
Attachment #8984140 - Flags: review?(snorp) → review?(nchen)
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review258452

Looks good overall; just a few comments.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashHandler.java:310
(Diff revision 4)
>              if (Build.VERSION.SDK_INT < 17) {
>                  pb = new ProcessBuilder(
>                      "/system/bin/am", "start",
>                      "-a", action,
>                      "-n", pkg + '/' + component,
>                      "--es", "minidumpPath", dumpFile);
>              } else {
>                  pb = new ProcessBuilder(
>                      "/system/bin/am", "start",
>                      "--user", /* USER_CURRENT_OR_SELF */ "-3",
>                      "-a", action,
>                      "-n", pkg + '/' + component,
>                      "--es", "minidumpPath", dumpFile);
>              }

You should update this block as well.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterService.java:85
(Diff revision 4)
> +    }
> +
> +    static void enqueueWork(@NonNull final Context context, @NonNull final Intent intent) {
> +        int jobId;
> +
> +        final boolean isReleaseBuild = BuildConfig.RELEASE_OR_BETA;

Why do we need different ID for Beta?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterStarterService.java:21
(Diff revision 4)
> + *     </li>
> + *     <li>
> + *         <em>/system/bin/am</em> cannot start a JobIntentService
> + *     </li>
> + *     <li>A recent bug in Android prevents us from sending a broadcast from <em>am</em>.
> + *          See <a href="http://dev.w3.org/html5/webvtt">call stack</a>

Wrong link?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterStarterService.java:27
(Diff revision 4)
> + *     </li>
> + * </ul>
> + *
> + * This service is started with <em>am start-foreground-service</em> which let's us start
> + * as a background service.<br>
> + * We do not though call <a href="http://dev.w3.org/html5/webvtt">startForeground(..)</a> immediately

Wrong link?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterStarterService.java:31
(Diff revision 4)
> + * as a background service.<br>
> + * We do not though call <a href="http://dev.w3.org/html5/webvtt">startForeground(..)</a> immediately
> + * and accept that after the 5 seconds the system would automatically stop us.<br>
> + * (We should have started {@link CrashReporterService} a long time before that anyway.)
> + */
> +public class CrashReporterStarterService extends Service {

Can we combine `CrashReporterStarterService` into `CrashReporterService`? Seems like you can just override `onStartCommand` inside `CrashReporterService`

::: mobile/android/services/src/main/java/org/mozilla/gecko/background/JobIdsConstants.java:7
(Diff revision 4)
> +
> +/**
> + * <p>
> + * Repository for all the IDs used to differentiate between different Android Jobs.<br>
> + * They should be unique across both the app’s own code and the code from any library that the app uses.<br>
> + * To try and mitigate this situations we will use IDs with negative values in our code.

I'm not sure we can be certain libraries won't have negative job IDs? I would just use normal IDs. For example, Android examples use IDs that start with 1000.

::: mobile/android/services/src/main/java/org/mozilla/gecko/background/JobIdsConstants.java:16
(Diff revision 4)
> + * differentiate between the Job Ids of the two apps to avoid any potential problems.
> + *
> + * @see <a href="https://developer.android.com/reference/android/app/job/JobInfo.Builder#JobInfo.Builder(int, android.content.ComponentName)">
> + *     JobId importance</a>
> + */
> +public class JobIdsConstants {

Why put this under "mobile/android/services/src/main/java/org/mozilla/gecko/background"?

::: toolkit/crashreporter/nsExceptionHandler.cpp:226
(Diff revision 4)
>  // explicitly pass it to am
>  static char* androidUserSerial = nullptr;
> +
> +// Before Android 8 we needed to use "startservice" to start the crash reporting service.
> +// After Android 8 we need to use "start-foreground-service"
> +static char* startServiceCommand = nullptr;

`static const char* androidStartServiceCommand`

::: toolkit/crashreporter/nsExceptionHandler.cpp:1558
(Diff revision 4)
>    } else {
> -    nsCString package(ANDROID_PACKAGE_NAME "/org.mozilla.gecko.CrashReporterService");
> +    nsCString package(ANDROID_PACKAGE_NAME "/org.mozilla.gecko.CrashReporterStarterService");
>      crashReporterPath = ToNewCString(package);
>    }
> +
> +  const int deviceSdkVersion = std::stoi(getenv("MOZ_ANDROID_DEVICE_SDK_VERSION"));

Use `PR_GetEnv` and make sure it's not null, and then use `atol`.
Attachment #8984140 - Flags: review?(nchen) → review-
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review258452

> You should update this block as well.

Updated the command to start the service and also the component to be the newly created stub service. Thanks!

> Why do we need different ID for Beta?

Firefox Release and Firefox Beta use the same UID (see bug 1437871).
We need different Job Ids to differentiate between the same Jobs running in different applications.

> Can we combine `CrashReporterStarterService` into `CrashReporterService`? Seems like you can just override `onStartCommand` inside `CrashReporterService`

Previous implementation only had `CrashReportingService`.
As per https://developer.android.com/about/versions/oreo/background
> After the system has created the service, the app has five seconds to call the service's startForeground() method 
> to show the new service's user-visible notification. If the app does not call startForeground() within the time limit, 
> the system stops the service and declares the app to be ANR.

This patch is trying to circumvent this limitation while also avoiding posting a user visible notification by using a JobIntentService (which can run in background and doesn't need to post a notification).
The issue with it is though that is cannot be started with a shell command.
A broadcast also cannot be used to start the JobIntentService because of https://pastebin.com/Las0Cv7e so we were left with using a stub service to start the JobIntentService responsable for the background work.

> I'm not sure we can be certain libraries won't have negative job IDs? I would just use normal IDs. For example, Android examples use IDs that start with 1000.

I'm also not sure we can check all the JobIds used by all project's libraries.
I think using negative Ids offers a good chance of avoiding potential clashes while also allowing a wide range of available Ids to use. For example I'm using an -1000 offset to differentiate between the Ids used by the Release and Beta versions of our app.

> Why put this under "mobile/android/services/src/main/java/org/mozilla/gecko/background"?

This class is ment to be a repository of all the IDs used by our JobIntentServices which helps to guarantee their unicity.
I've put it in `background` because it refers to background work.
It is easily accessible by most of the JobIntentServices used. Except `CrashReportingService`.
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review258452

> Previous implementation only had `CrashReportingService`.
> As per https://developer.android.com/about/versions/oreo/background
> > After the system has created the service, the app has five seconds to call the service's startForeground() method 
> > to show the new service's user-visible notification. If the app does not call startForeground() within the time limit, 
> > the system stops the service and declares the app to be ANR.
> 
> This patch is trying to circumvent this limitation while also avoiding posting a user visible notification by using a JobIntentService (which can run in background and doesn't need to post a notification).
> The issue with it is though that is cannot be started with a shell command.
> A broadcast also cannot be used to start the JobIntentService because of https://pastebin.com/Las0Cv7e so we were left with using a stub service to start the JobIntentService responsable for the background work.

You can still use `JobIntentService` as a regular `Service`, right? You just don't get the benefits of a `JobIntentService`. If you override `onStartCommand` in `CrashReporterService`, I think it should still be able to handle `adb start-foreground-service` or `adb startservice`.

> I'm also not sure we can check all the JobIds used by all project's libraries.
> I think using negative Ids offers a good chance of avoiding potential clashes while also allowing a wide range of available Ids to use. For example I'm using an -1000 offset to differentiate between the Ids used by the Release and Beta versions of our app.

I think we have a greater chance of avoiding duplicate IDs if we use IDs from examples like 1000, which libraries will likely avoid using.

> This class is ment to be a repository of all the IDs used by our JobIntentServices which helps to guarantee their unicity.
> I've put it in `background` because it refers to background work.
> It is easily accessible by most of the JobIntentServices used. Except `CrashReportingService`.

"mobile/android/services" is for Firefox Accounts source code. I would just put `JobIdConstants` under "mobile/android/base/java/org/mozilla/gecko"
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review258770

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/CrashReporterService.java:83
(Diff revision 5)
> +        // If JobScheduler stopped execution before work being completed it should not be restarted.
> +        return false;
> +    }
> +
> +    static void enqueueWork(@NonNull final Context context, @NonNull final Intent intent) {
> +        int jobId;

I forgot to mention. This ID should be configurable and not hardcoded, since geckoview is a library. Preferably it will be a setting inside `GeckoRuntime`. Fennec code will then configure the ID when it creates `GeckoRuntime`.
Attachment #8984140 - Flags: review?(nchen) → review-
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review258452

> You can still use `JobIntentService` as a regular `Service`, right? You just don't get the benefits of a `JobIntentService`. If you override `onStartCommand` in `CrashReporterService`, I think it should still be able to handle `adb start-foreground-service` or `adb startservice`.

I think that on Android O and above `JobIntentService`s might be invoked as bound services, in which case there's no call to `onStartCommand`, though.
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review258452

> I think that on Android O and above `JobIntentService`s might be invoked as bound services, in which case there's no call to `onStartCommand`, though.

That's when it's used through `JobScheduler` right? Our use case here involving `adb startservice` and `adb start-foreground-service` should still invoke `onStartCommand`
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review258452

> That's when it's used through `JobScheduler` right? Our use case here involving `adb startservice` and `adb start-foreground-service` should still invoke `onStartCommand`

Indeed, from my tests `startservice` still invokes `onStartCommand` even on >= Oreo.
`onStartCommand` is not invoked when running as a JobIntentService though.
Hi James,
:jchen suggested a different approach than the first version of the patch which I've now implemented but I see that he is on PTO.
Can you take a look at the patch? I've tested it and it seems ok but I'm not sure if the changes regarding GeckoRuntime respects all geckoview customs.
Flags: needinfo?(snorp)
Attachment #8984140 - Flags: review?(sdaswani)
Updated the patch with the change suggested by sebastian [1]: Added an offset for the JobIds of nightly builds also.

[1]bug 1407046 comment 49
Removed the nightly offset.
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review260028

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java:125
(Diff revision 8)
>              return false;
>          }
>  
> +        final int crashReportingJobId = settings.getCrashReportingServiceJobId();
> +        final SharedPreferences crashReportingPrefs = GeckoSharedPrefs.forCrashReporter(context);
> +        crashReportingPrefs.edit().putInt(PREF_CRASH_REPORTING_JOB_ID, crashReportingJobId).apply();

I don't like this. Can we send the job id to the crash service in the intent instead?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:157
(Diff revision 8)
>  
>          /**
> +         * On Oreo and later devices we use the JobScheduler for crash reporting in the background.
> +         * This allows for setting the unique Job Id to be used.
> +         *
> +         * @param id must be unique.

"A unique integer."

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java:158
(Diff revision 8)
>          /**
> +         * On Oreo and later devices we use the JobScheduler for crash reporting in the background.
> +         * This allows for setting the unique Job Id to be used.
> +         *
> +         * @param id must be unique.
> +         * <a href="https://developer.android.com/reference/android/app/job/JobInfo.Builder#JobInfo.Builder(int,%20android.content.ComponentName)">

I'd put this above in the method documentation

::: toolkit/crashreporter/nsExceptionHandler.cpp:1563
(Diff revision 8)
> +
> +  const char *deviceAndroidVersion = PR_GetEnv("MOZ_ANDROID_DEVICE_SDK_VERSION");
> +  if (deviceAndroidVersion != nullptr) {
> +    const int deviceSdkVersion = atol(deviceAndroidVersion);
> +    if (deviceSdkVersion >= 26) {
> +      androidStartServiceCommand = (char*)"start-foreground-service";

shouldn't need this cast

::: toolkit/crashreporter/nsExceptionHandler.cpp:1565
(Diff revision 8)
> +  if (deviceAndroidVersion != nullptr) {
> +    const int deviceSdkVersion = atol(deviceAndroidVersion);
> +    if (deviceSdkVersion >= 26) {
> +      androidStartServiceCommand = (char*)"start-foreground-service";
> +    } else {
> +      androidStartServiceCommand = (char*)"startservice";

same
Attachment #8984140 - Flags: review?(snorp) → review-
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #35)
> Comment on attachment 8984140 [details]
> Bug 1467461 - Migrate CrashReportingService to JobIntentService;
> 
> https://reviewboard.mozilla.org/r/249958/#review260028
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.
> java:125
> (Diff revision 8)
> >              return false;
> >          }
> >  
> > +        final int crashReportingJobId = settings.getCrashReportingServiceJobId();
> > +        final SharedPreferences crashReportingPrefs = GeckoSharedPrefs.forCrashReporter(context);
> > +        crashReportingPrefs.edit().putInt(PREF_CRASH_REPORTING_JOB_ID, crashReportingJobId).apply();
> 
> I don't like this. Can we send the job id to the crash service in the intent instead?

That should be a better solution but I don't know how can I pass jobId's value from GeckoRuntime's GeckoRuntimeSettings to CrashHandler(from which the Service is started) and to GeckoLoader(from which I can set it as an environment variable accessible to nsExceptionHandler).
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review260028

> I don't like this. Can we send the job id to the crash service in the intent instead?

To pass the Job Id from our app to CrashHandler and to nsExceptionHandler.cpp I went with:
- GeckoApplication will set the Id when creating the GeckoRuntimeSettings
- GeckoRuntime will save this in it's extras sent to GeckoThread in `GeckoThread.initMainProcess`
- GeckoThread (singleton) will save the Id end expose it through a static method accessible to both CrashHandler and nsExceptionHandler.cpp.
Comment on attachment 8984140 [details]
Bug 1467461 - Migrate CrashReportingService to JobIntentService;

https://reviewboard.mozilla.org/r/249958/#review261106
Attachment #8984140 - Flags: review?(snorp) → review+
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5be8804e7189
Migrate CrashReportingService to JobIntentService. r=snorp
https://hg.mozilla.org/mozilla-central/rev/5be8804e7189
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1480421
You need to log in before you can comment on or make changes to this bug.