Closed Bug 1579445 Opened 3 months ago Closed 3 months ago

Newly added device doesn't show up in the list of send tab devices

Categories

(Firefox :: Sync, defect)

71 Branch
Desktop
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox69 --- wontfix
firefox70 --- verified
firefox71 --- verified

People

(Reporter: jonalmeida, Assigned: eoger)

Details

Attachments

(1 file)

STR

  1. Have sync enabled on a desktop.
  2. Sync a new device (e.g. Fenix, Fire TV)
  3. Right-click a tab and try to send it to the newly sync'd device.

Expected

  • The device is available in the list of devices; a tab can be sent.

Actual

  • You see a notification on desktop that a new sync device was added.
  • The device is not available to send a tab.

You see a notification on desktop that a new sync device was added.

This indicates that Desktop has successfully received a push notification telling it about the new device.

We dispatch an fxaccounts:device_connected observer notification here, and we observe it in order to show the notification UI here. But AFAICT we don't use it as an opportunity to refresh the list of peer devices.

The list of send-tab targets comes from the sync clients engine here, which keeps an internal cache of peer devices. I believe we'll need to arrange for the Sync clients engine to observe the fxaccounts:device_connected notification and either invalidate or update its cache. I see that there is some amount of observer-notification plumbing in that file already.

:eoger does that all sound right to you?

Flags: needinfo?(eoger)

Also, surfacing from slack:

I believe Fire TV folks want this in time for Skyline (october) so if the change isn't complex,
would it be possible to uplift it to beta to make it on time?

This is all very complex :( IIUC, we should be refreshing the client list every time the "clients" engine syncs - and browser-sync.js attempts to arrange for that to happen each time the list of send-tab targets is shown - although we show what we currently have, then sync, then refresh the view. So ISTM that in theory it should work now (although note that we will not refetch new devices if we tried 10 seconds ago, and that seems possible here - the user is expecting a new device, so closes and re-opens the window hoping it will appear - but the 10 seconds prevents us actually checking again)

A lot of this complexity can probably die - IIRC, much of this complexity exists because we needed devices to have both fxa records and client collection records. However, that's no longer true, so we can probably arrange to keep the clients engine out of this entire process - but I think we need to work out why it's not currently working first.

(at least I think the above is all correct; Ed will correct me if I'm wrong!)

Thanks Ryan and Mark for the investigations.

New clients such as Fennec or Desktop also send a sync:collection_changed push message on first sync which as a side-effect refreshes the FxA devices lists of the other targets.
I don't think we should implement this change on Fennec as it is very much a clients-collection-first relic of the past. Let's rely on the device_connected notification to refresh our cached device list.

IIRC, much of this complexity exists because we needed devices to have both fxa records and client collection records. However, that's no longer true, so we can probably arrange to keep the clients engine out of this entire process - but I think we need to work out why it's not currently working first.

Sadly we need to keep that complexity here for a little while until Fennec is retired: it does not support FxA commands and relies on the clients collection.

TL;DR:

  1. We should address the immediate problem by simply updating the FxA devices list upon reception of device_connected.
  2. (Once Fennec is retired OR we're ok with killing Send Tab support for Fennec) remove the cruft of the Desktop UI and the caching of FxA Devices.
Flags: needinfo?(eoger)
Assignee: nobody → eoger
Status: NEW → ASSIGNED

(In reply to Edouard Oger [:eoger] from comment #4)

TL;DR:

  1. We should address the immediate problem by simply updating the FxA devices list upon reception of device_connected.

I'd still like to understand why the client sync triggered by showing the list of send tab targets doesn't cause an fxa device refresh?

  1. (Once Fennec is retired OR we're ok with killing Send Tab support for Fennec) remove the cruft of the Desktop UI and the caching of FxA Devices.

I still think there's some unpicking we can do here - Fennec is a sync15 client, so has a client record. It's still not quite clear why supporting Fennec requires a fully up-to-date fxa device list (but I agree it's not a huge deal)

Flags: needinfo?(eoger)

I'd still like to understand why the client sync triggered by showing the list of send tab targets doesn't cause an fxa device refresh?

This is because it it only works when sync is "loading".

Note that my patch suffers from a race condition: when the "device_connected" push notification has been received, there's a high probability the new device hasn't registered its send tabs keys yet (device.availableCommands is empty), therefore will not show in the Send Tab menu despite the refresh.

What we could arrange instead is refresh the devices list every time we open the "send tab" menu: note this will not work for the right click context menu (impossible to refresh once it's opened I believe).

Flags: needinfo?(eoger)

Alternatively if we believe this will generate too much /device API traffic, we could only do that "refresh on menu open" when setting a "dirty" flag upon reception of device_connected: the user's probably not fast enough to open the send tab menu in the ["new device notification", "send tab command registered"] window.

Attachment #9091440 - Attachment description: Bug 1579445 - Refresh the FxA device list on device connected. → Bug 1579445 - Refresh the FxA device list on Send Tab menu open.
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a3b2a7742fb
Refresh the FxA device list on Send Tab menu open. r=markh
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

Send tab in nightly is currently not working for the Fire TV team to verify this works for them. Once they are able to test and verify the change works for them, is it possible to upstream this to beta in order to meet their skyline requirements?

Flags: needinfo?(eoger)

Yes, I believe we should uplift asap.

Flags: needinfo?(eoger)

NI self to verify when Fire TV has confirmed this works for them.

Flags: needinfo?(jonalmeida942)

FYI, Fx70 RC week is next week. If this is going to be uplifted, it needs a request ASAP.

Flags: needinfo?(eoger)

Jon is on PTO and the fix works with Nightly, so let's just uplift.

Flags: needinfo?(eoger)

Comment on attachment 9091440 [details]
Bug 1579445 - Refresh the FxA device list on Send Tab menu open.

Beta/Release Uplift Approval Request

  • User impact if declined: Firefox desktop users won't see a Fire TV and Fenix Send Tab target when they connect their device.
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Small logic change.
  • String changes made/needed:
Attachment #9091440 - Flags: approval-mozilla-beta?

I wish I had seen this or there'd been an uplift request much earlier in the beta 70 cycle - please don't hesitate to ask or bring bugs to relman's attention.

But, let's take the patch and see if we can verify as best possible in beta 14.

Attachment #9091440 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

NI self to verify it when beta 14 will be released.

Flags: needinfo?(andrei.bodea)

Verified as fixed on the 70.0b14 and 71.0a1(latest nightly). Note that I've sent more than 15 tabs from beta 14 and the latest nightly, everything worked as expected, tabs being received on the Fire TV.
I will also, clear the NI for Jon.

Status: RESOLVED → VERIFIED
Flags: needinfo?(jonalmeida942)
Flags: needinfo?(andrei.bodea)
You need to log in before you can comment on or make changes to this bug.