Mark Hammond [:markh] [:mhammond] Assignee
	Description • 9 years ago

about:sync-tabs reaches into Sync to force the clients and tabs engine to Sync. This is bad for a number of reasons, not limited to:

* No locking - it is possible that the engine is already syncing.
* No login and other setup - if this is done before Sync has run normally once it fails in obscure ways.

Other work we are doing around synced tabs (eg, bug 1201331) will have the same issue.

While not perfect, a reasonable solution might be to allow Service.sync() to optionally have passed an array of engine names to sync.

Richard, what do you think?

	Richard Newman [:rnewman]
	Comment 1 • 9 years ago

Yeah, that's basically exactly what we do on Android and iOS.

	Richard Newman [:rnewman]
	Comment 2 • 9 years ago

Comment on attachment 8683547 [details] [diff] [review]
0006-Bug-XXXXXXX-Allow-Sync-to-only-sync-specified-engine.patch

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

::: services/sync/modules/stages/enginesync.js
@@ +144,5 @@
>      }
>  
>      try {
>        for (let engine of engineManager.getEnabled()) {
> +        if (enginesToSync && enginesToSync.indexOf(engine.name) == -1) {

My only concern with this is to make sure that we correctly process resets/wipes/etc if we don't sync right now.

Later in this function we also unset firstSync, so my inclination here is that you should check to see if anything scary is happening (such as a first sync) and ignore the 'hint' if so.

@@ +145,5 @@
>  
>      try {
>        for (let engine of engineManager.getEnabled()) {
> +        if (enginesToSync && enginesToSync.indexOf(engine.name) == -1) {
> +          this._log.info(`skipping sync of ${engine.name} as it was not requested`);

.debug(`Skipping

	Richard Newman [:rnewman]
	Comment 3 • 9 years ago

One other thought here: perhaps we should sync in the order engines are supplied. That is, walk *that* list checking it against enabled.

	Mark Hammond [:markh] [:mhammond] Assignee
	Comment 4 • 9 years ago

(In reply to Richard Newman [:rnewman] from comment #2)
> My only concern with this is to make sure that we correctly process
> resets/wipes/etc if we don't sync right now.
> 
> Later in this function we also unset firstSync, so my inclination here is
> that you should check to see if anything scary is happening (such as a first
> sync) and ignore the 'hint' if so.

Yeah, good idea. It looks like only that "first sync" is worth handling. I considered extending that to also ignore the hint when there are client commands, but that seems overkill, especially when a common command may end up being "displayURI". What do you think?

> One other thought here: perhaps we should sync in the order engines are
> supplied. That is, walk *that* list checking it against enabled.

SGTM!

	Richard Newman [:rnewman]
	Comment 5 • 9 years ago

(In reply to Mark Hammond [:markh] from comment #4)

> Yeah, good idea. It looks like only that "first sync" is worth handling. I
> considered extending that to also ignore the hint when there are client
> commands, but that seems overkill, especially when a common command may end
> up being "displayURI". What do you think?

I'm pretty confident that processIncomingCommands does the right thing for resetClient and wipeClient even if the affected engine isn't subsequently synced. The main thing that sticks out to me is that this will introduce a potential gap.

For example: if a remote client sends wipeClient("bookmarks"), we'll immediately throw away all of our bookmarks and reset to zero… but we won't now do the immediate resync that we would have, so the user might see a few more minutes of blankness.

That's not behavior required by the protocol (chuckle), and of course that resync could fail now, yielding the same behavior. Indeed, a slight delay might help us avoid race conditions! But this is a change in behavior, so it's worth thinking abouut.

	Pulsebot
	Comment 6 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/e85794f108f0

	Carsten Book [:Tomcat]
	Comment 7 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e85794f108f0