Don't require a restart after flipping the buffered engine pref

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: lina, Assigned: tcsc)

Tracking

unspecified
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Thom brought this up at our standup. It's annoying to have to restart Firefox when enabling or disabling the buffered engine, and all too easy to accidentally forget (see bug 1440518, comment 1, for example). This would also help with staged rollout and rollback: we could switch folks over immediately, instead of waiting for them to restart.

Thom suggested adding a "delegating engine" that proxies calls to the right engine based on the pref. We'd want to make sure we don't change the engine in the middle of a sync, of course.
Assignee: nobody → tchiovoloni
Priority: -- → P2
I did this more generically than I initially planned on, since I suspect that in the future (when we're moving engines to use mentat or whatever) we also may want to have other cases where what engine is used gets controlled by a preference.
Comment on attachment 8964695 [details]
Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime

https://reviewboard.mozilla.org/r/233394/#review239014

This looks awesome, thanks, Thom!

::: services/sync/modules/engines.js:512
(Diff revision 1)
>        engines.push(engine);
>      }
>      return engines;
>    },
>  
> +  async switchAlternatives() {

Let's add a comment that this is called just before every sync.

::: services/sync/modules/engines.js:530
(Diff revision 1)
> +      } catch (e) {
> +        this._log.warn(`Failed to unregister previous ${name} engine`, e);
> +      }
> +      info.lastValue = prefValue;
> +      let engineType = prefValue ? info.whenTrue : info.whenFalse;
> +      await this.register(engineType);

Nit: Let's `register` before setting `lastValue`, in case `register` throws? The mirror connects to the database lazily, so it doesn't matter much, but just in case.

::: services/sync/modules/engines.js:649
(Diff revision 1)
>  
>        this._log.error(`Could not initialize engine ${name}`, ex);
>      }
>    },
>  
> -  async unregister(val) {
> +  async unregister(val, removeAlternatives = true) {

Nit: This might be better as an options argument, but I don't have a strong feeling one way or the other.
Attachment #8964695 - Flags: review?(kit) → review+
Comment on attachment 8964695 [details]
Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime

https://reviewboard.mozilla.org/r/233394/#review239014

> Nit: Let's `register` before setting `lastValue`, in case `register` throws? The mirror connects to the database lazily, so it doesn't matter much, but just in case.

Hm, this was intentional, so that we don't retry every time if register throws. Although maybe we want to?

Alternatively, maybe the best semantics would probably be to stick with the old engine if register throws... Although then we'd go through this hassle each sync, and would introduce states like "they flipped the pref and synced but are still using the old engine".

I guess it's probably worth making sure that whatever state we get into here, we don't end up doing a bunch of pointless resyncs every time if register throws.

> Nit: This might be better as an options argument, but I don't have a strong feeling one way or the other.

Eh... I think the only caller that is likely to want to use removeAlternatives = false is switchAlternatives. Otherwise we're likely to re-add it next sync, which seems confusing and error prone.

Making this into an options argument implies to me that you may want to use these options, but maybe it's just me. Maybe it would be better if there were a separate `_unregister` method that *just* unregisters the engine and doesn't remove the alt info, and then both `unregister` and `switchAlternatives` would use that (and `unregister` would take no arguments, and always remove both).
Comment on attachment 8964695 [details]
Bug 1450152 - Handle changing services.sync.engine.bookmarks.buffer at runtime

https://reviewboard.mozilla.org/r/233394/#review239912

LGTM, thanks.
Attachment #8964695 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ec8c31cacc4
Handle changing services.sync.engine.bookmarks.buffer at runtime r=kitcambridge,markh
https://hg.mozilla.org/mozilla-central/rev/1ec8c31cacc4
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.