1421850 - Sync's sync() method should ensure Sync has completed initializing

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

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

Bug 1420349 treated the symptom but not the disease - we should just have sync's public methods await for initialization. I'm also going to move the creation of the scheduler back into onStartup (but next to where the error handler is setup) for consistency.

Comment 1

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

Attachment: Bug 1421850 - ensure Sync is fully initialized before syncing.

Review commit: https://reviewboard.mozilla.org/r/204122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204122/

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8933160 [details]
Bug 1421850 - ensure Sync is fully initialized before syncing.

https://reviewboard.mozilla.org/r/204122/#review209626

If we're actually treating the disease, we might want to be a bit more thorough about making sure nobody is using Weave.Service stuff before it's ready to be used. 

Although, I haven't looked that much. If any of it is hard to address here, I could be convinced it's worth doing in a follow up.

::: services/sync/modules/service.js:295
(Diff revision 1)
>    async onStartup() {
>      this.status = Status;
>      this.identity = Status._authManager;
>      this.collectionKeys = new CollectionKeyManager();
>  
> +    this.scheduler = new SyncScheduler(this);

Can you make sure everywhere else will do the right thing if we move this back here? 

https://searchfox.org/mozilla-central/search?q=Weave.Service&case=false&regexp=false&path= at a minimum brings up https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#301 which looks wrong (but maybe we're guaranteed to be initialized there?)

::: services/sync/modules/service.js:835
(Diff revision 1)
>      }
>    },
>  
>    async login() {
>      async function onNotify() {
> +      await this.promiseInitialized;

As discussed, this is probably not necessary -- callers are expected to call `_shouldLogin` first, which already will throw if we aren't initialized.

Comment 3

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

(In reply to Thom Chiovoloni [:tcsc] from comment #2)
> If we're actually treating the disease, we might want to be a bit more
> thorough about making sure nobody is using Weave.Service stuff before it's
> ready to be used. 

I backed away from doing something like getService() because this stuff is so convoluted and messy, and in particular, some uses of that are in synchronous methods, which is a refactor I didn't think worthwhile for this given they all take the ready state into account.

Also, note that I moved the this.scheduler before the await in onStartup - so I think it will be fine regardless - although in some ways that's not ideal - it seems better for things to fail in an obvious and noisy way rather than subtly doing the wrong thing because (say) some engine is yet to register.

> https://searchfox.org/mozilla-central/search?q=Weave.
> Service&case=false&regexp=false&path= at a minimum brings up
> https://searchfox.org/mozilla-central/source/browser/components/
> nsBrowserGlue.js#301 which looks wrong (but maybe we're guaranteed to be
> initialized there?)

I believe we will do the right thing in all those cases. That nsBrowserGlue code is responding to "weave:service:ready". Everywhere else (eg, prefs, browser-sync) does seem to correctly check the ready state from the xpcom service. Stuff like browser-pageActions.js did not when it called Weave.Service.Sync, but that would be handled by this patch.

> 
> ::: services/sync/modules/service.js:835
> (Diff revision 1)
> >      }
> >    },
> >  
> >    async login() {
> >      async function onNotify() {
> > +      await this.promiseInitialized;
> 
> As discussed, this is probably not necessary -- callers are expected to call
> `_shouldLogin` first, which already will throw if we aren't initialized.

Yeah, thanks, that's what I'll do - especially given bug 821143.

Comment 4

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

Comment on attachment 8933160 [details]
Bug 1421850 - ensure Sync is fully initialized before syncing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204122/diff/1-2/

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8933160 [details]
Bug 1421850 - ensure Sync is fully initialized before syncing.

https://reviewboard.mozilla.org/r/204122/#review209630

Makes sense, thanks!

Comment 6

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

Comment on attachment 8933160 [details]
Bug 1421850 - ensure Sync is fully initialized before syncing.

https://reviewboard.mozilla.org/r/204122/#review209632

Comment 7

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

Comment on attachment 8933160 [details]
Bug 1421850 - ensure Sync is fully initialized before syncing.

oops

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/157f0da9f144
ensure Sync is fully initialized before syncing. r=tcsc

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/157f0da9f144