We should probably annotate all tasks with @UsedByReflection

Ah; I've seen issues resembling this before. This definitely sounds like an issue! This would definitely be helpful to fix as soon as possible.

+dtrainor FYI

In addition to annotation, we would need to change the task IDs for all non-annotated classes, and then also attempt to cancel the old task IDs in JobScheduler.

Andrew, you might find this interesting and you might be able to help.
Did anything about proguard configuration WRT background tasks renaming change?

This is an important bug to fix for Explore Sites, ideally in M72.  So I'd like to agree on a path forward with folks here.

This bug will manifest itself if a task is scheduled on version N and executed on version N+1.  This seems most likely for periodic tasks, but it could cause one-off tasks to be lost as well.

There are a few solutions I have in mind:
* Remove the use of reflection and have a static mapping of task ID to class
* Force background task class names to be retained (this should be enforced by a presubmit, or globally using the proguard flags file.)
** With this solution, we will need new task IDs for all periodic tasks, and some code to cancel existing periodic tasks.

Other workarounds may be required if one-off tasks are not resilient against dropped tasks.

I am working on a patch to manually change just the ExploreSites task over at https://chromium-review.googlesource.com/c/chromium/src/+/1370318 - this does the second solution, just for Explore Sites.  Anyone have opinions about this?

Yikes! Certainly need some proguard rules here, or to manually map classes -> TaskIds.

For proguard rules, I think what we'd want is:
1. Add a BackgroundSchedulerTask interface (that's empty)
2. Add a proguard rule: -keepnames class * implements BackgroundSchedulerTask { public <init>() }

That said, would it make sense to statically map TaskIds -> Classes? e.g. have a switch statement or Map that returns the class given a task ID.

The difficulty with the mapping is that the classes implementing the interface are not known to the component and are defined at a level above, by various features in the browser. We should not rely on knowing them.

We could inject the switch statement from the browser level, but I am not sure how that would be initialized, when jobservice/taskservice are started by the system.

To inject the job ID -> class mappings, it might require something crazy/hacky.  Hopefully not more than serializing class names to disk :)

* Use Dagger 2 to inject a Chrome-level dependency into BackgroundTaskJobService at runtime. https://goto.google.com/clank-dagger
  - I don't know if this is in Chromium yet?

* LocalBroadcastManager.sendBroadcastSync() could be used to call a chrome-layer broadcastreceiver and cause it to register its background tasks with the BackgroundTaskScheduler

If we can get around this with a one-off fix in the codebase for the various features using this, and then have a proguard rule for the future, that'd be my preference.

#9 - that would be nice but it is somewhat tricky to inject that map (of Chrome-level classes) into components (where BTS lives).  I was looking into Dagger2 for that purpose, but do you have any ideas?

Looks like there's already an interface for background tasks. Here's a CL for adding the -keep:
https://chromium-review.googlesource.com/c/chromium/src/+/1371244


Here's another idea that doesn't involve having to register that map during start-up, and doesn't require depending on clients:


class TaskIds {
    public static final int GCM_BACKGROUND_TASK_JOB_ID = 1;
    public static final int NOTIFICATION_SERVICE_JOB_ID = 21;
    ...
    private static final String GCM_BACKGROUND_TASK_CLASS_NAME =
            "org.chromium.chrome.browser.services.gcm.GCMBackgroundTask";
    ...

    BackgroundTask createById(int id) {
        switch (id):
            case GCM_BACKGROUND_TASK_JOB_ID:
                return (BackgroundTask) Class.forName(GCM_BACKGROUND_TASK_CLASS_NAME).newInstance();
            ...
    }
}


* Would allow recording the taskId but not having to record the class names (so devs can rename the classes without breaking this).
* Proguard/r8 will convert the reflection into direct calls, eliminating reflection overhead.
* Once we switch to r8, we can even add -identifiernamestring to have the classes obfuscated & checked for correctness.
* It's a bit odd to have the class names here, but the class already has an ID for each client, so seems no worse to me to also encode the class name.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5573d1f00e2a0c50f4e7c92439d254f8c88dd0a3

commit 5573d1f00e2a0c50f4e7c92439d254f8c88dd0a3
Author: Andrew Grieve <agrieve@chromium.org>
Date: Tue Dec 11 14:36:27 2018

Android: Add proguard flag to prevent obfuscation of BackgroundTask classes

These classes need to be the same across releases.

Bug:  913116 
Change-Id: I57dc8928f24944f0234863f5fcfc248886c6a7cc
Reviewed-on: https://chromium-review.googlesource.com/c/1371244
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615519}
[modify] https://crrev.com/5573d1f00e2a0c50f4e7c92439d254f8c88dd0a3/components/background_task_scheduler/BUILD.gn
[add] https://crrev.com/5573d1f00e2a0c50f4e7c92439d254f8c88dd0a3/components/background_task_scheduler/android/proguard.flags

Andrew: Are you planning to merge the CL from Comment #15 to M72?

Sure. Adding the merge-reguest label. It's not actually fixing a regression (this problem has likely existed since background task scheduler was introduced), but it's a small & safe change, and early on for the branch, so I'd guess it would be fine to merge.

Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Pls merge your change to M72 branch 3626 ASAP. Thank you.

Pls merge your change to M72 branch 3626 ASAP so we can pick it up for next M72 Beta release. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3fa2e21d7b4feff2d345fd5a2786c1a7ab84e8ae

commit 3fa2e21d7b4feff2d345fd5a2786c1a7ab84e8ae
Author: Andrew Grieve <agrieve@chromium.org>
Date: Thu Dec 13 21:38:02 2018

Android: Add proguard flag to prevent obfuscation of BackgroundTask classes

These classes need to be the same across releases.

TBR=agrieve@chromium.org

(cherry picked from commit 5573d1f00e2a0c50f4e7c92439d254f8c88dd0a3)

Bug:  913116 
Change-Id: I57dc8928f24944f0234863f5fcfc248886c6a7cc
Reviewed-on: https://chromium-review.googlesource.com/c/1371244
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#615519}
Reviewed-on: https://chromium-review.googlesource.com/c/1377173
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#341}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/3fa2e21d7b4feff2d345fd5a2786c1a7ab84e8ae/components/background_task_scheduler/BUILD.gn
[add] https://crrev.com/3fa2e21d7b4feff2d345fd5a2786c1a7ab84e8ae/components/background_task_scheduler/android/proguard.flags

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/3fa2e21d7b4feff2d345fd5a2786c1a7ab84e8ae

Commit: 3fa2e21d7b4feff2d345fd5a2786c1a7ab84e8ae
Author: agrieve@chromium.org
Commiter: agrieve@chromium.org
Date: 2018-12-13 21:38:02 +0000 UTC

Android: Add proguard flag to prevent obfuscation of BackgroundTask classes

These classes need to be the same across releases.

TBR=agrieve@chromium.org

(cherry picked from commit 5573d1f00e2a0c50f4e7c92439d254f8c88dd0a3)

Bug:  913116 
Change-Id: I57dc8928f24944f0234863f5fcfc248886c6a7cc
Reviewed-on: https://chromium-review.googlesource.com/c/1371244
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#615519}
Reviewed-on: https://chromium-review.googlesource.com/c/1377173
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#341}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Filed bug 916875 to look at not storing class names (idea from #14).