Remove SYNC_EXTRAS_MANUAL entirely

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: nalexander, Unassigned, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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
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?
(Reporter)

Comment 2

3 years ago
(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.
Created attachment 8708815 [details] [diff] [review]
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.
Attachment #8708815 - Flags: review?(nalexander)
I'm sorry for the delay in posting the patch, I was caught up with other work
(Reporter)

Comment 5

3 years ago
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.
Attachment #8708815 - Flags: review?(nalexander) → feedback+
Created attachment 8710615 [details] [diff] [review]
Removes EXTRAS_SYNC_MANUAL entirely

Removed all instances of SYNC_RESPECT*, renamed logSyncHints to logSyncOptions, added a method to save lines spent making bundles.
Attachment #8708815 - Attachment is obsolete: true
Attachment #8710615 - Flags: review?(nalexander)
(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
Created attachment 8710623 [details] [diff] [review]
Removes EXTRAS_SYNC_MANUAL entirely

Rectified a mistake where a forced sync would be initiated if a special case of requestSync was used.
Attachment #8710615 - Attachment is obsolete: true
Attachment #8710615 - Flags: review?(nalexander)

Updated

3 years ago
Attachment #8710623 - Flags: review?(nalexander)
(Reporter)

Comment 9

3 years ago
Created attachment 8711145 [details]
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/
Attachment #8711145 - Flags: review?(nalexander)
(Reporter)

Comment 10

3 years ago
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.
Attachment #8710623 - Flags: review?(nalexander)
(Reporter)

Comment 11

3 years ago
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 :)
Attachment #8711145 - Flags: review?(nalexander)
Attachment #8711145 - Flags: review+
Attachment #8711145 - Flags: feedback?(varunj.1011)
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!
Attachment #8711145 - Flags: feedback?(varunj.1011) → feedback+

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a9c0981668c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Updated

11 months ago
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.