Closed Bug 676042 Opened 8 years ago Closed 8 years ago

Clients Engine Score Changes Do Not Impact Sync Score

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla9

People

(Reporter: gps, Assigned: gps)

References

Details

(Whiteboard: [verified in services])

Attachments

(1 file, 2 obsolete files)

Currently, score changes in the clients engine do not impact the sync score, calculated in Sync.SyncScheduler.calculateScore() because the client engine is not enabled.

Expected: Changes to the clients engine score should impact the overall sync score.

This is preventing computer name changes from incurring an immediate sync (bug 646539).
This is a trivial patch to track the clients engine score in the overall sync score.

Interestingly, this patch causes the test_score_triggers.js unit test to deadlock in test_sync_triggered. I'm looking into that now.

The patch is also obviously lacking tests. These will be added later.

I'm only posting the patch in case others want to informally comment on it or lend thoughts on why they suspect the aforementioned test is deadlocking.
Comment on attachment 550893 [details] [diff] [review]
Track Clients Engine Score in Overall Score

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

::: services/sync/modules/policies.js
@@ +237,5 @@
>  
>    calculateScore: function calculateScore() {
> +    for each (let engine in Engines.getAll()) {
> +      // The clients engine is a special case. It is always disabled because
> +      // it is special. Yet, we still want to track its score so it can

Incorrect, according to clients.js:

  // Always sync client data as it controls other sync behavior
  get enabled() true,

So you can probably drop this whole patch so far to simply:

  - var engines = Engines.getEnabled();
  + let engines = [Clients].concat(Engines.getEnabled());

which is what service.js does elsewhere if you grep for "[Clients]"...
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → gps
This patch tracks the clients engine score in the overall score.

It includes a test which failed before (it hung) and passes with the simple change in policies.js.
Attachment #550893 - Attachment is obsolete: true
Attachment #551606 - Flags: review?(philipp)
Comment on attachment 551606 [details] [diff] [review]
Track Clients Engine Score in Overall Score, v2

>   calculateScore: function calculateScore() {
>-    var engines = Engines.getEnabled();
>+    var engines = [Clients].concat(Engines.getEnabled());

Nit: while you're in here, s/var/let/

>+add_test(function test_clients_engine_sync_triggered() {
>+  _("Ensure that client engine score changes trigger a sync.");
>+
>+  // This test is the result of bug 676042. It was discovered that score
>+  // changes in the clients engine did not incur a sync. This is because the
>+  // clients engine is not a registered engines, like other engines. Here,

Typo/grammar: plural on registered engines.

>+  // we verify that score changes to the clients engine result in a
>+  // recorded score update.

Rather than giving the historical note which can be looked up in the bug, I would just note that this test exists because Clients is not registered with the engine manager and refer to bug 676042 for details.
Attachment #551606 - Flags: review?(philipp) → review+
Patch with nits applied. Marked r+. Ready for commit.
Attachment #551606 - Attachment is obsolete: true
Attachment #551610 - Flags: review+
Keywords: checkin-needed
Pushed to s-c:

  http://hg.mozilla.org/services/services-central/rev/7550224fb6c1

gps, please write up STRs in this bug.
Keywords: checkin-needed
Whiteboard: [fixed in services]
Steps to reproduce:

1) Perform an action that would result in clients engine score being increased, such as changing the Client's name preference (services.sync.client.name)

2) Verify sync was triggered (and then performed) immediately afterwards (likely by looking at log output)
Works fine on Windows as you have to dismiss the Prefs dialog to make changes take.

On Mac however, it's not working as one might expect.  Changing the client name in the Options (or in the config) doesn't fire a sync.  I did discover that if you change the name in the Options UI and then hit <enter> that a sync is fired as expected.

Should some sort of listener be added to catch changes to the pref?
I just tested this on my Nightly build on Mac and changing the client name in the Sync Prefs pane does trigger a sync.

The code works by watching the preference, so it shouldn't matter which method you use to update the client name.

Can you double verify you don't see the "Sync.Tracker.Clients" logger emitting a debug level message that "client.name preference changed"?
hmmm, turned on that logging an now auto-sync of this pref change works as expected.  that's weird.  anyway, verified s-c
Whiteboard: [fixed in services] → [verified in services]
http://hg.mozilla.org/mozilla-central/rev/7550224fb6c1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
verified on m-c
Status: RESOLVED → VERIFIED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.