Closed
Bug 1145896
Opened 10 years ago
Closed 10 years ago
ClientsDatabaseAccessor contains stale data
Categories
(Android Background Services Graveyard :: Firefox Accounts, defect)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: mcomella, Assigned: vivek, Mentored)
References
Details
(Keywords: verifyme)
Attachments
(1 file)
(deleted),
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
I'm not sure of exact STR, but I had sync enabled with multiple synced devices, removed the Firefox Account from the device, and ClientsDatabaseAccessor.clientsCount() returned 16 (this is in the context of bug 1122302, which uses ClientsDatabaseAccessor).
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 1•10 years ago
|
||
This caused bug 1145897 (but I'm going to use a workaround there).
Comment 2•10 years ago
|
||
NI to me to complete mentor information.
Assignee: nobody → nalexander
Mentor: nalexander, rnewman
Flags: needinfo?(nalexander)
Comment 3•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #2)
> NI to me to complete mentor information.
What this ticket needs is the background services databases cleaned when Firefox Accounts are removed. That will mean adding, around [1], code that cleans the databases created at [2] and [3].
AndroidBrowserHistoryDataExtender looks like it doesn't track profile or account at all; we should wipe the table entirely when any account is removed. (And trust that there's only ever one account at any time.)
Clients database tracks profile, but not really Account. We should wipe the tables entirely when any account is removed here, as well.
[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/receivers/FxAccountDeletedService.java#74
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/AndroidBrowserHistoryDataExtender.java
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/ClientsDatabase.java
Flags: needinfo?(nalexander)
Comment 4•10 years ago
|
||
To test this, connect to a Firefox Account, make sure other devices connect to the account, and sync. You should see additional devices in the Synced Tabs display and in the Send Tab list (which uses the ClientsTable, IIRC). Remove the Account, and then connect to another one (with no devices connected). Before fixing this ticket, you should see the old devices (from the other account); after fixing this ticket, the old devices should be purged when the account is removed.
Assignee | ||
Comment 5•10 years ago
|
||
Stale data from client and historyDataExtender database flushed in AccountDeletedService.
Attachment #8589754 -
Flags: review?(nalexander)
Comment 6•10 years ago
|
||
Updated•10 years ago
|
Assignee: nalexander → vivekb.balakrishnan
Status: NEW → ASSIGNED
Comment 7•10 years ago
|
||
Comment on attachment 8589754 [details] [diff] [review]
1145896.patch
Review of attachment 8589754 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. I landed with some try/catch blocks.
Attachment #8589754 -
Flags: review?(nalexander) → review+
Comment 8•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 9•10 years ago
|
||
mcomella: here's how I tested this. I created an FxA, synced against a few remote clients, and verified that using the Share Plane > Send to Other Device from within Fennec had a list of external clients. Then I deleted the FxA. I then tried the Share Plane again, but of course there is no account so there is no list of clients. (This is expected, right?) So I created a new FxA with no other remote clients, and verified that using the Share Plane > Send to Other Device was present (and empty). (Fuzzy on that.) I then added another device to the new FxA, synced, and verified that only the new device was present in the Other Device list.
Could you test your scenario and get QA involved where needed?
mcomella: on an unrelated note, all use of the Sync-specific clients database can and should transition to the TabsProvider, which is an honest ContentProvider and has not the same issues. That's a little technical debt to dig out from; it's not urgent.
Updated•10 years ago
|
Flags: needinfo?(michael.l.comella)
Reporter | ||
Comment 10•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #9)
> I then tried the Share Plane again, but of course there is no
> account so there is no list of clients. (This is expected, right?)
When you say there is no list of clients, do you mean you get an empty dialog, or do you get a "No connected devices" toast? If the latter, I filed bug 1159005 because this is not expected.
> created a new FxA with no other remote clients, and verified that using the
> Share Plane > Send to Other Device was present (and empty). (Fuzzy on
> that.)
It should not be present with no other devices (bug 1159005 again?).
> I then added another device to the new FxA, synced, and verified
> that only the new device was present in the Other Device list.
My scenario would be the same except I would in the end add a different sync account with devices to ensure the list changes - previously, I would get the same list as the initial account.
> Could you test your scenario and get QA involved where needed?
I'll write out clearer steps and add the appropriate flags.
> mcomella: on an unrelated note, all use of the Sync-specific clients
> database can and should transition to the TabsProvider
Should this take the place of bug 1132730?
Flags: needinfo?(michael.l.comella) → needinfo?(nalexander)
Comment 11•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #10)
> (In reply to Nick Alexander :nalexander from comment #9)
> > I then tried the Share Plane again, but of course there is no
> > account so there is no list of clients. (This is expected, right?)
>
> When you say there is no list of clients, do you mean you get an empty
> dialog, or do you get a "No connected devices" toast? If the latter, I filed
> bug 1159005 because this is not expected.
Empty dialog.
> > created a new FxA with no other remote clients, and verified that using the
> > Share Plane > Send to Other Device was present (and empty). (Fuzzy on
> > that.)
>
> It should not be present with no other devices (bug 1159005 again?).
Not clear.
> > I then added another device to the new FxA, synced, and verified
> > that only the new device was present in the Other Device list.
>
> My scenario would be the same except I would in the end add a different sync
> account with devices to ensure the list changes - previously, I would get
> the same list as the initial account.
>
> > Could you test your scenario and get QA involved where needed?
>
> I'll write out clearer steps and add the appropriate flags.
>
> > mcomella: on an unrelated note, all use of the Sync-specific clients
> > database can and should transition to the TabsProvider
>
> Should this take the place of bug 1132730?
No, just making sure you're aware. The more we build on top of Sync's internal databases, the harder the technical debt is to pay off in the future.
Flags: needinfo?(nalexander)
Reporter | ||
Comment 12•10 years ago
|
||
STR:
* Prepare two sync accounts - each one with a different list of synced devices (so you can tell the difference between them)
* Connect the browser to one account - verify the "Send to other devices" menu appears with the devices you'd expect. You may need to wait for (or force) the device to sync before this menu item appears.
* Remove the account. Verify the "Send to other devices" menu item does not appear or if it does (bug 1159005), it shows a toast rather than showing the previous sync account's device list.
* Add the second account. Verify "Send to other devices" appears and has a different device list from the first account.
For me, the quick share menu state keeps the "Send to other devices" icon even after I remove the account, however, when resolving the activity, it skips it (thus choosing a different activity). The full share menu did not have this problem. I'm not sure how to repro indepedent of bug 1159005 so I did not file.
Keywords: verifyme
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #11)
> Empty dialog.
Filed bug 1159019.
> > > created a new FxA with no other remote clients, and verified that using the
> > > Share Plane > Send to Other Device was present (and empty). (Fuzzy on
> > > that.)
> >
> > It should not be present with no other devices (bug 1159005 again?).
>
> Not clear.
The "Send to other devices" menu item should not be present if you have no other devices connected to your sync account.
> > > mcomella: on an unrelated note, all use of the Sync-specific clients
> > > database can and should transition to the TabsProvider
Filed bug 1159020.
You need to log in
before you can comment on or make changes to this bug.
Description
•