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

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: Kanchan Kumari QA, Assigned: eoger)

Tracking

53 Branch
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox54 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

9 months ago
Created attachment 8829155 [details]
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.
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.
Flags: needinfo?(rnewman)
Flags: needinfo?(rnewman) → needinfo?(gkruglov)
Created attachment 8830480 [details]
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

9 months ago
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
Flags: needinfo?(gkruglov)
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?
Flags: needinfo?(rfkelly)
Flags: needinfo?(kkumari)
Priority: -- → P2
(Reporter)

Comment 5

9 months ago
> 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)").
Flags: needinfo?(kkumari)

Comment 6

9 months ago
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.
Flags: needinfo?(rfkelly) → needinfo?(gkruglov)

Comment 7

9 months ago
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

9 months ago
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

9 months ago
(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.
Flags: needinfo?(gkruglov)

Updated

9 months ago
See Also: → bug 1335514
Comment hidden (mozreview-request)
(Assignee)

Comment 11

9 months ago
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

9 months ago
mozreview-review
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 :)
Attachment #8833072 - Flags: review?(markh) → review+

Comment 13

9 months ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a1d740af9d6
Alternate wording for Sync new device connected notification when device name missing. r=markh
(Assignee)

Updated

9 months ago
Assignee: nobody → eoger
Priority: P2 → P1

Comment 14

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a1d740af9d6
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Reporter)

Updated

9 months ago
status-firefox54: fixed → verified
You need to log in before you can comment on or make changes to this bug.