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.

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.

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()?

Comment on attachment 8406225 [details] [diff] [review] Always fake Weave login Review of attachment 8406225 [details] [diff] [review]: ----------------------------------------------------------------- Deferring to rnewman here.

(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.

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