This is something we have noticed since we can run tps tests with fxaccounts. Whenever we run tests a second time, they fail because some data has not been deleted. In case of our tps tests we first wipe the server to ensure to start with fresh data: http://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/tps/resource/tps.jsm#807 810 Weave.Service.login(); 811 Weave.Service.wipeServer(); 812 Weave.Service.resetClient(); 813 Weave.Service.login(); Please make sure that my patch on bug 982591 may be needed, so we do not make use of faked auth data while signing into the fxaccount at the moment. Mark, can you please tell me which logging preferences I need to see what could be broken?

Oh, and what I forgot to say: This is not happening for the old sync authentication.

You prettymuch want the following prefs to all have the value "Trace": pref("services.sync.log.appender.console", "Warn"); pref("services.sync.log.appender.dump", "Error"); pref("services.sync.log.appender.file.level", "Trace"); pref("services.sync.log.rootLogger", "Debug"); pref("services.sync.log.logger.addonutils", "Debug"); pref("services.sync.log.logger.declined", "Debug"); pref("services.sync.log.logger.service.main", "Debug"); pref("services.sync.log.logger.status", "Debug"); pref("services.sync.log.logger.authenticator", "Debug"); pref("services.sync.log.logger.network.resources", "Debug"); pref("services.sync.log.logger.service.jpakeclient", "Debug"); pref("services.sync.log.logger.engine.bookmarks", "Debug"); pref("services.sync.log.logger.engine.clients", "Debug"); pref("services.sync.log.logger.engine.forms", "Debug"); pref("services.sync.log.logger.engine.history", "Debug"); pref("services.sync.log.logger.engine.passwords", "Debug"); pref("services.sync.log.logger.engine.prefs", "Debug"); pref("services.sync.log.logger.engine.tabs", "Debug"); pref("services.sync.log.logger.engine.addons", "Debug"); pref("services.sync.log.logger.engine.apps", "Debug"); pref("services.sync.log.logger.identity", "Debug"); pref("services.sync.log.logger.userapi", "Debug"); and: services.sync.log.appender.file.logOnSuccess=false services.sync.log.cryptoDebug=true

oops - services.sync.log.appender.file.logOnSuccess=true

This mainly might depend on my fix on bug 981848. With it applied I don't see the problems anymore.

This has been fixed by my patch on bug 981848.

Looks like there is still the problem each second time we run tests. Here with test_sync.js: CROSSWEAVE INFO: starting action: Passwords__add CROSSWEAVE INFO: executing action ADD on password {"hostname":"http://www.example.com","submitURL":"http://login.example.com","username":"joe","password":"SeCrEt123","usernameField":"uname","passwordField":"pword","changes":{"password":"zippity-do-dah"}} ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "This login already exists.'This login already exists.' when calling method: [nsILoginManager::addLogin]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: resource://tps/modules/passwords.jsm :: Password.prototype.Create :: line 87" data: no] ************************************************************

All my latest patches don't fix that issue. So I will have a look at it now.

As talked with Richard on IRC the problem here might be that we have collisions between the first initial sync, and the by tps triggered wipeServer() command. As Richard pointed out he can see a lot of 'locked: already syncing?' errors. So that could be the reason why wipeServer() is not successful. So one other solution would be to wipe all data from the server at the end of the test, and not at the beginning. This will be a bit harder to do, given that we always have to run that code even when a forced close of the browser happens. If we want to keep cleaning up of the server data at the beginning, we should wait for the initial sync, and start the resetData actions afterward, like * waitforfirstsync() * wipeServer() * wipeClient() I will check first the latter method tomorrow, and if that doesn't work I will have a look at cleaning up during shutdown.

This patch ensures that we always call wipeServer() at the end of a test, whether if it is passing or failing. Also if the browser gets quit, we handle that now via the quit-application-requested observer notification. While working on that patch, I created some other notifications and methods, which I left in for now. We might need those later, and for now it helps with debugging and investigation just by looking at the log output.

Comment on attachment 8400602 [details] [diff] [review] Ensure wipeServer() call v1 Review of attachment 8400602 [details] [diff] [review]: ----------------------------------------------------------------- For historical context: the reason TPS doesn't wipe after the test is for forensics. rs=me ::: services/sync/tps/extensions/tps/resource/tps.jsm @@ +118,4 @@ > } > }, > > + DumpError: function TPS__DumpError(msg) { No need for this.

(In reply to Richard Newman [:rnewman] from comment #10) > For historical context: the reason TPS doesn't wipe after the test is for > forensics. We actually did wipe the data of the server but only when we run the tests successfully. In any case of a failure we didn't hit the last action for wiping, and simply failed. I don't see that this was on purpose. If you think that would be helpful we can change that and really do the wipe call at the beginning with any caveats we would have. > > + DumpError: function TPS__DumpError(msg) { > > No need for this. That was just to get the declaration in sync with the other methods. Thank you both for the reviews.

Comment on attachment 8400602 [details] [diff] [review] Ensure wipeServer() call v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): None User impact if declined: None, only affects testing Testing completed (on m-c, etc.): inbound and m-c Risk to taking this patch (and alternatives if risky): None String or IDL/UUID changes made by this patch: None

Comment on attachment 8400602 [details] [diff] [review] Ensure wipeServer() call v1 [Triage Comment] Given the low risk here and the affected on Beta, please also uplift there if it can land cleanly otherwise put up a new branch-specific patch for approval.

Leaving the beta uplift for this and the other TPS patches to Henrik.