1407046 - Abide by Android Oreo background execution limits

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P3)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Oreo introduced new execution background limits [1]. Roughly, if your app targets O, your app can run only run background services when it’s active, for several minutes after being backgrounded, or for several minutes after receiving an important push/broadcast/event (e.g. Firebase cloud message). You can still run foreground services without limitations and JobScheduler will still intelligently schedule your jobs so you can use that to replace previous background service work.

Caveat: while its enforced when your app targets O, users on Android O can enable the restrictions on any app if they choose too.

Wrappers:
- JobIntentService was introduced to run IntentServices on JobScheduler where available, or IntentService otherwise
- Evernote’s Android-job library [2] wraps JobScheduler and pre-JobScheduler implementations and directs to the appropriate one as necessary

---

We should look at our background services (e.g. go through the manifest) to identify which will need to change to adapt to the new format. Right now, it looks like we have at least 11 classes that extend `IntentService` [3].

Consider changing this bug to a meta to change the code incrementally.

[1]: https://developer.android.com/about/versions/oreo/background.html
[2]: https://github.com/evernote/android-job
[3]: http://searchfox.org/mozilla-central/search?q=path%3Amobile%2Fandroid+extends+intentservice&path=

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI Grisha as an FYI for Sync services; no action needed.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] Blocked on targeting Android O.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

JobIntentService was added in 26.1.0 so we first need the Android support library used to be at at least that same version.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

I wanted to migrate FxAccountDeletedService and FxAccountProfileService which are now IntentServices to JobIntentService, but I see thay both are in /mobile/android/services/... and the ReadMe of this module says
"These files are managed in the android-sync repo. Do not modify directly, or your changes will be lost." [1]

How should I proceed?

Comment 5

[Jan Henning [:JanH]]

That comment is outdated, as is the one at https://dxr.mozilla.org/mozilla-central/rev/a466172aed4bc2afc21169b749b8068a4b98c93f/mobile/android/services/src/main/java/org/mozilla/gecko/sync/README.txt.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

Thanks Jan!

Indeed I can find those classes in moziilla-central:
FxAccountProfileService - https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountProfileService.java
FxAccountDeletedService - https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java

I created bug 1465053 to address the removal of those confusing READMEs.

Comment 7

[Petru-Mugurel Lingurar [:petru]]

Another big issue is with Stumbler.
I understand it was supposed to always run in the background, collect data and upload it. It was supposed to even start at boot, irrespective of app's state.
This will not be possible anymore unless it will be a foreground service (with a notification visible to the user).

I also heard about talks to remove it.

NI'ing Susheel to help choosing between this two options.

Comment 8

[:sdaswani]

Andreas, do you still use the Stumbler data for anything? I think we have to shut it off if we want to support Oreo.

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate DownloadContentService to JobIntentService;

Broke the big IntentService into four small JobIntentServices because
the same JobIntentService class cannot be used with multiple JobIds
(https://github.com/aosp-mirror/platform_frameworks_support/blob/b6838fd2d2e834fdd38aab34511d385cb7108f63/compat/src/main/java/android/support/v4/app/JobIntentService.java#L121)
Also:
    - will make the code easier to be migrated to WorkManager in the future
    - more in line with SRP. It was initially doing too much.

Cleaned the code a little, removed the unneededly creation of new Threads for
DownloadContentCatalog().persistChanges() / .startLoadFromDisk()
as those methods are always called from the background threads
of the new JobIntentServices.
The new DlcHelper helps reducing duplicated code.

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

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate TabReceivedService to JobIntentService;

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

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate FxAccountProfileService to JobIntentService;

Also cleaned the code a little.

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

Comment 12

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService;

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

Comment 13

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate TelemetryUploadService to JobIntentService;

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

Comment 14

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate FileCleanupService to JobIntentService;

Also cleaned the code a little.

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

Comment 15

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate UpdateService to JobIntentService;

Broke the big IntentService into four small JobIntentServices because
the same JobIntentService class cannot be used with multiple JobIds
(https://github.com/aosp-mirror/platform_frameworks_support/blob/b6838fd2d2e834fdd38aab34511d385cb7108f63/compat/src/main/java/android/support/v4/app/JobIntentService.java#L121)
Also:
    - will make the code easier to be migrated to WorkManager in the future
    - more in line with SRP. It was initially doing too much.

All the functionality of the big UpdateService class has been incorporated in
Updater.java, UpdatesPrefs.java and UpdatesServiceHelper.java
with the main logic to drive the important actions inside the new Services.
UpdaterService is used as parent of the newly created service to help avoid
duplicated code.
Created an inner BroadcastReceiver to act upon notification actions while
the service which posted it is running as it's state needed to be modified.
Created a BroadcastReceiver to act on actions from notifications which remained
posted after the service that posted them finished. This receiver will just
start another UpdaterService.
Otherwise the services are to be started from the UpdateServiceHelper class.

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

Comment 16

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

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

Comment 17

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983764 [details]
Bug 1407046 - Migrate TabReceivedService to JobIntentService;

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

Comment 18

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983765 [details]
Bug 1407046 - Migrate FxAccountProfileService to JobIntentService;

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

Comment 19

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983766 [details]
Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService;

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

Comment 20

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983767 [details]
Bug 1407046 - Migrate TelemetryUploadService to JobIntentService;

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

Comment 21

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983768 [details]
Bug 1407046 - Migrate FileCleanupService to JobIntentService;

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

Comment 22

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983769 [details]
Bug 1407046 - Migrate UpdateService to JobIntentService;

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

Comment 23

[Kevin Brosnan [Ex-Mozilla]]

Recent thread on mozilla.dev.geolocation "Status of Mozilla Stumbler?" - https://groups.google.com/forum/#!topic/mozilla.dev.geolocation/lx09zGU8dKM

Dropping some NI to people who own(ish) the data. See comment 7 & 8

Comment 24

[Jan Henning [:JanH]]

Comment on attachment 8983765 [details]
Bug 1407046 - Migrate FxAccountProfileService to JobIntentService;

https://reviewboard.mozilla.org/r/249596/#review256656

Comment 25

[Jan Henning [:JanH]]

Comment on attachment 8983766 [details]
Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService;

https://reviewboard.mozilla.org/r/249598/#review256672

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java:52
(Diff revision 2)
> -  public FxAccountDeletedService() {
> -    super(LOG_TAG);
> +  public static void enqueueWork(@NonNull final Context context, @NonNull final Intent workIntent) {
> +    enqueueWork(context, FxAccountDeletedService.class, JobIdsConstants.JOB_ID_PROFILE_DELETE, workIntent);
>    }
>  
>    @Override
> -  protected void onHandleIntent(final Intent intent) {
> +  protected void onHandleWork(Intent intent) {

Recent framework code now explicitly have the Intent in the IntentService's `onHandleIntent(@Nullable final Intent intent)` as `@Nullable`, whereas for the JobIntentService on the other hand the Intent is declared to be `@NonNull`.

I've also tried looking at the source for the JobIntentService (e.g. [here](http://androidxref.com/8.0.0_r4/xref/frameworks/support/compat/java/android/support/v4/app/JobIntentService.java#365)) and by my reading it, unlike presumably the IntentService implementation does, it doesn't leave delivery of the Intent to the system, where apparently under some circumstances it can get lost.
Instead, it keeps its own list of work items and when it runs, the Intent is retrieved from that work item and then directly passed to `onHandleWork`.

So in summary I think you can preserve the original function signature and mark this `@NonNull` as well.

Comment 26

[Jan Henning [:JanH]]

Comment on attachment 8983767 [details]
Bug 1407046 - Migrate TelemetryUploadService to JobIntentService;

https://reviewboard.mozilla.org/r/249600/#review256688

Whether and how to abort all queued uploads if we encountered a failure needs some more thinking about (see below), but on the other hand for the presumably most common case (no network connection available) we already return early beforehand without aborting other queued work, so I guess we can just land this and think about that problem when there's some more spare time.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java:89
(Diff revision 2)
>          if (!wereAllUploadsSuccessful) {
>              // If we had an upload failure, we should stop the IntentService and drop any
>              // pending Intents in the queue so we don't waste resources (e.g. battery)
>              // trying to upload when there's likely to be another connection failure.
>              Log.d(LOGTAG, "Clearing Intent queue due to connection failures");
>              stopSelf();

I have a suspicion that this trick might not longer work on Android O and above, where we're running as a `JobService`, which, given the entry for `onBind` in the documentation ("Returns the IBinder for the JobServiceEngine when running as a JobService on O and later platforms.") might run as a bound service, where calling stopSelf() won't have any effect.

Plus I think in that `JobService` mode the list of Jobs still to be executed might no longer even be held by ther service itself, so even if we could destroy it that wouldn't help in that regard.

Unfortunately I've got no idea how necessary this is , i.e. how hard we should try finding an alternative design for Android O and above...

At the very least, leave a follow-up bug for this.

Comment 27

[Jan Henning [:JanH]]

Comment on attachment 8983768 [details]
Bug 1407046 - Migrate FileCleanupService to JobIntentService;

https://reviewboard.mozilla.org/r/249602/#review256696

Comment 28

[Jan Henning [:JanH]]

Comment on attachment 8983764 [details]
Bug 1407046 - Migrate TabReceivedService to JobIntentService;

https://reviewboard.mozilla.org/r/249594/#review256700

::: mobile/android/base/java/org/mozilla/gecko/tabqueue/TabReceivedService.java:89
(Diff revision 2)
>          prefs.edit().putInt(PREF_NOTIFICATION_ID, notificationId).apply();
>      }
>  
> +    @Override
> +    public boolean onStopCurrentWork() {
> +        // Reschedule the work it it has been stopped before completing (shouldn't)

Nit: "Reschedule the work it it has [...]" -> if it

Comment 29

[Victor Ng [:vng] [:crankycoder]]

:kbrosnan We can't easily remove the Stumbler from Firefox as MLS is still an ongoing concern at this point.  The majority of our data collection is because of the passive stumbler in Firefox Android.   We're trying to schedule some meetings to figure out what to do about Stumbler and MLS during the allhands coming up in SF.

Comment 30

[Jan Henning [:JanH]]

(In reply to Petru-Mugurel Lingurar[:petru] from comment #7)
> Another big issue is with Stumbler.
> I understand it was supposed to always run in the background, collect data
> and upload it. It was supposed to even start at boot, irrespective of app's
> state.
> This will not be possible anymore unless it will be a foreground service
> (with a notification visible to the user).
> 
> I also heard about talks to remove it.
> 
> NI'ing Susheel to help choosing between this two options.

There also a third option - only start the service when Firefox is in foreground, at least when running on Oreo or higher...

Comment 31

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #30)
> (In reply to Petru-Mugurel Lingurar[:petru] from comment #7)
> > Another big issue is with Stumbler.
> > I understand it was supposed to always run in the background, collect data
> > and upload it. It was supposed to even start at boot, irrespective of app's
> > state.
> > This will not be possible anymore unless it will be a foreground service
> > (with a notification visible to the user).
> > 
> > I also heard about talks to remove it.
> > 
> > NI'ing Susheel to help choosing between this two options.
> 
> There also a third option - only start the service when Firefox is in
> foreground, at least when running on Oreo or higher...

Forget what I said there - background services will be stopped by the OS some time after the app goes into the background.

Comment 32

[Jan Henning [:JanH]]

Just noticed that this is actually only required "if your app targets O", so needs to block that bug as well.

Comment 33

[Jan Henning [:JanH]]

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

https://reviewboard.mozilla.org/r/249592/#review256864

The DLC changes themselves look absolutely fine, but JobIdsConstants.java needs some more work because of our shared UID problem, hence the r-.

::: mobile/android/services/src/main/java/org/mozilla/gecko/background/JobIdsConstants.java:5
(Diff revision 2)
> +package org.mozilla.gecko.background;
> +
> +/**
> + * 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>

Hmm, the documentation says that
> This ID must be unique across all clients of the same uid (not just the same package).

and unfortunately Firefox and Firefox Beta *do* use a shared UID (see bug 1437871), as did org.mozilla.fennec and org.mozilla.fennec_aurora, although the latter case is less relevant since the "fennec" builds have now been discontinued.

So I fear that this could lead to unintended behaviour for users who have installed both Release and Beta, since one app could unintentionally mess up the other app's jobs.

So maybe we should add (or subtract, since you're using negative IDs) a constant coming out of our build config, so we can specify differing offsets for Release and Beta. Ping nalexander if you need any help there.

Comment 34

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

https://reviewboard.mozilla.org/r/249592/#review256864

> Hmm, the documentation says that
> > This ID must be unique across all clients of the same uid (not just the same package).
> 
> and unfortunately Firefox and Firefox Beta *do* use a shared UID (see bug 1437871), as did org.mozilla.fennec and org.mozilla.fennec_aurora, although the latter case is less relevant since the "fennec" builds have now been discontinued.
> 
> So I fear that this could lead to unintended behaviour for users who have installed both Release and Beta, since one app could unintentionally mess up the other app's jobs.
> 
> So maybe we should add (or subtract, since you're using negative IDs) a constant coming out of our build config, so we can specify differing offsets for Release and Beta. Ping nalexander if you need any help there.

Thanks! Didn't know about this shared UID issue.
Will be using an offset set in `JobIdsConstants` to provide an offsetted id for Beta.

Comment 35

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983767 [details]
Bug 1407046 - Migrate TelemetryUploadService to JobIntentService;

https://reviewboard.mozilla.org/r/249600/#review256688

> I have a suspicion that this trick might not longer work on Android O and above, where we're running as a `JobService`, which, given the entry for `onBind` in the documentation ("Returns the IBinder for the JobServiceEngine when running as a JobService on O and later platforms.") might run as a bound service, where calling stopSelf() won't have any effect.
> 
> Plus I think in that `JobService` mode the list of Jobs still to be executed might no longer even be held by ther service itself, so even if we could destroy it that wouldn't help in that regard.
> 
> Unfortunately I've got no idea how necessary this is , i.e. how hard we should try finding an alternative design for Android O and above...
> 
> At the very least, leave a follow-up bug for this.

Thanks for looking into this.
How I viewed this situation:
When running on pre Oreo the JobIntentService will be a normal IntentService so that stopSelf() should still work as before.
When running on >= Oreo the JobServiceEngine will indeed bind to the JobIntentService (that's why the bind permission is needed in manifest). Even if stopSelf() wouldn't have any effect here it is the last code executing in the `onHandleWork()` block. But I don't know if after this the service will be unbinded and destroyed. Created bug 1468284 to investigate.

Comment 36

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

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

Comment 37

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983764 [details]
Bug 1407046 - Migrate TabReceivedService to JobIntentService;

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

Comment 38

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983765 [details]
Bug 1407046 - Migrate FxAccountProfileService to JobIntentService;

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

Comment 39

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983766 [details]
Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService;

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

Comment 40

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983767 [details]
Bug 1407046 - Migrate TelemetryUploadService to JobIntentService;

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

Comment 41

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983768 [details]
Bug 1407046 - Migrate FileCleanupService to JobIntentService;

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

Comment 42

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983769 [details]
Bug 1407046 - Migrate UpdateService to JobIntentService;

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

Comment 43

[Petru-Mugurel Lingurar [:petru]]

Updated the patches to support different Job Ids for Release / Beta builds.

Comment 44

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

Comment on attachment 8983766 [details]
Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService;

https://reviewboard.mozilla.org/r/249598/#review258502

Comment 45

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

Comment on attachment 8983769 [details]
Bug 1407046 - Migrate UpdateService to JobIntentService;

https://reviewboard.mozilla.org/r/249604/#review258504

We don't actually use the updater service anymore. I think it's okay if we don't make this change. (We should remove the service eventually)

Comment 46

[Jan Henning [:JanH]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #45)
> Comment on attachment 8983769 [details]
> Bug 1407046 - Migrate UpdateService to JobIntentService;
> 
> https://reviewboard.mozilla.org/r/249604/#review258504
> 
> We don't actually use the updater service anymore.

Don't we? It's still working in practice, is AFAIK the only update mechanism for single-locale builds (which aren't on Google Play, are they?), plus it also provides updates for anybody else who can't or doesn't want to use the Play Store?

Comment 47

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

Hm you're right. We do still use the updater service for these edge cases.

Comment 48

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

Comment on attachment 8983769 [details]
Bug 1407046 - Migrate UpdateService to JobIntentService;

https://reviewboard.mozilla.org/r/249604/#review258750

Looks okay, assuming it's been thoroughly tested

::: mobile/android/base/java/org/mozilla/gecko/updater/PackageVerifier.java:14
(Diff revision 3)
> +import java.io.FileInputStream;
> +import java.io.InputStream;
> +import java.security.MessageDigest;
> +
> +public class PackageVerifier {
> +    private static final String LOGTAG = "PackageVerifier";

"GeckoPackageVerifier"

::: mobile/android/base/java/org/mozilla/gecko/updater/UpdateServiceReceiver.java:14
(Diff revision 3)
> + * Receiver only used in local notifications posted by an {@link UpdaterService} after which it finishes.<br>
> + * Used to start another variant of {@link UpdaterService}.
> + */
> +public class UpdateServiceReceiver extends BroadcastReceiver {
> +    private static final String LOGTAG = "UpdateServiceReceiver";
> +

Add "Gecko" prefix to `LOGTAG` here and elsewhere.

::: mobile/android/base/java/org/mozilla/gecko/updater/UpdateServiceReceiver.java:22
(Diff revision 3)
> +
> +    @Override
> +    public void onReceive(Context context, Intent intent) {
> +        final String action = intent.getAction();
> +
> +        Log.d(LOGTAG, String.format("Will enqueue \"%s\" work", action));

Put logging inside `if (DEBUG) { ... }` here and elsewhere.

::: mobile/android/base/java/org/mozilla/gecko/updater/UpdaterService.java:16
(Diff revision 3)
> +    protected Updater updater;
> +    protected UpdatesPrefs prefs;
> +
> +    @Override
> +    public void onCreate() {
> +        crashHandler = CrashHandler.createDefaultCrashHandler(getApplicationContext());

We will end up creating multiple crash handlers for the different services. We should only create one

Comment 49

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

https://reviewboard.mozilla.org/r/249592/#review259194

::: mobile/android/services/src/main/java/org/mozilla/gecko/background/JobIdsConstants.java:20
(Diff revision 3)
> + * @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 {
> +    // Offset to differentiate the ids for Release/Beta versions of the app
> +    private static final int BETA_OFFSET = -1000;

I think we are going to need different offsets for Nightly <-> Beta <-> Release?

Comment 50

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

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

Comment 51

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983764 [details]
Bug 1407046 - Migrate TabReceivedService to JobIntentService;

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

Comment 52

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983765 [details]
Bug 1407046 - Migrate FxAccountProfileService to JobIntentService;

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

Comment 53

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983766 [details]
Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService;

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

Comment 54

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983767 [details]
Bug 1407046 - Migrate TelemetryUploadService to JobIntentService;

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

Comment 55

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983768 [details]
Bug 1407046 - Migrate FileCleanupService to JobIntentService;

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

Comment 56

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983769 [details]
Bug 1407046 - Migrate UpdateService to JobIntentService;

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

Comment 57

[Petru-Mugurel Lingurar [:petru]]

- moved `JobIdsConstants()` to "mobile/android/base/java/org/mozilla/gecko" and started the Ids from 1000 as Jim suggested in bug 1467461 comment 24
- added a second offset for Nightly
- implemented the changes Jim suggested
- rebased the patches

Comment 58

[Jan Henning [:JanH]]

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

https://reviewboard.mozilla.org/r/249592/#review259194

> I think we are going to need different offsets for Nightly <-> Beta <-> Release?

Nightly doesn't share an UID with our other APKs, so it doesn't actually need a separate offset, does it?

Comment 59

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jan Henning (on vacation in June) [:JanH] from comment #58)
> Comment on attachment 8983763 [details]
> Bug 1407046 - Migrate DownloadContentService to JobIntentService;
> 
> https://reviewboard.mozilla.org/r/249592/#review259194
> 
> > I think we are going to need different offsets for Nightly <-> Beta <-> Release?
> 
> Nightly doesn't share an UID with our other APKs, so it doesn't actually
> need a separate offset, does it?

Ah, that's right. I assumed we would set sharedUserId to the same value on all our builds. But they Nightly seems to use a different value AND is signed with a different key too. Sorry!

Comment 60

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

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

Comment 61

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983764 [details]
Bug 1407046 - Migrate TabReceivedService to JobIntentService;

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

Comment 62

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983765 [details]
Bug 1407046 - Migrate FxAccountProfileService to JobIntentService;

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

Comment 63

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983766 [details]
Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService;

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

Comment 64

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983767 [details]
Bug 1407046 - Migrate TelemetryUploadService to JobIntentService;

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

Comment 65

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983768 [details]
Bug 1407046 - Migrate FileCleanupService to JobIntentService;

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

Comment 66

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983769 [details]
Bug 1407046 - Migrate UpdateService to JobIntentService;

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

Comment 67

[Petru-Mugurel Lingurar [:petru]]

- removed the nightly offset
- added missing MPL licenses

Comment 68

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

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

https://reviewboard.mozilla.org/r/249592/#review260032

Comment 69

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8983763 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249592/diff/5-6/

Comment 70

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate TabReceivedService to JobIntentService;

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

Comment 71

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate FxAccountProfileService to JobIntentService;

Also cleaned the code a little.

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

Comment 72

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService;

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

Comment 73

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate TelemetryUploadService to JobIntentService;

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

Comment 74

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate FileCleanupService to JobIntentService;

Also cleaned the code a little.

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

Comment 75

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate UpdateService to JobIntentService

Broke the big IntentService into four small JobIntentServices because
the same JobIntentService class cannot be used with multiple JobIds
(https://github.com/aosp-mirror/platform_frameworks_support/blob/b6838fd2d2e834fdd38aab34511d385cb7108f63/compat/src/main/java/android/support/v4/app/JobIntentService.java#L121)
Also:
    - will make the code easier to be migrated to WorkManager in the future
    - more in line with SRP. It was initially doing too much.

All the functionality of the big UpdateService class has been incorporated in
Updater.java, UpdatesPrefs.java and UpdatesServiceHelper.java
with the main logic to drive the important actions inside the new Services.
UpdaterService is used as parent of the newly created service to help avoid
duplicated code.
Created an inner BroadcastReceiver to act upon notification actions while
the service which posted it is running as it's state needed to be modified.
Created a BroadcastReceiver to act on actions from notifications which remained
posted after the service that posted them finished. This receiver will just
start another UpdaterService.
Otherwise the services are to be started from the UpdateServiceHelper class.

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

Comment 76

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate DownloadContentService to JobIntentService

Broke the big IntentService into four small JobIntentServices because
the same JobIntentService class cannot be used with multiple JobIds
(https://github.com/aosp-mirror/platform_frameworks_support/blob/b6838fd2d2e834fdd38aab34511d385cb7108f63/compat/src/main/java/android/support/v4/app/JobIntentService.java#L121)
Also:
    - will make the code easier to be migrated to WorkManager in the future
    - more in line with SRP. It was initially doing too much.

Cleaned the code a little, removed the superfluous creation of new Threads for
DownloadContentCatalog().persistChanges() / .startLoadFromDisk()
as those methods are always called from the background threads
of the new JobIntentServices.
The new DlcHelper helps reducing duplicated code.

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

Comment 77

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate TabReceivedService to JobIntentService

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

Comment 78

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate FxAccountProfileService to JobIntentService

Also cleaned the code a little.

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

Comment 79

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService

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

Comment 80

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate TelemetryUploadService to JobIntentService

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

Comment 81

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1407046 - Migrate FileCleanupService to JobIntentService

Also cleaned the code a little.

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

Comment 82

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992268 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService

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

Comment 83

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992269 [details]
Bug 1407046 - Migrate TabReceivedService to JobIntentService

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

Comment 84

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992270 [details]
Bug 1407046 - Migrate FxAccountProfileService to JobIntentService

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

Comment 85

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992271 [details]
Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService

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

Comment 86

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992272 [details]
Bug 1407046 - Migrate TelemetryUploadService to JobIntentService

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

Comment 87

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992273 [details]
Bug 1407046 - Migrate FileCleanupService to JobIntentService

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

Comment 88

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992025 [details]
Bug 1407046 - Migrate UpdateService to JobIntentService

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

Comment 89

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e224f2f4c44a
Migrate DownloadContentService to JobIntentService. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/69fd84bee441
Migrate TabReceivedService to JobIntentService. r=JanH
https://hg.mozilla.org/integration/mozilla-inbound/rev/162d4eb92cdb
Migrate FxAccountProfileService to JobIntentService. r=JanH
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b84b2e18b5
Migrate FxAccountDeletedService to JobIntentService. r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c81eca09564
Migrate TelemetryUploadService to JobIntentService. r=JanH
https://hg.mozilla.org/integration/mozilla-inbound/rev/16424a7c3866
Migrate FileCleanupService to JobIntentService. r=JanH
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c647187c1a2
Migrate UpdateService to JobIntentService. r=jchen

Comment 90

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992268 [details]
Bug 1407046 - Migrate DownloadContentService to JobIntentService

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

Comment 91

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992269 [details]
Bug 1407046 - Migrate TabReceivedService to JobIntentService

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

Comment 92

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992270 [details]
Bug 1407046 - Migrate FxAccountProfileService to JobIntentService

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

Comment 93

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992271 [details]
Bug 1407046 - Migrate FxAccountDeletedService to JobIntentService

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

Comment 94

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992272 [details]
Bug 1407046 - Migrate TelemetryUploadService to JobIntentService

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

Comment 95

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992273 [details]
Bug 1407046 - Migrate FileCleanupService to JobIntentService

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

Comment 96

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8992025 [details]
Bug 1407046 - Migrate UpdateService to JobIntentService

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

Comment 97

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd3a8daeb33
Migrate FileCleanupService to JobIntentService: Follow-up to for android-test. r=test-fix

Comment 98

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

A(test) still fails with the follow-up: https://treeherder.mozilla.org/logviewer.html#?job_id=188428834&repo=mozilla-inbound

The changes from revision 2 to 3 had been manually ported, but I don't see any error.

Push with the failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ddd3a8daeb331d69da2dd7547d5bc87fdc97acf1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 99

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e224f2f4c44a
https://hg.mozilla.org/mozilla-central/rev/69fd84bee441
https://hg.mozilla.org/mozilla-central/rev/162d4eb92cdb
https://hg.mozilla.org/mozilla-central/rev/b2b84b2e18b5
https://hg.mozilla.org/mozilla-central/rev/2c81eca09564
https://hg.mozilla.org/mozilla-central/rev/16424a7c3866
https://hg.mozilla.org/mozilla-central/rev/2c647187c1a2
https://hg.mozilla.org/mozilla-central/rev/ddd3a8daeb33

Comment 100

[Bogdan Tara[:bogdan_tara | bogdant]]

This is still happening.

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=188456489&repo=mozilla-central

Comment 101

[Petru-Mugurel Lingurar [:petru]]

Will fix the tests failing in Bug 1476237

Comment 102

[Intermittent Failures Robot]

23 failures in 123 pushes (0.187 failures/push) were associated with this bug yesterday.    

Repository breakdown:
* autoland: 13
* mozilla-inbound: 9
* mozilla-central: 1

Platform breakdown:
* android-4-0-armv7-api16: 23

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1407046&startday=2018-07-17&endday=2018-07-17&tree=trunk

Comment 103

[Intermittent Failures Robot]

46 failures in 653 pushes (0.07 failures/push) were associated with this bug in the last 7 days. 

This is the #14 most frequent failure this week.  

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **  

Repository breakdown:
* mozilla-inbound: 20
* autoland: 20
* mozilla-central: 6

Platform breakdown:
* android-4-0-armv7-api16: 46

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1407046&startday=2018-07-16&endday=2018-07-22&tree=trunk