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

RESOLVED FIXED in Firefox 17

Status

Android Background Services
Android Sync
P1
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: AdrianT, Assigned: nalexander)

Tracking

({relnote})

unspecified
mozilla18
ARM
Android
relnote

Firefox Tracking Flags

(firefox16-, firefox17+ fixed, fennec18+)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 660811 [details]
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
I'm unable to reproduce this on Android 4.0.4, and Android 4.1.1
Severity: critical → normal
Component: General → Android Sync
Product: Firefox for Android → Mozilla Services
Version: Trunk → unspecified

Comment 2

5 years ago
adrian, can you also try on a android 2.2 device?  could you reproduce on another gingerbread device?
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)
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!
(Reporter)

Comment 5

5 years ago
(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.

Updated

5 years ago
tracking-firefox16: --- → ?
(Assignee)

Comment 6

5 years ago
> 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.
Assignee: nobody → nalexander
Priority: -- → P1

Updated

5 years ago
tracking-firefox16: ? → +

Updated

5 years ago
tracking-firefox17: --- → +
(Assignee)

Comment 7

5 years ago
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.
Depends on: 792099
Summary: Unable to remove the sync account when there are multiple Firefox versions synced → Allow successful removal of Sync account when multiple Firefox versions are installed
(Assignee)

Comment 8

5 years ago
Update for tracking-firefox16: I developed a solution for this yesterday; today I will be device testing it.
If the fix is low-risk enough, we could uplift to aurora.
tracking-fennec: ? → 18+
(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.
tracking-firefox16: + → -
(Assignee)

Comment 11

5 years ago
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 nalexander+test0919@mozilla.com being removed; broadcasting secure intent org.mozilla.firefox_sync.accounts.SYNC_ACCOUNT_DELETED_ACTION.
     54:I FxSync(20595)               firefox :: SyncAccountDeletedService :: Sync account named nalexander+test0919@mozilla.com being removed; deleting saved pickle file 'sync.account.json'.
     55:I FxSync(20536)               firefox_beta :: SyncAccountDeletedService :: Sync account named nalexander+test0919@mozilla.com being removed; deleting saved pickle file 'sync.account.json'.
     56:I FxSync(20595)               firefox :: SyncAccountDeletedService :: Account named nalexander+test0919@mozilla.com being removed; deleting client record from server.
     81:I FxSync(20536)               firefox_beta :: SyncAccountDeletedService :: Account named nalexander+test0919@mozilla.com 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.

Updated

5 years ago
QA Contact: twalker
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
tracking-* update: waiting on review.
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.
(Assignee)

Comment 17

5 years ago
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 nalexander+test0919@mozilla.com being removed; deleting saved pickle file 'sync.account.json'.
    845:I FxSync(11287)               firefox_beta :: SyncAuthService :: Account named nalexander+test0919@mozilla.com 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 nalexander+test0919@mozilla.com (version 1, pickled at 82240684).
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Comment 19

5 years ago
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?
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?
> Is it a total conflict explosion without Bug 787249?

I of course meant Bug 770785 here. *sigh*
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
Created attachment 665602 [details] [diff] [review]
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.
Attachment #665602 - Flags: review?(rnewman)
(Assignee)

Comment 24

5 years ago
Created attachment 665603 [details] [diff] [review]
Patch against m-a
Attachment #665603 - Flags: review?(rnewman)
(Assignee)

Comment 25

5 years ago
Created attachment 665610 [details] [diff] [review]
Patch against m-i

Now with commit message!
Attachment #665602 - Attachment is obsolete: true
Attachment #665602 - Flags: review?(rnewman)
(Assignee)

Updated

5 years ago
Attachment #665610 - Flags: review?(rnewman)
(Assignee)

Comment 26

5 years ago
Created attachment 665613 [details] [diff] [review]
Patch against m-a
Attachment #665603 - Attachment is obsolete: true
Attachment #665603 - Flags: review?(rnewman)
Attachment #665613 - Flags: review?(rnewman)
(Assignee)

Comment 27

5 years ago
Created attachment 665614 [details] [diff] [review]
Patch against m-a

Now with commit message!
Attachment #665613 - Attachment is obsolete: true
Attachment #665613 - Flags: review?(rnewman)
Attachment #665614 - Flags: review?(rnewman)
Attachment #665610 - Flags: review?(rnewman) → review+
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…
Attachment #665614 - Flags: review?(rnewman) → review+
(Assignee)

Comment 29

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/134946c72253
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla18
(Assignee)

Comment 30

5 years ago
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.
Attachment #665614 - Flags: approval-mozilla-aurora?
(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.

Updated

5 years ago
Keywords: relnote
https://hg.mozilla.org/mozilla-central/rev/134946c72253
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Still waiting to hear back from you Nick, before approving.
(Assignee)

Comment 34

5 years ago
(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.

Updated

5 years ago
Attachment #665614 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 35

5 years ago
tracking-* update: I'll land this on m-a tomorrow morning.
(Assignee)

Comment 36

5 years ago
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.
status-firefox17: --- → fixed
Component: Android Sync → Android Sync
Product: Mozilla Services → Android Background Services

Updated

3 years ago
QA Contact: twalker
You need to log in before you can comment on or make changes to this bug.