Existing contacts are not shown if the Loop panel is opened before registration happens

VERIFIED FIXED in Firefox 34

Status

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: standard8, Assigned: mikedeboer)

Tracking

unspecified
mozilla36
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-moztrap -
qe-verify +

Firefox Tracking Flags

(firefox33 unaffected, firefox34+ verified, firefox35+ verified, firefox36 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
STR:

1) Sign-in and create a contact
2) Restart Firefox
3) Wait about 10-20 seconds
4) Open the panel and select contact list

=> Contact is shown as expected.

5) Restart Firefox
6) Open the panel straight away and select the contact list

=> Contacts are not shown, nor do they get displayed later on.
Assignee

Comment 1

5 years ago
I'll investigate this.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Assignee

Comment 2

5 years ago
Ok, this is a race condition - the database name is changed while an operation is in progress. I'm putting a patch up soon.
Comment on attachment 8503164 [details] [diff] [review]
Patch v1: switch to a different database in the 'userProfile' getter to always be in sync with the correct state

Review of attachment 8503164 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/MozLoopService.jsm
@@ +1398,5 @@
> +    let profile = getJSONPref("loop.fxa_oauth.tokendata") &&
> +                  getJSONPref("loop.fxa_oauth.profile");
> +    let newName = profile ? profile.uid : null;
> +    if (LoopStorage.databaseName != newName) {
> +      LoopStorage.switchDatabase(newName);

This is pretty wonky here. I think it can already be confusing that the getter may return null if the tokendata pref is blank but the profile pref is present.

But having a getter cause a database switch? It seems like this is possibly a convenient place to put this but it doesn't seem like the right place to put it.

This can have the negative effect down the road of random callers requesting the userProfile just for the side effects.

Besides the nature of this fix, how does this change fix the problem? Where, outside of notifyStatusChanged, does the database need to be changed?
Attachment #8503164 - Flags: review?(jaws) → review-
If anything, the fxAOAuthTokenData setter may be a better place to switch the database. We may update the profile multiple times but it will still be the same account. I don't think we'll want to lose the database in that instance.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> If anything, the fxAOAuthTokenData setter may be a better place to switch
> the database. We may update the profile multiple times but it will still be
> the same account. I don't think we'll want to lose the database in that
> instance.

The uid will be the same in that case though. When fxAOAuthTokenData is set, access to the profile's UID may not be e.g. during normal login.
Assignee

Comment 7

5 years ago
I forgot to add a proper explanation about how I got to this solution, my apologies.

The problem is that MozLoopService.userProfile now _always_ returns the currently logged in profile data, so the UI state always shows the currently signed in user.
The fxAOAuthTokenData setter is only invoked when the profile actually changes to 1) the same profile login 'refresh', 2) logout - null or 3) signin as a different user. Similarly, notifyStatusChanged() is invoked once some HAWK request has finished or upon status change.
This renders the fxAOAuthTokenData setter and notifyStatusChanged methods not particularly suited for making sure the database is set to the correct one, according to the tokendata.

Since, at this point, the only place where the tokendata is retrieved is in MozLoopService.userProfile, it seemed to me that this is the only place where I can set the database correctly and as early as possible. I don't think we'd want the contacts list to be loaded as late as a HAWK request returning and setting the fxAOAuthTokenData to the exact same data as it already is.

This leads to my second problem; notifyStatusChanged() is invoked quite a number of times, but I don't want to reload the contacts list all the time. So what I do is see if the `uid` field changed, compared to the current userProfile, and based on that conditional refresh the list or do nothing at all. Since we're persisting the tokendata now, the `uid` doesn't change, so how will the UI know when to refresh the contacts list?
Flags: needinfo?(jaws)
Assignee

Comment 8

5 years ago
I think I just came up with a better solution.
Flags: needinfo?(jaws)
Assignee

Comment 10

5 years ago
Comment on attachment 8504622 [details] [diff] [review]
Patch v1.1: switch to a different database if a userProfile is active during the first mozLoop.contacts access

Review of attachment 8504622 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/MozLoopAPI.jsm
@@ +12,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "CommonUtils",
> +                                        "resource://services-common/utils.js");
> +XPCOMUtils.defineLazyModuleGetter(this, "LoopContacts",
> +                                        "resource:///modules/loop/LoopContacts.jsm");

I made these lazy too, because, well, they should've been from the start.
Assignee

Comment 11

5 years ago
Comment on attachment 8504622 [details] [diff] [review]
Patch v1.1: switch to a different database if a userProfile is active during the first mozLoop.contacts access

Matt, since Jared is less available due to TRIBE activities, can you review this patch?
Attachment #8504622 - Flags: review?(jaws) → review?(MattN+bmo)
Iteration: 35.3 → 36.1
Comment on attachment 8504622 [details] [diff] [review]
Patch v1.1: switch to a different database if a userProfile is active during the first mozLoop.contacts access

Review of attachment 8504622 [details] [diff] [review]:
-----------------------------------------------------------------

A simple test would have been good but I'll leave it up to you.

::: browser/components/loop/MozLoopAPI.jsm
@@ -5,5 @@
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
> -Cu.import("resource://services-common/utils.js");

Revert this since it's imported many times already at startup. This file is normally just imported.

@@ +228,5 @@
> +
> +        // Make a database switch when a userProfile is active already.
> +        let profile = MozLoopService.userProfile;
> +        if (profile) {
> +          LoopStorage.switchDatabase(profile.uid);

This combined with the existing notifyStatusChanged call should work.
Attachment #8504622 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/30af3afe667e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Pauly -- Can you verify this in Nightly?  (It should have been in Thursday's Nightly since it landed Wednesday.)
Flags: needinfo?(paul.silaghi)
backlog: --- → Fx34+
Comment on attachment 8504622 [details] [diff] [review]
Patch v1.1: switch to a different database if a userProfile is active during the first mozLoop.contacts access

Approval Request Comment
[Feature/regressing bug #]:Contacts for Hello
[User impact if declined]: User won't be able to see or use his contacts and can't make direct calls
[Describe test coverage new/current, TBPL]: landed on m-c, manual testing
[Risks and why]: No risk outside of Hello (and a small patch).  Without this patch the user could not use direct calling and would think the service failed
[String/UUID change made/needed]: No strings
Attachment #8504622 - Flags: approval-mozilla-beta?
Attachment #8504622 - Flags: approval-mozilla-aurora?
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:
Tracking for uplift to Fx34 and Fx35 -- See Comment 16
Verified fixed 36.0a1 (2014-10-16) Win 7, OS X 10.9.5
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
Comment on attachment 8504622 [details] [diff] [review]
Patch v1.1: switch to a different database if a userProfile is active during the first mozLoop.contacts access

Beta+
Aurora+
Attachment #8504622 - Flags: approval-mozilla-beta?
Attachment #8504622 - Flags: approval-mozilla-beta+
Attachment #8504622 - Flags: approval-mozilla-aurora?
Attachment #8504622 - Flags: approval-mozilla-aurora+
Tested and good on Aurora nightly build; windows and linux.
Probably related - after the browser restarts, a user can't be called in the first 10 seconds
Flags: needinfo?(mdeboer)
Pauly -- Can you file a separate bug?  A call can't be placed until the server knows that the user is there. We can provide user (UX) feedback that we are waiting to register with the server.
Assignee

Updated

5 years ago
Flags: needinfo?(mdeboer)
Paul, can you please make sure this is verified fixed in the next Beta?
Flags: needinfo?(paul.silaghi)
Flags: in-testsuite?
Flags: in-moztrap-
QA Contact: anthony.s.hughes → paul.silaghi
Verified fixed FF 34b2 Win7 x64
Flags: needinfo?(paul.silaghi)
Reporter

Comment 26

4 years ago
Clearing in-testsuite requests for features that are being removed from Hello as part of te user journey work in bug 1209713.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.