[tps] Synced data on the server is not always wiped at the end of a test.

RESOLVED FIXED in Firefox 30

Status

defect
--
major
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Blocks 1 bug)

unspecified
mozilla31
Dependency tree / graph

Firefox Tracking Flags

(firefox29 wontfix, firefox30 fixed, firefox31 fixed)

Details

Attachments

(1 attachment)

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?
Flags: needinfo?(mhammond)
Summary: Calling wipeServer() via TPS does not complete remove the data for the fxaccount authenticated account → Calling wipeServer() via TPS does not completely remove the data for the fxaccount authenticated account
Oh, and what I forgot to say: This is not happening for the old sync authentication.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
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
Flags: needinfo?(mhammond)
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.
No longer blocks: 981836
Depends on: 981848
This has been fixed by my patch on bug 981848.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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]
************************************************************
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
All my latest patches don't fix that issue. So I will have a look at it now.
Status: REOPENED → ASSIGNED
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.
Component: Firefox Sync: Backend → TPS
Product: Mozilla Services → Testing
Summary: Calling wipeServer() via TPS does not completely remove the data for the fxaccount authenticated account → [tps] Synced data on the server is not always wiped at the end of a test.
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.
Attachment #8400602 - Flags: review?(rnewman)
Attachment #8400602 - Flags: review?(jgriffin)
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.
Attachment #8400602 - Flags: review?(rnewman) → review+
Attachment #8400602 - Flags: review?(jgriffin) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/d82714afadf1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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
Attachment #8400602 - Flags: approval-mozilla-aurora?
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.
Attachment #8400602 - Flags: approval-mozilla-beta+
Attachment #8400602 - Flags: approval-mozilla-aurora?
Attachment #8400602 - Flags: approval-mozilla-aurora+
Leaving the beta uplift for this and the other TPS patches to Henrik.
Flags: needinfo?(hskupin)
Flags: needinfo?(hskupin)
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.