608231 - New engines get disabled on first sync

Status: RESOLVED WORKSFORME

(Firefox :: Sync, defect, P5)

Description

[Wladimir Palant]

When the user adds an extension that implements a sync engine he usually expects that it will start syncing data automatically. However, on first sync this engine is "disabled remotely". Sync seems to be assuming that no data for this preference means "engine is disabled" rather than "keep the local setting".

Comment 1

[Philipp von Weitershausen [:philikon]]

(In reply to comment #0)
> Sync seems to be assuming that no data for this preference means
> "engine is disabled" rather than "keep the local setting".

Yes, this is by design. The way around that for custom engines would be to ensure that when they're first installed and want to be on by default, they can let the service know about this by way of the "engineStatusChanged" preference. Pseudo-code:

  if (firstInstall) {
    Weave.Svc.Obs.add("weave:service:ready", function() {
      // Let Weave.Service know that the local setting overrides the remote setting
      Weave.Svc.Prefs.set("engineStatusChanged.myEngine", true);
    });
  }

If we were to add new engines to Sync in the future, we would need the same kind of migration code. 'firstInstall' could probably be determined by comparing Weave.Svc.Prefs.get("lastversion") with Weave.WEAVE_VERSION (once they're meaningful on trunk, too).

Comment 2

[Wladimir Palant]

Waiting for "weave:service:ready" and using Sync services is not very useful given that Sync might not even be installed - and you wasted your chance to do something on first install. Just mentioning because https://wiki.mozilla.org/Services/Sync/FxSync/Developer/ClientAPI (the only available documentation it seems) is also always assuming that Sync services are available and can be used.

But - yes, sounds like a possibility. Too bad that Sync resets engineStatusChanged preference instead of setting it to false - otherwise it would simply be a matter of adding engineStatusChanged.myEngine as default preference with value true.

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #2)
> Waiting for "weave:service:ready" and using Sync services is not very useful
> given that Sync might not even be installed - and you wasted your chance to do
> something on first install.

I don't understand. If Sync isn't installed, why bother at all? If it's installed later, it'll pick up the enabled engined.

> Just mentioning because
> https://wiki.mozilla.org/Services/Sync/FxSync/Developer/ClientAPI (the only
> available documentation it seems) is also always assuming that Sync services
> are available and can be used.

Sure. I trust developers will able to use try-catch as appropriate...

> But - yes, sounds like a possibility. Too bad that Sync resets
> engineStatusChanged preference instead of setting it to false - otherwise it
> would simply be a matter of adding engineStatusChanged.myEngine as default
> preference with value true.

Good point! That actually sounds like an elegant solution. Care to whip up a patch + test?

Comment 4

[Wladimir Palant]

Sorry, I know that it is simple but it's not going to fit in my schedule. Right now I am rather busy preparing Adblock Plus 1.3 release - and deciding whether Sync support can make it somehow.

Comment 5

[Mike Connor [:mconnor]]

So... I guess we need to sort this out before we land new sync engines...

Comment 6

[Gregory Szorc [:gps]]

When this is addressed, the add-ons engine needs to be considered. Currently, the add-ons engine is disabled by default for existing Sync users but enabled for new users. When this bug is fixed, that could result in the add-ons engine being enabled for existing sync users, which would be wrong. Of course, the expected behavior of the add-ons engine could change between now and when this is implemented.

Comment 7

[Richard Newman [:rnewman]]

Bug 692620 is vaguely related to this.

Comment 8

[Mathias De Maré]

Attachment: bug608231.patch

In attachment I've added a first patch, comments welcome. Specifically, I'd like to know if I'm going in the right direction, as I'm unsure.

I've gone with a new 'newEngine' preference, because that makes it easy to add a new engine (simply setting 'newEngine.foo' to true for a new engine named foo, and to false afterwards).

One of the things I'm still unsure about is the correct handling in service.js. Currently, I just go over all the names in ENGINE_MODULES, I'm not sure if that's a good approach?
Additionally, these new engines will (at least with my current patch) not be enabled on later runs: they don't get added to the registerEngines preference yet. To me, it looks like simply appending them to this preference is a good approach. Perhaps I'm wrong :-)

Comment 9

[Richard Newman [:rnewman]]

Apologies for the lateness of this feedback; things have been hectic.


> I've gone with a new 'newEngine' preference, because that makes it easy to
> add a new engine (simply setting 'newEngine.foo' to true for a new engine
> named foo, and to false afterwards).

I have two questions/criticisms of this approach.

Firstly, how would a particular profile know to set newEngine, and when to unset it? First syncs can fail, and first syncs can reoccur after subsequent server wipes, so it seems like this is fraught with danger. It would seem like each engine would need to carefully account for when wipes have been detected, and when the sync account is now in a good state. I don't see that in this patch.

Secondly, the symptoms in Bug 692620 indicate that we have similar issues with built-in engines, which will never be 'new'.

I have a hunch that fixing this bug correctly involves understanding how Bug 692620 occurs, and that means thoroughly understanding the effects that various failure modes have on the meta/global left on the server.

I don't think this is the kind of bug that is correctly solved by wallpapering over one particular symptom, so any proposal of a direction necessarily involves you providing a convincing case and an accounting of the consequences (and non-consequences).

Comment 10

[Mathias De Maré]

Regarding the first comment: my idea was to set any engine that is 'added' (for example say a ffoo engine is added to the codebase in Firefox 34) to newEngine.foo as true when Firefox is upgraded from <34.
An upgrade, a new profile (or perhaps a trigger like a plugin setting newEngine.pluginName) would be the expected events where this would occur.

However, I believe was a bit overconfident in tackling this bug. Like you say, it looks like this requires a different approach.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8510491 [details] [diff] [review]
bug608231.patch

Clearing this flag per discussion. Thanks for the stab at this!

Comment 12

[Andy McKay]

With legacy extensions gone, is this bug still relevant?

Comment 13

[Richard Newman [:rnewman]]

(In reply to Andy McKay [:andym] from comment #12)
> With legacy extensions gone, is this bug still relevant?

I think that between `declined` and the absence of third-party sync engines after 57, this bug is no longer relevant; the See Also list captures issues that might still occur. But I'll wait for Mark to make the call!

Comment 14

[Mark Hammond [:markh] [:mhammond]]

I think bug 692620 is probably WFM now we have declined engines. Bug 1317868 is still an issue and the only remaining issue I'm aware of around this, so I agree with Richard.