830270 - Removing Sync account does not remove synced tabs

Status: VERIFIED FIXED

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

Description

[Demetrius S.]

Due to my Nexus S still showing up under my synced tabs even though it wasn't there for months I decided to delete my sync account and recreate a new one. After deleting it and setting it up again on my laptop I go over to my Nexus 7. I have to manually delete sync, and sign up again with my laptop. The only problem is that it still lists the devices that I haven't owned for months and nothing I attempt removes them.

Comment 1

[Nick Alexander :nalexander [he/him]]

Hmm, there are a few things I can think of that might be involved:

1) Deleting sync might mean that remote records in the tabs DB on each device are never purged.  Cleaning DBs on sync removal and cleaning the tabs DB periodically from Fennec would address this.

2) We might have a clients record TTL issue.  Old devices are supposed to drop out of the list of clients after 1 or 3 weeks (Android or desktop, respectively).  We could verify that clients records are, in fact, being purged.

Thanks for the report.  I will try to look at 1) in the near future; QA could you test 2)?

Comment 2

[Nick Alexander :nalexander [he/him]]

Harshit, are you interested in addressing this?  There are a bunch of ways you could do this, but I think there is already an "Android Accounts changed" observer somewhere in Fennec.  If that observer finds that the last Sync account is deleted, it could try to delete all remote tabs in the database.

Comment 3

[Nick Alexander :nalexander [he/him]]

So, at

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AboutHomeContent.java#143

we register a callback that updates the layout of about:home when sync accounts are updated.  We should make sure this removes the Sync portion of about:home (I think it does) and then we should also try to be clever about cleaning the tabs and clients DB.

I suggest writing some code that carefully cleans the tabs and clients DB, then trying to tie it in to this callback.

Comment 4

[Tsai]

Hello.  I was wondering if my partner and I could work on this issue for a class project.  Please let me know if this bug is still open.  Thank you so much.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Tsai from comment #4)
> Hello.  I was wondering if my partner and I could work on this issue for a
> class project.  Please let me know if this bug is still open.  Thank you so
> much.

Definitely still open!  Read through the comments and see where you get; you can leave questions on the ticket and I'll reply as quickly as I can.  Thanks for helping.

Comment 6

[Nick Alexander :nalexander [he/him]]

Hi Lala, any progress here?

Comment 8

[Nick Alexander :nalexander [he/him]]

Vivek: this might be a good next bug for you; involved, but you're providing great patches on other tickets...

Comment 9

[Vivek Balakrishnan[:vivek]]

I'm interested in working on this.

Comment 10

[Aaron Train [:aaronmt]]

Feel free to work on this and to attach a patch and we'll happily review it for feedback. There seems to be several interested parties working on this so don't mind the already assigned member.

Comment 11

[Nick Alexander :nalexander [he/him]]

AaronMT: I invited Vivek to work on this because he's been doing great work on some other tickets.  Clearing the assigned fields since Lala hasn't commented in many months.

Comment 12

[Nick Alexander :nalexander [he/him]]

There are a few parts to this.

1. Add a function that deletes the entire clients database (corresponds to remote Sync devices) and the non-local tabs (the tabs from remote devices) from the tabs database.  Non-local tabs have client_guid == null; see http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserContract.java#298.

See http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/FennecTabsRepository.java#272 for how to delete things from the databases (but don't try to use that wipe function directly, since it's tied up in other machinery.)

2. Add static member functions to both Fx- and Sync- AccountDeletedService to invoke your database cleaning functions, and add them to the list of things invoked:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/receivers/FxAccountDeletedService.java#57
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/receivers/SyncAccountDeletedService.java#76

3. Test with a new Sync account.  (Testing with old sync is tricky right now :)

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: Link to Github pull-request: https://github.com/mozilla-services/android-sync/pull/467

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8446221 [details] [review]
Link to Github pull-request: https://github.com/mozilla-services/android-sync/pull/467

Really nice work, vivek.  Awesome to see an *ooooold* ticket get some love.  I've left a ton of tiny comments on the Github PR, and a few larger remarks.  Flag me for re-review when it's ready.

Thanks!  Great job!

Comment 15

[Vivek Balakrishnan[:vivek]]

Hi Nick,
I've updated the pull request with feedback comments.

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to vivek from comment #15)
> Hi Nick,
> I've updated the pull request with feedback comments.

Sorry, I don't have time to get to this before I leave on vacation tomorrow.  I'll be back mid-August.  In the meantime, perhaps rnewman can look at this.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8446221 [details] [review]
Link to Github pull-request: https://github.com/mozilla-services/android-sync/pull/467

Looks good; just nits.

Comment 18

[Vivek Balakrishnan[:vivek]]

nits corrected and pull request updated

Comment 19

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/7614df67484e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7614df67484e

Comment 21

[Nick Alexander :nalexander [he/him]]

rnewman landed this, but didn't include a file (for reasons unknown):

https://hg.mozilla.org/integration/fx-team/rev/d7585d5adb6a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7585d5adb6a