790931 - Allow successful removal of Sync account when multiple Firefox versions are installed

Status: RESOLVED FIXED

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

Description

[Adrian Tamas (:AdrianT)]

Attachment: logs

Firefox Mobile 16.0b3
Samsung Galaxy R (Android 2.3.4)

Steps to reproduce:
1. Have Firefox Mobile Beta and Release installed.
2. Set up sync on Firefox Beta
3. Go to Android Settings -> Accounts&Sync -> Firefox sync account and also check the sync checkbox for realease
4. Wait for sync to complete and delete the account

Expected results:
The account can be removed

Actual results:
Once the second Firefox has been added the sync account can no longer be removed from the device.

I have tried unchecking the checkboxes, turning off auto-sync, removing the account several times but the account is always recreated. I had to uninstall one version in order to be able to remove the account

Notes: 
Please see the video: http://youtu.be/k9G3kvgbdHY

Comment 1

[Aaron Train [:aaronmt]]

I'm unable to reproduce this on Android 4.0.4, and Android 4.1.1

Comment 2

[Tony Chung [:tchung]]

adrian, can you also try on a android 2.2 device?  could you reproduce on another gingerbread device?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Nick - Is an account for Beta and Release, at the same time, a valid situation? I thought it was one of (nightly or aurora) and one of (beta or release)

Comment 4

[Richard Newman [:rnewman]]

mfinkle: beta and release are supposed to fall into the same bucket, so they should be able to use the same account. Not sure if account deletion during sharing has been tested, though.

It's perhaps being recreated because both apps are backing up the account (our SD card removal workaround).

Might be unintentional — I don't think release and beta share accounts correctly yet; Nick would know — might be unavoidable!

Comment 5

[Adrian Tamas (:AdrianT)]

(In reply to Tony Chung [:tchung] from comment #2)
> adrian, can you also try on a android 2.2 device?  could you reproduce on
> another gingerbread device?

I am able to easily reproduce the issue following the steps from Comment 0 on HTC Desire (Android 2.2), Motorola Droid Pro (Android 2.3.4) and Samsung Galaxy Tab 2 7.0 (Android 4.0.4). 

However I have noticed that on the Galaxy Tab 2 (Android 4.0.4) the account seems removed at first but closing the Android settings app and going back to Firefox to the about:home page the Sync Banner is not displayed and synced tabs are still there. At this point going back to settings will redisplay the sync account.

Comment 6

[Nick Alexander :nalexander [he/him]]

> It's perhaps being recreated because both apps are backing up the account
> (our SD card removal workaround).

Ding ding ding!  I bet you're correct.
 
> Might be unintentional — I don't think release and beta share accounts
> correctly yet; Nick would know — might be unavoidable!

I wonder.  We could try to be clever in a variety of ways:

1.  Have only the owning App restore the account (but this could be problematic if the App the user actually uses is not the owning App).

2.  Restore more selectively, for example only if the App is currently installed on the SD card (but this could be problematic if the user moves the App on and off the SD card).

3.  Have all Apps listen for Account deleted.

I'll add this ticket to my queue and see if our instinct is correct.  I think 3. will be most fruitful, so I'll have a poke and see what that will entail.

Comment 7

[Nick Alexander :nalexander [he/him]]

After discussion with rnewman, we're going to make this ticket be, allow removing the Sync account when there are multiple Firefox versions sharing an Account, and all Apps are either installed on device (by far the most common scenario) or all Apps are installed on the SD card.  Moving on this part now.

See Bug 792099 for a WONTFIXed ticket for supporting mixed on device/on SD card App installs.

Comment 8

[Nick Alexander :nalexander [he/him]]

Update for tracking-firefox16: I developed a solution for this yesterday; today I will be device testing it.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

If the fix is low-risk enough, we could uplift to aurora.

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> If the fix is low-risk enough, we could uplift to aurora.

Agreed - we're not concerned about this for FF16's release.

Comment 11

[Nick Alexander :nalexander [he/him]]

Test APKs at:

http://people.mozilla.com/~nalexander/790931.beta.signed.apk
http://people.mozilla.com/~nalexander/790931.official.signed.apk

Those are branded beta and official, and share an Account type.

Test instructions:
1. Single App installed:
   - pair Android device
   - force sync
   - verify device appears in desktop list of devices
   - delete Account
   - verify Sync account no longer appears in desktop list of devices
   - start Firefox
   - verify Set up Sync is offered
   - verify Android Account has not been recreated

2. Both Apps installed:
   - pair Android device
   - enable Syncing both Apps
   - force sync
   - verify both Apps appears in desktop list of devices
   - delete Account
   - verify both Apps no longer appear in desktop list of devices
   - start both Firefoxes
   - verify Set up Sync is offered both times (Marketplace can get in the way -- try Settings > Sync as well)
   - verify Android Account has not been recreated
   - look for logs like:
     53:I FxSync(20595)               firefox :: SyncAuthService :: Account named [e-mail] being removed; broadcasting secure intent org.mozilla.firefox_sync.accounts.SYNC_ACCOUNT_DELETED_ACTION.
     54:I FxSync(20595)               firefox :: SyncAccountDeletedService :: Sync account named [e-mail] being removed; deleting saved pickle file 'sync.account.json'.
     55:I FxSync(20536)               firefox_beta :: SyncAccountDeletedService :: Sync account named [e-mail] being removed; deleting saved pickle file 'sync.account.json'.
     56:I FxSync(20595)               firefox :: SyncAccountDeletedService :: Account named [e-mail] being removed; deleting client record from server.
     81:I FxSync(20536)               firefox_beta :: SyncAccountDeletedService :: Account named [e-mail] being removed; deleting client record from server.
    160:I FxSync(20595)               firefox :: ClientRecTerminator :: Deleted client record with GUID 955Kbwzv8C_o from server.
    212:I FxSync(20536)               firefox_beta :: ClientRecTerminator :: Deleted client record with GUID nBVCseoES8T7 from server.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #11)
> Test APKs at:
> 
> http://people.mozilla.com/~nalexander/790931.beta.signed.apk
> http://people.mozilla.com/~nalexander/790931.official.signed.apk
> 
> Those are branded beta and official, and share an Account type.

I should add that they are signed with my personal key.  I will personally test that an unofficial build, modified to use the official Account type and signed with a different key, does not receive the broadcast notification.

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Alex Keybl [:akeybl] from comment #10)
> (In reply to Mark Finkle (:mfinkle) from comment #9)
> > If the fix is low-risk enough, we could uplift to aurora.
> 
> Agreed - we're not concerned about this for FF16's release.

I'll discuss further with rnewman, but this is definitely not low-risk.

I ended up adding an Android permission, a BroadcastReceiver, and an IntentService.  On account deletion, the Account authenticator broadcasts that a Sync account has been deleted.  The permission is needed because the broadcast Intent contains sensitive user information (the Sync username and password, needed to delete the Sync client record from the remote server).  This is working well for me, but needs to bake and is absolutely not low-risk.

For this to be useful, it needs to land on Aurora and Nightly simultaneously -- this is the case with all multiple-Fennec type things.

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #12)
> (In reply to Nick Alexander :nalexander from comment #11)
> > Test APKs at:
> > 
> > http://people.mozilla.com/~nalexander/790931.beta.signed.apk
> > http://people.mozilla.com/~nalexander/790931.official.signed.apk
> > 
> > Those are branded beta and official, and share an Account type.
> 
> I should add that they are signed with my personal key.  I will personally
> test that an unofficial build, modified to use the official Account type and
> signed with a different key, does not receive the broadcast notification.

I tried to do this testing, but there are enough changes needed to get permissions in place that I don't think it's worth it.  This would be an Android issue or a code problem in our manifest or broadcast code; I think review will suffice.

Comment 15

[Nick Alexander :nalexander [he/him]]

tracking-* update: waiting on review.

Comment 16

[Richard Newman [:rnewman]]

Reviewed. I'm happy with this fix; it's about as compact an implementation as one could hope for, and it's very thorough with error detection.

Note for release management: this works by introducing a communication channel (secured broadcast intents) between two installed versions. Both installed versions need to have this patch applied -- if the version that owns the account doesn't, no intent is sent; if the other version doesn't, then the intent is not acted upon.

So:

* If we land in Nightly and don't early uplift: Nightly/Aurora will appear fixed as of 2012-10-09; Beta/Release will appear fixed as of 2013-01-06.

* If we uplift to Aurora before 2012-10-09: Beta/Release will appear fixed as of 2012-11-19.

My tentative suggestion is to land now, uplift to Aurora soon after, and watch for issues over the next two weeks, but I defer to release drivers.

Comment 17

[Nick Alexander :nalexander [he/him]]

Testing notes: verifying that pickle files really are the cause of this:

1. Install official-branded m-a and beta-branded m-i.
2. Add Account, sync one App (org.mozilla.firefox_beta).
3. DO NOT Sync other App (org.mozilla.firefox).
4. Witness pickle in only one place:

$ adb shell run-as org.mozilla.firefox ls files
mozilla
$ adb shell run-as org.mozilla.firefox_beta ls files
mozilla
sync.account.json

5. Remove Account.
6. Witness pickle file removed:

$ adb shell run-as org.mozilla.firefox ls files
mozilla
$ adb shell run-as org.mozilla.firefox_beta ls files
mozilla

6. start Firefox and Firefox beta.
7. Observe "Set up Sync" box offered and Account not re-created.

8. Add account, check both Apps, force Sync both Apps.
9. Witness pickle in both places:

$ adb shell run-as org.mozilla.firefox_beta ls files
mozilla
sync.account.json
$ adb shell run-as org.mozilla.firefox ls files
mozilla
sync.account.json

10. Remove Account unsuccessfully.
11. Observe logs like:

    844:I FxSync(11287)               firefox_beta :: SyncAuthService :: Account named [e-mail] being removed; deleting saved pickle file 'sync.account.json'.
    845:I FxSync(11287)               firefox_beta :: SyncAuthService :: Account named [e-mail] being removed; deleting client record from server.
    881:I FxSync(11287)               firefox_beta :: ClientRecTerminator :: Deleted client record with GUID TczWCnYN231M from server.
    933:I FxSync(11435)               firefox :: AccountPickler :: Un-pickled Android account named [e-mail] (version 1, pickled at 82240684).

Comment 18

[Nick Alexander :nalexander [he/him]]

Testing notes: with fix in place for App owning Authenticator:

1. remove Account works (!) but leaves pickle file behind.

$ adb shell run-as org.mozilla.firefox_beta ls files
mozilla
$ adb shell run-as org.mozilla.firefox ls files
mozilla
sync.account.json

2. Starting App org.mozilla.firefox_beta offers Set up Sync.
3. Starting App org.mozilla.firefox unpickles Account.

Comment 19

[Nick Alexander :nalexander [he/him]]

tracking-* update: this builds on Bug 770785 and the followup to fix bustage, Bug 797249.  (Everything is related: clean up on Account deletion.)

The m-c patches for 770785 and 797249 apply cleanly to m-a.  On top of those two, the patch I've prepared against m-i for this applies with only a single edit.

I think it will be possible to make this apply without the earlier fixes, but how do we feel about uplifting 770785 and 797249 (to Aurora) at the same time as this?

Comment 20

[Richard Newman [:rnewman]]

itym Bug 787249.

I'm pretty happy with that dependency. Bug 770785 is a little more involved.

Is it a total conflict explosion without Bug 787249?

Comment 21

[Richard Newman [:rnewman]]

> Is it a total conflict explosion without Bug 787249?

I of course meant Bug 770785 here. *sigh*

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #21)
> > Is it a total conflict explosion without Bug 787249?
> 
> I of course meant Bug 770785 here. *sigh*

Yes, but it's a patch-conflict, not an idea-conflict.  Bug 770785 is fairly low risk, but let me have a crack at clawing it out and see what that looks like.  Will report back.

Comment 23

[Nick Alexander :nalexander [he/him]]

Attachment: Patch against m-i

Since we're looking to uplift, I'd rather have r+ on the m-i and m-a patches at the same time.  Please diff the two patches to see the changes necessary to land Bug 790931 on m-a without Bug 770785; they're really not that large.

Comment 24

[Nick Alexander :nalexander [he/him]]

Attachment: Patch against m-a

Comment 25

[Nick Alexander :nalexander [he/him]]

Attachment: Patch against m-i

Now with commit message!

Comment 26

[Nick Alexander :nalexander [he/him]]

Attachment: Patch against m-a

Comment 27

[Nick Alexander :nalexander [he/him]]

Attachment: Patch against m-a

Now with commit message!

Comment 28

[Richard Newman [:rnewman]]

Comment on attachment 665614 [details] [diff] [review]
Patch against m-a

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

This looks fine to me. Land the other on inbound and begin baking…

Comment 29

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/134946c72253

Comment 30

[Nick Alexander :nalexander [he/him]]

Comment on attachment 665614 [details] [diff] [review]
Patch against m-a

[Approval Request Comment]
Bug caused by (feature/regressing bug #):

I suppose this is fallout from Bug 769745 (pickle Android account to disk, used for making Sync not suck completely when Fennec is installed on SD card) interacting with Bug 761206 (support multiple Fennecs and Sync).

User impact if declined:

Users with both beta and release, or both aurora and nightly, will not be able to delete Sync account.

Testing completed (on m-c, etc.): 

With an official and beta branded m-i, and with an official branded m-a and a beta branded m-i.  All combinations with and without fix, to test interactions.

Risk to taking this patch (and alternatives if risky):

Discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=790931#c13.  This has had careful review, and really needs to land on nightly and aurora simultaneously to be tested.

String or UUID changes made by this patch:

None.

Comment 31

[Alex Keybl [:akeybl]]

(In reply to Nick Alexander :nalexander from comment #30)
> Risk to taking this patch (and alternatives if risky):
> 
> Discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=790931#c13.  This
> has had careful review, and really needs to land on nightly and aurora
> simultaneously to be tested.

Read and re-read comment 13. If there's any significant risk for sync users on a single version of Firefox, I don't think we should uplift a fix for deleting Sync accounts when multiple versions of Firefox are installed.

We'd instead wait till 18 was on Aurora and 19 on Nightly to confirm over a full cycle.

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/134946c72253

Comment 33

[Alex Keybl [:akeybl]]

Still waiting to hear back from you Nick, before approving.

Comment 34

[Nick Alexander :nalexander [he/him]]

(In reply to Alex Keybl [:akeybl] from comment #33)
> Still waiting to hear back from you Nick, before approving.

Sorry, akeybl, I misunderstood: I interpreted what you said to be the decision that we'd wait a cycle.

There is no significant risk for Sync users with a single version of Firefox.  Let's uplift.

Comment 35

[Nick Alexander :nalexander [he/him]]

tracking-* update: I'll land this on m-a tomorrow morning.

Comment 36

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ddf71cd3a720

I *think* that I do not update target milestone; release driver, please set the correct milestone if I am wrong.