Closed Bug 1003250 Opened 10 years ago Closed 10 years ago

Disable automatic sync for TPS tests

Categories

(Testing Graveyard :: TPS, defect)

Product:
Testing Graveyard
Component:
TPS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox30 fixed, firefox31 fixed, firefox32 fixed, b2g-v1.4 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox30 --- fixed
firefox31 --- fixed
firefox32 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

Disable automatic sync v1
3.89 KB, patch
rnewman
: review+
Details | Diff | Splinter Review 
As what I have seen when working on bug 990509, the call to Sync() could cause a raise condition if an already active sync is present when called. See the following output:

CROSSWEAVE INFO: tab for data:text/html,<html><head><title>Bye</title></head><body>Bye</body></html> finished loading
CROSSWEAVE INFO: ----------event observed: weave:service:sync:start
CROSSWEAVE INFO: tab for http://hg.mozilla.org/automation/crossweave/raw-file/2d9aca9585b6/pages/page3.html finished loading
CROSSWEAVE INFO: All tabs loaded, continuing...
CROSSWEAVE INFO: starting action: TPS__Sync
CROSSWEAVE INFO: Executing Sync
CROSSWEAVE INFO: ----------event observed: weave:service:sync:finish

After the first tab has been loaded a sync is triggered by Firefox. TPS will afterward add another tab. This new tab might not be seen by Sync for the currently running Sync action. A call to TPS.Sync() hooks into this active sync and waits for its completion. That means a restart of the application afterward, could cause the new tab not been synced.

I think the best would be that TPS has to wait until the current sync is done, and to start another one right after. Only that way we can make sure that all recently added data will be synced with the server.

Mark and Richard, what is your take on that? Does that sound good?
Flags: needinfo?(rnewman)
Flags: needinfo?(mhammond)
(In reply to Henrik Skupin (:whimboo) from comment #0)

> I think the best would be that TPS has to wait until the current sync is
> done, and to start another one right after. Only that way we can make sure
> that all recently added data will be synced with the server.
> 
> Mark and Richard, what is your take on that? Does that sound good?

It's kinda hacky, and it might introduce other problems if we're expecting that TPS's Sync call results in some action (imagine if the automatic sync happens after all of the setup stuff -- the second sync will do nothing).

But overall this seems like a safe change. Should be as simple as observing start/finish events and tracking a boolean in TPS.
Flags: needinfo?(rnewman)
What Richard said :)
Flags: needinfo?(mhammond)
Thanks for the feedback, and good to hear that. I also thought more about it since I left it off yesterday. What about disabling the automatic syncs completely for TPS tests? Given the above problem it could give us failures in a couple of situations:

* A test explicitly sets up user data but does NOT call Sync(). An automatic sync executed by Firefox will cause this data to be synced, so the next phase will fail because data has been synced.

* Currently a call to Sync() will hook into the currently active sync. So data which has been modified after the sync action has been started will not be synced

So I talked with Mark on IRC and we came to an agreement that we should better disable the automatic sync in TPS tests. Instead a test has to explicitly call Sync() to get data synced.

We could do that with those two preferences: 

services.sync.scheduler.idleInterval
services.sync.scheduler.activeInterval
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: Call to Sync() should wait until an active sync is done → Disable automatic sync for TPS tests
Setting only those prefs will not work:

1398846783037	Sync.SyncScheduler	DEBUG	Global Score threshold hit, triggering sync.
1398846783037	Sync.SyncScheduler	TRACE	There's already a sync scheduled in 7197963 ms.

Looks like we reach the threshold kinda quick after even opening a single tab. I will check if we can modify that threshold constantly, so we can workaround this. It would be way nicer if there would exist a boolean pref to simply turn automatic syncs on and off.
So, those thresholds are hard-coded in constants.js:

// score thresholds for early syncs
SINGLE_USER_THRESHOLD:                 1000,
MULTI_DEVICE_THRESHOLD:                300,

So I doubt we can safely disable auto-sync for TPS. Richard and Mark, do you have another idea?
Flags: needinfo?(rnewman)
Flags: needinfo?(mhammond)
policies.js:

    this.idleInterval         = Svc.Prefs.get("scheduler.idleInterval")         * 1000;
    this.activeInterval       = Svc.Prefs.get("scheduler.activeInterval")       * 1000;
    this.immediateInterval    = Svc.Prefs.get("scheduler.immediateInterval")    * 1000;
    this.eolInterval          = Svc.Prefs.get("scheduler.eolInterval")          * 1000;


Note that you also need to raise the score-based threshold:

  get syncThreshold() Svc.Prefs.get("syncThreshold", SINGLE_USER_THRESHOLD),
  set syncThreshold(value) Svc.Prefs.set("syncThreshold", value),
Flags: needinfo?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #6)
> Note that you also need to raise the score-based threshold:

Note also that all .startOver() calls will probably reset these prefs, so setting them once probably isn't going to be enough - they will need to be set by code - possibly on an observer notification - which all starts to sound somewhat complicated.

I'd be fine with a pref that disables it completely, but that's probably going to have the exact same issue unless the pref is outside the services.sync tree.  I'm really not sure how difficult or intrusive such a flag would be.
Flags: needinfo?(mhammond)
You could also swizzle Weave.Service.sync so that it only works if called via TPS.
Looks like the above is doing the trick. Yay! With that changed the tabs.js test seems to always pass and the log looks like:

CROSSWEAVE INFO: starting action: Tabs__add
CROSSWEAVE INFO: executing action ADD on tab {"uri":"http://hg.mozilla.org/automation/crossweave/raw-file/2d9aca9585b6/pages/page3.html","title":"Crossweave Test Page 3","profile":"profile2"}
CROSSWEAVE INFO: executing action ADD on tab {"uri":"data:text/html,<html><head><title>Bye</title></head><body>Bye</body></html>","profile":"profile2"}
CROSSWEAVE TEST PASS: executing action ADD on tabs
CROSSWEAVE INFO: tab for data:text/html,<html><head><title>Bye</title></head><body>Bye</body></html> finished loading
CROSSWEAVE INFO: tab for http://hg.mozilla.org/automation/crossweave/raw-file/2d9aca9585b6/pages/page3.html finished loading
CROSSWEAVE INFO: all tabs loaded, continuing...
CROSSWEAVE INFO: starting action: TPS__Sync
CROSSWEAVE INFO: Executing Sync
CROSSWEAVE INFO: ----------event observed: weave:service:sync:start
CROSSWEAVE INFO: ----------event observed: weave:service:sync:finish

So we clearly don't have an automatic sync in-between but really get it started by calling Sync().

Not sure if I really want to play with Weave.Service.sync(). That sounds awkward. I hope the above will do it. I will run a lot of testruns over the weekend.
This delays the automatic sync as much as possible. I don't think we will need higher numbers here. I run all the tests a couple of times and they are passing. No single one is failing. So it looks like we indeed suffered from this race condition in all of our tests.
Attachment #8417407 - Flags: review?(rnewman)
Comment on attachment 8417407 [details] [diff] [review]
Disable automatic sync v1

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

This looks fine. Something to keep in mind: this will prevent any kind of TPS test that aims to trigger syncs via scoring or timing, should we ever want to do that. At that point we'd have to revert this change and figure out a different approach.
Attachment #8417407 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #11)
> This looks fine. Something to keep in mind: this will prevent any kind of
> TPS test that aims to trigger syncs via scoring or timing, should we ever
> want to do that. At that point we'd have to revert this change and figure
> out a different approach.

Absolutely sure. One of the next possible steps is to transform TPS to be async itself. So maybe we could already revert it by then, and figure out a strategy how to tackle that.

https://hg.mozilla.org/mozilla-central/rev/7dcb8e0d9668

Lets wait for the next TPS results! I'm kinda curious if we get them fixed all now.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox32: affected → fixed
Resolution: --- → FIXED
Comment on attachment 8417407 [details] [diff] [review]
Disable automatic sync v1

I run a couple of test-runs across different machines, and also asked several people to do the same. For everyone of us all tests are passing now. Our TPS CI machine still shows some failures, but its most likely busted in some way. We will replace it soonish.

For now lets get this backported to aurora and beta, so we will have completely passing test-runs there too.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None, just testing code which fixes possible race conditions in tests
Testing completed (on m-c, etc.): mc
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None
Attachment #8417407 - Flags: approval-mozilla-beta?
Attachment #8417407 - Flags: approval-mozilla-aurora?
Comment on attachment 8417407 [details] [diff] [review]
Disable automatic sync v1

This is test-only code, so as given by sheriffs we can directly land with a=testonly without having to request approval.
Attachment #8417407 - Flags: approval-mozilla-beta?
Attachment #8417407 - Flags: approval-mozilla-aurora?
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: