1182206 - Remove SYNC_EXTRAS_MANUAL entirely

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Nick Alexander :nalexander [he/him]]

Right now, forcing a Sync (from within Fennec, or in the Firefox Account Status Activity) forces too hard: we use SYNC_EXTRAS_MANUAL, which is SYNC_EXTRAS_IGNORE_SETTINGS | SYNC_EXTRAS_IGNORE_BACKOFF [1].  The problem is that SYNC_EXTRAS_IGNORE_SETTINGS syncs even disabled (unchecked in Android Settings) SyncAdapters.  We shouldn't do that.

This ticket tracks remove SYNC_EXTRAS_MANUAL from the whole code-base, and also handling forced Syncs better.  Right now, the FxAccountSyncAdapter will deny back-to-back sync requests (even forced!) due to the 30 second rate limiter.  This ticket also tracks improving that.

This code is intertwingled with an abstraction around SyncHints in FirefoxAccounts.java [2] that didn't quite work out.  We might just remove that abstraction, since the ContentResolver one is decent: just pass through flags like the Android system does.

This is a good mentor bug, but not a good first bug.

[1] http://developer.android.com/reference/android/content/ContentResolver.html#SYNC_EXTRAS_MANUAL

[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/FirefoxAccounts.java

Comment 1

[Varun Joshi (:vjoshi)]

If I understand correctly, this entails:
1. replacing SYNC_EXTRAS_MANUAL with SYNC_EXTRAS_IGNORE_BACKOFF
2. exempting forced sync attempts from the 30 second rate limit
3. removing SyncHints, and using ContentResolver instead everywhere

Is this right?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Varun Joshi from comment #1)
> If I understand correctly, this entails:
> 1. replacing SYNC_EXTRAS_MANUAL with SYNC_EXTRAS_IGNORE_BACKOFF
> 2. exempting forced sync attempts from the 30 second rate limit
> 3. removing SyncHints, and using ContentResolver instead everywhere
> 
> Is this right?

I think so.  I'd start by removing SyncHints, and figure out what you'd need to replace.  Then remove SYNC_EXTRAS_MANUAL, and update the code around https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java#402.

Finally I'd do 2., by checking if we have a SYNC_EXTRAS_IGNORE_BACKOFF or something like that before testing for the 30 seconds debounce around https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java#406.

Feel free to ask questions!  Best to flag me with a needinfo? flag so I don't lose your question.

Comment 3

[Varun Joshi (:vjoshi)]

Attachment: Removes SYNC_EXTRAS_MANUAL

This is a rough version, I just want to know if I'm on the right track. I'll have to update TestFirefoxAccounts with something more suitable.

Comment 4

[Varun Joshi (:vjoshi)]

I'm sorry for the delay in posting the patch, I was caught up with other work

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8708815 [details] [diff] [review]
Removes SYNC_EXTRAS_MANUAL

Review of attachment 8708815 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking very good.  I have a few nits, but they're minor.  Two significant things: first, drop `SYNC_EXTRAS_RESPECT*`.  Two, add a special case for just one flag to `requestSync`, so we're spending fewer lines creating bundles.

The tests I will be running against this are:

1) making sure we pull-to-refresh in Synced Tabs panel without rate limiting;
2) making sure we can force sync in Settings > Sync without rate limiting;
3) making sure we do rate limit against background syncs (not manually initiated);
4) making sure we can disable syncing in the Android Settings, and then manually force a sync in that interface.  (I think this is still possible.)

If you can try those all and report back, that will be great.  Fine work, Varun!

::: mobile/android/base/java/org/mozilla/gecko/home/RemoteTabsBaseFragment.java
@@ +257,5 @@
>          @Override
>          public void onRefresh() {
>              if (FirefoxAccounts.firefoxAccountsExist(getActivity())) {
>                  final Account account = FirefoxAccounts.getFirefoxAccount(getActivity());
> +                Bundle syncOptions = new Bundle();

nit: final (everywhere possible).

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FirefoxAccounts.java
@@ +148,5 @@
>      return account.name;
>    }
>  
> +  public static void logSyncHints(Bundle syncOptions) {
> +    final boolean scheduleNow = syncOptions.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, false);

Let's kill `SYNC_EXTRAS_RESPECT*` entirely while we're here.  That is, after this, we'll *only* care about `SYNC_EXTRAS_IGNORE_BACKOFF`.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java
@@ +551,4 @@
>     * @param stagesToSync stage names to sync.
>     * @param stagesToSkip stage names to skip.
>     */
> +  public void requestSync(Bundle syncOptions, String[] stagesToSync, String[] stagesToSkip) {

Consider also adding a special case for just one option and creating the `Bundle` in that special method.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java
@@ +223,5 @@
>      if (extras == null) {
>        return false;
>      }
>  
> +    final boolean forced = extras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, false);

Excellent.  The logic in this file looks exactly like I expected.  Good work!

::: mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/fxa/TestFirefoxAccounts.java
@@ +36,5 @@
>    }
>  
>    public void testMakeTestBundle() {
>      Bundle bundle;
> +

nit: drop spurious new lines.

Comment 6

[Varun Joshi (:vjoshi)]

Attachment: Removes EXTRAS_SYNC_MANUAL entirely

Removed all instances of SYNC_RESPECT*, renamed logSyncHints to logSyncOptions, added a method to save lines spent making bundles.

Comment 7

[Varun Joshi (:vjoshi)]

(In reply to Nick Alexander :nalexander from comment #5)
> Comment on attachment 8708815 [details] [diff] [review]
> Removes SYNC_EXTRAS_MANUAL
> 
> Review of attachment 8708815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking very good.  I have a few nits, but they're minor.  Two
> significant things: first, drop `SYNC_EXTRAS_RESPECT*`.  Two, add a special
> case for just one flag to `requestSync`, so we're spending fewer lines
> creating bundles.
> 
> The tests I will be running against this are:
> 
> 1) making sure we pull-to-refresh in Synced Tabs panel without rate limiting;
> 2) making sure we can force sync in Settings > Sync without rate limiting;
> 3) making sure we do rate limit against background syncs (not manually
> initiated);
> 4) making sure we can disable syncing in the Android Settings, and then
> manually force a sync in that interface.  (I think this is still possible.)
> 
> If you can try those all and report back, that will be great.  Fine work,
> Varun!

Thank you for the review and sorry for the delay!

1 and 2 work. Tried forcing about 3 syncs back-to-back, all succeeded.
I'm not sure about how I can test 3.
As for 4, we can't manually force a sync after disabling syncing (not from Android Settings, not from anywhere else) 

I also need to know what I should include in TestFirefoxAccounts

Comment 8

[Varun Joshi (:vjoshi)]

Attachment: Removes EXTRAS_SYNC_MANUAL entirely

Rectified a mistake where a forced sync would be initiated if a special case of requestSync was used.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1182206 - Remove SYNC_EXTRAS_MANUAL entirely. r=nalexander

This patch removes the bespoke SyncHints type in favour of using
Android's own flags to ignore backoff and request an expedited sync.
It also unifies a simpler request{Immediate,Eventual}Sync API for
sync-requesting consumers.

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

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8710623 [details] [diff] [review]
Removes EXTRAS_SYNC_MANUAL entirely

Review of attachment 8710623 [details] [diff] [review]:
-----------------------------------------------------------------

Varun -- this is looking good.  I've pushed an updated version for you to OK.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8711145 [details]
MozReview Request: Bug 1182206 - Remove SYNC_EXTRAS_MANUAL entirely. r=nalexander

Varun: your patch looked good, but I made some changes -- including removing the unneeded test -- so I'd like you to OK them.  I simplified the API a little and added the http://developer.android.com/reference/android/content/ContentResolver.html#SYNC_EXTRAS_EXPEDITED flag, since on my devices multiple IGNORE_BACKOFF Sync requests don't actually trigger a sync.  (This is Android version and device specific.)

Great work on this ticket!  Do you have another bug under way, or would you like me to suggest something interesting?  You're strong so I'll need to find you something more significant :)

Comment 12

[Varun Joshi (:vjoshi)]

Comment on attachment 8711145 [details]
MozReview Request: Bug 1182206 - Remove SYNC_EXTRAS_MANUAL entirely. r=nalexander

This looks perfect!

I'm not working on anything else, and I'm always up for a challenge, so it would be awesome if you could find me something interesting :) Thanks a lot!

Comment 13

[Varun Joshi (:vjoshi)]

https://hg.mozilla.org/try/rev/663ccc798bde

Comment 14

[Varun Joshi (:vjoshi)]

Oops, wrong link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=663ccc798bde

Comment 15

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/4a9c0981668c0c1a29b8143b54ad56e323d65b72
Bug 1182206 - Remove SYNC_EXTRAS_MANUAL entirely. r=nalexander

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4a9c0981668c