1332858 - Notification display device name as null, when Android device (phone) added to Sync account

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Kanchan Kumari QA]

Attachment: AndroidDeviceName.PNG

STR:
1. Launch firefox with an existing sync account on a desktop
2. Sign in to Sync on Android device (phone) with the same account

Expected: Firefox Sync notification pops up on Deskotop as "This computer is now syncing with <phone name>"

Actual : Firefox Sync notification pops up on Deskotop as "This computer is now syncing with (null)"

Please note that this behavior was seen with LG Nexus and one more Android phone.

Comment 1

[Lina Butler [:lina]]

Richard, is it possible for Fennec to have a null device name, or is something getting lost along the way? AFAIK, Desktop just uses whatever it gets in the push notification from FxA. I don't see the parenthesized "(null)" string anywhere, though.

Comment 2

[Ryan Feeley [:rfeeley]]

Attachment: null.png

I got it today too when I sent a tab from home (Firefox 54.0a1, Android) and it arrived in Nightly at work. I haven't sent from there in a while, but was just testing it out.

Comment 3

[:Grisha Kruglov]

I'm seeing this too when connecting a device.

Looking through the code, as far as I can tell, it shouldn't be null when leaving a device.

We clobber together a local client record at [0], and part of that process is fetching a name from persisted settings or generating it at [1].
So it shouldn't by null, and I think we would have seen other symptoms show up at some point prior.

[0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java#456-457
[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/SharedPreferencesClientsDataDelegate.java#74-83

Comment 4

[Lina Butler [:lina]]

From triage: This happens on the very first sync. The Android client is definitely sending its device name in the client record, but Desktop uses the device name from the push payload. This message comes from the FxA auth server. (Is Android not registering correctly with the device manager?) Next step is to reproduce. We might also need to make the message more generic, if we can't determine the device name.

Kanchan, do you see the correct device name in https://accounts.firefox.com/settings/clients? Ryan, Mark tells me FxA synthesizes a device name; is that done at registration time, or just in the UI?

Comment 5

[Kanchan Kumari QA]

> Kanchan, do you see the correct device name in
> https://accounts.firefox.com/settings/clients? 

Yes Kit, I see correct Android device(phone) name in https://accounts.firefox.com/settings/clients but it still shows as null in notification message ("This computer is now syncing with (null)").

Comment 6

[Ryan Kelly [:rfkelly]]

Does Android register its FxA device record before it starts syncing?  Specifically, before attempting to call /certificate/sign on the FxA server?

One possibility here is that FxA is creating a "placeholder" device record before Android registers the real one.  This placeholder record reserves a deviceId but doesn't specify a name, type, etc.  I'll have to dig into the flow, but it's possible that this causes the notification to go out with a null value in the "name" field.  Leaving ni? myself to follow up on that.

Comment 7

[Ryan Kelly [:rfkelly]]

Yep, I'm pretty sure the "(null)" is coming from here:

  https://dxr.mozilla.org/mozilla-central/rev/71224049c0b52ab190564d3ea0eab089a159a4cf/xpcom/string/nsTextFormatter.cpp#532

Which AFAICT is what would happen if the FxA push notification does not contain a `deviceName` field in its payload body.  FxA creating a placeholder device record is the most likely explanation.

FWIW the logic that we use in FxA is this:

* If we get a request to /certificate/sign, and
* That sessionToken does not have a device id associated with it, and
* There's no ?service= query parameter, or there's service=sync, then
* Create a device record for that sessionToken, because it's pretty definitely a device using sync

These placeholder record are created to help deal with older clients that do not support native device registration.  They're important for metrics purposes, and to ensure that the device shows up correctly in the FxA devices view.

We could try a couple of things here:

* Have Android register its device record before it attempts a /certification/sign (and document this requirement for future consumers of the API)
* Have desktop check for a deviceName field in the payload and handle things gracefully if it's missing
* Have FxA synthesize a device name so that we avoid sending push messages with no deviceName in the body.

Probably we should do all three, but the third seems like the easiest way to resolve this bug in the short term.

Comment 8

[Ryan Kelly [:rfkelly]]

Here's an FxA PR which should result in us sending a sensible default value when creating placeholder device records:

  https://github.com/mozilla/fxa-auth-server/pull/1633

It's still theoretically possible that the deviceName could come through as an empty string, so receiving code should be prepared to handle this case by showing "Unnamed device" or similar.

Comment 9

[:Grisha Kruglov]

(In reply to Ryan Kelly [:rfkelly] from comment #6)
> Does Android register its FxA device record before it starts syncing? 
> Specifically, before attempting to call /certificate/sign on the FxA server?

Android registers device record after calling /certificate/sign, which matches your hypothesis.

> * Have Android register its device record before it attempts a /certification/sign (and document this 
> requirement for future consumers of the API)
> * Have desktop check for a deviceName field in the payload and handle things gracefully if it's missing
> * Have FxA synthesize a device name so that we avoid sending push messages with no deviceName in the body.

> Probably we should do all three, but the third seems like the easiest way to resolve this bug in the short
> term.

I've filed Bug 1335514 to track Android changes, which don't seem particularly difficult.

Comment 10

[Edouard Oger [:eoger]]

Attachment: Bug 1332858 - Alternate wording for Sync new device connected notification when device name missing.

Review commit: https://reviewboard.mozilla.org/r/109294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109294/

Comment 11

[Edouard Oger [:eoger]]

Here's a patch for desktop, in case we don't receive the deviceName we display "This computer is now syncing with a new device."

Comment 12

[Mark Hammond [:markh] [:mhammond] - PTO until April 20]

Comment on attachment 8833072 [details]
Bug 1332858 - Alternate wording for Sync new device connected notification when device name missing.

https://reviewboard.mozilla.org/r/109294/#review110928

I was going to ignore this part of the bug as almost no other parts of our UI handles the fact a device name might be empty. However, those other parts care more about the sync "client" concept rather than the FxA "device" concept, so I guess it's fine as a special case, even though we intend removing that special case at some point.

So yeah, thanks :)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a1d740af9d6
Alternate wording for Sync new device connected notification when device name missing. r=markh

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a1d740af9d6