Closed
Bug 1332858
Opened 6 years ago
Closed 6 years ago
Notification display device name as null, when Android device (phone) added to Sync account
Categories
(Firefox :: Sync, defect, P1)
Tracking
()
RESOLVED
FIXED
Firefox 54
People
(Reporter: kkumari, Assigned: eoger)
References
Details
Attachments
(3 files)
214.50 KB,
image/png
|
Details | |
30.31 KB,
image/png
|
Details | |
Bug 1332858 - Alternate wording for Sync new device connected notification when device name missing.
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
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•6 years ago
|
||
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)
Updated•6 years ago
|
Flags: needinfo?(rnewman) → needinfo?(gkruglov)
Comment 2•6 years ago
|
||
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•6 years 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)
Comment 4•6 years ago
|
||
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•6 years 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•6 years 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•6 years 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•6 years 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•6 years 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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•6 years 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•6 years 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•6 years 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•6 years ago
|
Assignee: nobody → eoger
Priority: P2 → P1
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9a1d740af9d6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Reporter | ||
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•