676042 - Clients Engine Score Changes Do Not Impact Sync Score

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

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

Comment 1

[Gregory Szorc [:gps]]

Attachment: Track Clients Engine Score in Overall Score

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 2

[Richard Newman [:rnewman]]

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]"...

Comment 3

[Gregory Szorc [:gps]]

Attachment: Track Clients Engine Score in Overall Score, v2

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.

Comment 4

[Philipp von Weitershausen [:philikon]]

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.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Track Clients Engine Score in Overall Score, v3

Patch with nits applied. Marked r+. Ready for commit.

Comment 6

[Richard Newman [:rnewman]]

Pushed to s-c:

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

gps, please write up STRs in this bug.

Comment 7

[Gregory Szorc [:gps]]

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)

Comment 8

[Tracy Walker [:tracy]]

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?

Comment 9

[Gregory Szorc [:gps]]

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"?

Comment 10

[Tracy Walker [:tracy]]

hmmm, turned on that logging an now auto-sync of this pref change works as expected.  that's weird.  anyway, verified s-c

Comment 11

[Richard Newman [:rnewman]]

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

Comment 12

[Tracy Walker [:tracy]]

verified on m-c