Closed Bug 996027 Opened 10 years ago Closed 10 years ago

Ensure that TPS always fakes login into Weave

Categories

(Testing Graveyard :: TPS, defect)

defect
Not set
normal

Tracking

(firefox29 affected, firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- affected
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Right now we fail at different places to with syncing tabs. As I have seen this is because we do not correctly log into Weave by fxaccounts at all, and with the old sync in profile 2.

I simply missed that in my initial patch to update TPS for fxaccounts, and broke it.
OS: Linux → All
Hardware: x86_64 → All
Summary: Ensure that TPS always fakes logging into Weave → Ensure that TPS always fakes login into Weave
Attached patch Always fake Weave login (obsolete) — Splinter Review
So as seen on bug 990509 we fail because we do no longer fake the login to Weave. This patch is fixing this for fxaccounts, and also enhances it to not do that if we are already logged in, unless the force option is specified. 

I'm not sure if all that is 100% ok, so I would also like to see a review by Richard.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #8406225 - Flags: review?(rnewman)
Attachment #8406225 - Flags: review?(jgriffin)
Component: Mozmill → TPS
QA Contact: hskupin
Comment on attachment 8406225 [details] [diff] [review]
Always fake Weave login

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

::: services/sync/tps/extensions/tps/resource/tps.jsm
@@ +832,5 @@
> +    if (!Authentication.isLoggedIn || force) {
> +      Logger.logInfo("Setting client credentials and login.");
> +      let account = this.fxaccounts_enabled ? this.config.fx_account
> +                                            : this.config.sync_account;
> +      Authentication.signIn(account);

This method is now misnamed -- it doesn't sign in at all. Instead, it sets the identity.

@@ +841,5 @@
> +      Logger.logInfo("Logging into Weave.");
> +      Weave.Service.login();
> +      Logger.AssertEqual(Weave.Status.login, Weave.LOGIN_SUCCEEDED,
> +                         "Weave logged in");
> +      Weave.Svc.Obs.notify("weave:service:setup-complete");

Note that setup-complete is notified by login() if parameters are passed. It's worth commenting that here… or just implement this by passing parameters to login()?
Attachment #8406225 - Flags: review?(rnewman) → review+
Comment on attachment 8406225 [details] [diff] [review]
Always fake Weave login

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

Deferring to rnewman here.
Attachment #8406225 - Flags: review?(jgriffin)
(In reply to Richard Newman [:rnewman] from comment #2)
> ::: services/sync/tps/extensions/tps/resource/tps.jsm
> @@ +832,5 @@
> > +    if (!Authentication.isLoggedIn || force) {
> > +      Logger.logInfo("Setting client credentials and login.");
> > +      let account = this.fxaccounts_enabled ? this.config.fx_account
> > +                                            : this.config.sync_account;
> > +      Authentication.signIn(account);
> 
> This method is now misnamed -- it doesn't sign in at all. Instead, it sets
> the identity.

Not really. We indeed still call client.signIn() AND set the new identity. But all that is tied to Firefox Accounts but not Weave. So I think we should still keep it that way, and handle the login for Weave separately. Which also is identical between Firefox Accounts and the old Sync authentication.

> @@ +841,5 @@
> > +      Logger.logInfo("Logging into Weave.");
> > +      Weave.Service.login();
> > +      Logger.AssertEqual(Weave.Status.login, Weave.LOGIN_SUCCEEDED,
> > +                         "Weave logged in");
> > +      Weave.Svc.Obs.notify("weave:service:setup-complete");
> 
> Note that setup-complete is notified by login() if parameters are passed.
> It's worth commenting that here… or just implement this by passing
> parameters to login()?

Oh, interesting. I will check that. Thanks Richard!
(In reply to Henrik Skupin (:whimboo) from comment #4)
> > > +      Logger.logInfo("Logging into Weave.");
> > > +      Weave.Service.login();
> > > +      Logger.AssertEqual(Weave.Status.login, Weave.LOGIN_SUCCEEDED,
> > > +                         "Weave logged in");
> > > +      Weave.Svc.Obs.notify("weave:service:setup-complete");
> > 
> > Note that setup-complete is notified by login() if parameters are passed.
> > It's worth commenting that here… or just implement this by passing
> > parameters to login()?
> 
> Oh, interesting. I will check that. Thanks Richard!

Something is maybe wrong with the initial sync when using the old sync authentication with TPS. It's failing because we use a 'dummy' account. Later logins will not send the notification, so we have to fake it. I filed bug 997279 to take care of it.
What about this? The fake sending of the notification is now part of the sync authentication. Once we get rid of it, it will no longer be in the way. I think that's way cleaner now.
Attachment #8406225 - Attachment is obsolete: true
Attachment #8407655 - Flags: review?(rnewman)
Attachment #8407655 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/6196c121c4f4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
The patch as attached here doesn't fix all the intermittent issues. I will file follow-up ones shortly. But this patch should land on aurora too.
Comment on attachment 8407655 [details] [diff] [review]
Always fake Weave login v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 966434 
User impact if declined: none, just testing code
Testing completed (on m-c, etc.): mozilla-central for a couple of days
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None
Attachment #8407655 - Flags: approval-mozilla-aurora?
Attachment #8407655 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.