584241 - Disable trackers when client isn't configured

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Trackers should disable themselves when the client isn't configured (Status.service == CLIENT_NOT_CONFIGURED, STATUS_DISABLED), enable themselves as soon as setup happens (weave:service:setup-complete notification) and disable themselves again if "Use different account" is used to reset the service (weave:service:start-over notification)

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Change trackers to not hook themselves up until they receive the "weave:engine:start-tracking" notification. Tear down on the "weave:engine:stop-tracking" notification.

"start-tracking" is emitted by Weave.Service on startup if crypto is available and no setup is needed, as well as after completing the setup wizard. "stop-tracking" is emitted when the user starts over (which puts the client in the same state as if it had never been configured).

Comment 2

[Ed Lee :Mardak]

If the tracker doesn't track data while not configured, there's a chance of data being reconciled the wrong way. Tracking the data records when the data was changed vs getting all ids and setting the modified time to "now".

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #2)
> If the tracker doesn't track data while not configured, there's a chance of
> data being reconciled the wrong way. Tracking the data records when the data
> was changed vs getting all ids and setting the modified time to "now".

Sure, though this could've happened with the add-on already (e.g. if somebody disabled it). I wouldn't call this "the wrong way" therefore, but I will agree that our modified/changed semantics could be better (just because a server record is newer doesn't mean it's actually the newer record).

Comment 4

[Mike Connor [:mconnor]]

We have the same problem with "use a different account" then, so we need to fix/resolve this anyway.  Tracking changes for all users just in case they might maybe sync someday doesn't make sense, so we need to just fix the initial sync case harder.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: v1.1

Split tests off into separate patch. No code changes.

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: tests

Tests and test fixes.

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to comment #6)
> Tests and test fixes.

I should say I also added tests for the form tracker, mostly so I have test coverage before I further touch the form tracker for bug 487558. We could still do with tests for the password, prefs and tabs trackers.

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: tests v1.1

Have tests clean up after themselves even on failures.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 463146 [details] [diff] [review]
v1.1

>+  Observers.add("places-shutdown", this);
>+  Observers.add("weave:engine:start-tracking", this);
>+  Observers.add("weave:engine:stop-tracking", this);

We have util.js here, so we can use Svc.Obs... we can probably stop importing observers.js in this file...

>+  _enabled: false,
>+  observe: function observe(subject, topic, data) {
>+    switch (topic) {
>+      case "weave:engine:start-tracking":
>+        if (!this._enabled) {
>+          Svc.Bookmark.addObserver(this, true);
>+          this._enabled = true;
>+        }
>+        break;
>+      case "weave:engine:stop-tracking":
>+        if (this._enabled) {
>+          Svc.Bookmark.removeObserver(this);
>+          this._enabled = false;
>+        }
>+        break;

hmm, why not fall through here?  If we're stopping tracking, we should null things out right away, not at shutdown.

>+      case "places-shutdown":
>+        // Explicitly nullify our references to our cached services so
>+        // we don't leak
>+        this.__ls = null;
>+        this.__bms = null;
>+        break;


>diff --git a/services/sync/modules/engines/prefs.js b/services/sync/modules/engines/prefs.js

>+      case "weave:engine:stop-tracking":
>+        if (this._enabled) {
>+          this._prefs.removeObserver("", this);
>+          this._enabled = false;
>+        }
>+        break;
>       case "profile-before-change":
>         this.__prefs = null;
>         this.__syncPrefs = null;

fall through here as well?

r=me with changes

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: v1.2

Addressed review comments.

Comment 11

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/services/fx-sync/rev/02f76eecea87
http://hg.mozilla.org/services/fx-sync/rev/d876d115769c