Closed Bug 633062 Opened 13 years ago Closed 6 years ago

Don't spin the event loop in any observers

Categories

(Firefox :: Sync, defect, P1)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: philikon, Assigned: eoger)

References

Details

(Whiteboard: [sync:scale][sync:rigor])

Attachments

(8 files, 1 obsolete file)

Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 633062 p3 - Remove event loop spinning from the EngineManager.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 633062 p1 - Introduce AsyncQueueCaller and AsyncObserver.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 633062 p5 - Remove event loop spinning from addonutils.js.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 633062 p6 - Remove event loop spinning from head_helper.js addon-related methods.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 633062 p7 - Make most of the addon reconciler async.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 633062 p8 - Add Async observers.
59 bytes, text/x-review-board-request
tcsc
: review+
lina
: review+
markh
: review+
Details 
Observers are executed synchronously, spinning the event loop while in an observer can cause hangs and other weird things. All trackers and other observers should be audited for this.
Depends on: 633266
Depends on: 633274
Depends on: 654900
No longer depends on: 633274
Depends on: 660753
No longer depends on: 633266
Depends on: 663115
Would it make sense to add an assertion (after fixing the dependencies)?
(In reply to comment #1)
> Would it make sense to add an assertion (after fixing the dependencies)?

I would say so. Spinning the event loop is generally a foot gun and the more we disallow it, the better. Note that the dependencies are probably not yet complete as they only list trackers and not other observers. It should be fairly easy to audit them, though.

Where would you add this assertion? In the observer service? Perhaps we could make it optional in the beginning to help us track down offenders without actually impacting production code.

Also note that we are slowly getting rid of *all* event loop spinning... it's just taking a long time to re-engineer Sync to have async APIs.
Blocks: 745408
Whiteboard: [sync:scale][sync:rigor]
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8933481 [details]
Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.

https://reviewboard.mozilla.org/r/204414/#review209970

::: services/sync/modules/bookmark_repair.js:704
(Diff revision 1)
>      Svc.Obs.remove("weave:engine:sync:uploaded", this.onUploaded, this);
>      if (subject.failed) {
>        return;
>      }
>      log.debug(`bookmarks engine has uploaded stuff - creating a repair response`, subject);
> -    Async.promiseSpinningly(this._finishRepair());
> +    this._finishRepair();

This call was useless since we are in a observer callback already.

::: services/sync/modules/browserid_identity.js:386
(Diff revision 1)
>          });
>        }
>      } break;
>  
>      case fxAccountsCommon.ONLOGOUT_NOTIFICATION:
> -      Async.promiseSpinningly(Weave.Service.startOver());
> +      // Start over in the background.

Same here, I don't think it matters that much to spin the event loop here.

::: services/sync/modules/service.js:443
(Diff revision 1)
>          }
>          break;
>        case "fxaccounts:device_disconnected":
>          data = JSON.parse(data);
>          if (!data.isLocalDevice) {
> -          Async.promiseSpinningly(this.clientsEngine.updateKnownStaleClients());
> +          // Refresh the known stale clients list in the background.

It didn't matter that much to await here, this can be removed safely.
Comment on attachment 8933481 [details]
Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.

https://reviewboard.mozilla.org/r/204414/#review209978

Partial review. I'll try to look at the rest tomorrow but I'm worried enough about the (unintentional?) sematic change with observers that I'm commenting now.

The biggest question I have here is whether or not we need the complexity (and risk, as mentioned below) for serializing/queueing event handling.

What are we trying to avoid? It's worth noting that reentrant calls to observers are possible in the current system -- `observe` (and `notifyObservers`) calls are synchronous. E.g. spinning an event loop only prevents the *next* observer for that event from getting called. Importantly, it does *not* prevent us from getting other notifications in the mean-time, which is what the queues do.

And so the question is "do we generally care about preventing the next observer from being called?" AFAIK the answer is no -- Which is good, since the only way we realistically could do that (short of making the observers async aware) is event loop spinning.

::: services/common/async.js:229
(Diff revision 1)
> +    this.queue = Promise.resolve();
> +  }
> +
> +  enqueueCall(func) {
> +    return this.queue = (async () => {
> +      await this.queue;

Once concern I'd have is that a single 'stuck' promise here would break things forever.

There are two cases I'm concerned about here.

1. The promise never resolves due to some bug in some code somewhere. This is bad, but in a promiseSpinningly world, we'll actually keep working and keep handling events. With this patch, the observer will never observe another event again. That seems very dangerous.
2. Recursive use of the queue. E.g. won't `q.enqueueCall(async() => q.enqueueCall(() => console.log("foo")))` be stuck forever? It seems that way to me, and it seems like this could happen accidentally or something, IDK.

As I mention (further) above, I don't think we need this anyway. If we do need it for some cases, I'd want to resolve those issues though, or be completely convinced they're impossible where we're using it.

::: services/common/async.js:233
(Diff revision 1)
> +    return this.queue = (async () => {
> +      await this.queue;
> +      try {
> +        await func();
> +      } catch (e) {
> +        Cu.reportError(e);

Will this show up in a sync error log? I think the answer is no but we'd want it to...

::: services/common/async.js:255
(Diff revision 1)
> +    super();
> +    this.obj = obj;
> +  }
> +
> +  observe(subject, topic, data) {
> +    this.enqueueCall(() => this.obj.observe(subject, topic, data));

Do we want to return here? Answer is probably 'no', but maybe tests will want to wait on it? Thoughts?

::: services/common/tests/unit/test_tokenserverclient.js:467
(Diff revision 1)
>    do_check_eq(callbackCount, 1);
>  
> -  server.stop(run_next_test);
> +  await promiseStopServer(server);
> +});
> +
> +function promiseStopServer(server) {

We already have this in head_helpers.js https://searchfox.org/mozilla-central/source/services/sync/tests/unit/head_helpers.js#494

::: services/sync/modules/addonsreconciler.js:466
(Diff revision 1)
>  
>      for (let listener of this._listeners) {
>        try {
> -        listener.changeListener(date, change, state);
> +        await listener.changeListener(date, change, state);
>        } catch (ex) {
>          this._log.warn("Exception calling change listener", ex);

Should this be an error log?
Attachment #8933481 - Flags: review?(tchiovoloni)
Comment on attachment 8933481 [details]
Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.

https://reviewboard.mozilla.org/r/204414/#review209986

Thanks for starting this and I think it's valuable, just not yet ready. It's a very large patch, so it would be great if you can see if there are ways to split it a little - eg, some of the tokenserver related changes seem a only vaguely unrelated, and it *might* even be possible to make some of the async tracker changes independent of this, even if it means adding one or 2 additional promiseSpinningly calls in the "part n" patch, just to be removed again and replaced with your magic queueing in "part n+1".

But the biggest issue is that sync itself never waits for these new queues, so *appears* to work fine on our hardware but is inherently racey.

::: services/common/async.js:233
(Diff revision 1)
> +    return this.queue = (async () => {
> +      await this.queue;
> +      try {
> +        await func();
> +      } catch (e) {
> +        Cu.reportError(e);

I don't think this is enough error reporting - these should go to logs. Maybe have the ctor take a log? IIUC, almost everything that will be creating one of these will have a log handy (and maybe you could make it optional, using some default log in that case?)

::: services/common/async.js:239
(Diff revision 1)
> +      }
> +    })();
> +  }
> +
> +  promiseEmptyQueue() {
> +    return this.queue;

a total nit, but this name looks possibly confusing - it could mean "promise a new empty queue" or even "promise you have discarded all items"

Maybe something to reflect "enqueueCall" - say, "promiseCallsComplete" or similar?

And should this.queue be "private" (ie, this._queue)?

::: services/common/async.js:258
(Diff revision 1)
> +
> +  observe(subject, topic, data) {
> +    this.enqueueCall(() => this.obj.observe(subject, topic, data));
> +  }
> +
> +  promiseAllEventsHandled() {

and here, maybe promiseObserversComplete()?

However, this appears to only be called in tests, which seems very racey and a bit of a smell IMO - sync itself should be waiting. I'd think we certainly want to ensure all observers fired before we start syncing (eg, those which track changes) have completed before we actually start syncing. You can simulate this issue by awaiting a 1s timer in enqueueCall() and running TPS - it should pass (but obviously slower), but it fails.

::: services/common/tests/unit/test_tokenauthenticatedrequest.js:52
(Diff revision 1)
>    do_check_eq(message, req.response.body);
>  
> -  server.stop(run_next_test);
> +  await promiseStopServer(server);
>  });
> +
> +function promiseStopServer(server) {

this exists in head_helpers.js, so there should be no need to add this.

::: services/common/tests/unit/test_tokenserverclient.js:42
(Diff revision 1)
>  
>    let client = new TokenServerClient();
> -  let cb = Async.makeSpinningCallback();
>    let url = server.baseURI + "/1.0/foo/1.0";
> -  client.getTokenFromBrowserIDAssertion(url, "assertion", cb);
> -  let result = cb.wait();
> +  let result = await new Promise(res => {
> +    client.getTokenFromBrowserIDAssertion(url, "assertion", (err, val) => {

ISTM that it might be better to make getTokenFromBIDAssertion promise-based first? (ie, in a part 1 or different bug)

::: services/common/tests/unit/test_tokenserverclient.js:467
(Diff revision 1)
>    do_check_eq(callbackCount, 1);
>  
> -  server.stop(run_next_test);
> +  await promiseStopServer(server);
> +});
> +
> +function promiseStopServer(server) {

no need to add this.

::: services/sync/modules/addonsreconciler.js:121
(Diff revision 1)
>  this.AddonsReconciler = function AddonsReconciler() {
>    this._log = Log.repository.getLogger("Sync.AddonsReconciler");
>    let level = Svc.Prefs.get("log.logger.addonsreconciler", "Debug");
>    this._log.level = Log.Level[level];
>  
> +  this.queueCaller = Async.asyncQueueCaller();

I don't quite understand why we have a queueCaller here but the only things that wait for it to be drained are in tests? Specifically, I'm thinking that we may end up missing some events on shutdown meaning we don't track certain addon changes?

::: services/sync/modules/bookmark_repair.js:704
(Diff revision 1)
>      Svc.Obs.remove("weave:engine:sync:uploaded", this.onUploaded, this);
>      if (subject.failed) {
>        return;
>      }
>      log.debug(`bookmarks engine has uploaded stuff - creating a repair response`, subject);
> -    Async.promiseSpinningly(this._finishRepair());
> +    this._finishRepair();

Didn't the promiseSpinningly ensure we didn't return control to the sender of the observer until the promise was resolved, whereas now it doesn't?

::: services/sync/modules/browserid_identity.js:786
(Diff revision 1)
>  
>    /**
>     * @return a Hawk HTTP Authorization Header, lightly wrapped, for the .uri
>     * of a RESTRequest or AsyncResponse object.
>     */
> -  _getAuthenticationHeader(httpObject, method) {
> +  async _getAuthenticationHeader(httpObject, method) {

are these changes really related to observers?

::: services/sync/tps/extensions/tps/resource/tps.jsm:495
(Diff revision 1)
>            break;
>          case ACTION_VERIFY:
> -          Logger.AssertTrue(addon.find(state), "addon " + addon.id + " not found");
> +          Logger.AssertTrue((await addon.find(state)), "addon " + addon.id + " not found");
>            break;
>          case ACTION_VERIFY_NOT:
>            Logger.AssertFalse(addon.find(state), "addon " + addon.id + " is present, but it shouldn't be");

we need an |await| here. Note that even with this await added, test_addon_sanity.js fails for some other reason I didn't dig in to.
Attachment #8933481 - Flags: review?(markh) → review-
Comment on attachment 8933481 [details]
Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.

https://reviewboard.mozilla.org/r/204414/#review210030

oops - I also meant to say I didn't end up looking at all of this, and I still encourage Thom and Kit to take a look at it.
Sorry Thom, I didn't see your comments when I started my look.

(In reply to Thom Chiovoloni [:tcsc] from comment #5)
> The biggest question I have here is whether or not we need the complexity
> (and risk, as mentioned below) for serializing/queueing event handling.
> 
> What are we trying to avoid? It's worth noting that reentrant calls to
> observers are possible in the current system -- `observe` (and
> `notifyObservers`) calls are synchronous.

It's possible (eg, when an observer itself sends a notification), but uncommon in general.

> E.g. spinning an event loop only
> prevents the *next* observer for that event from getting called.
> Importantly, it does *not* prevent us from getting other notifications in
> the mean-time, which is what the queues do.

I think it does actually - eg, bookmarks listens for "bookmarks-restore-begin" and "bookmarks-restore-success". If we spin in "bookmarks-restore-begin" we aren't going to get "bookmarks-restore-success" until that observer returns. In a promise based world, we will (by default) return before the promise is complete, so could well get "bookmarks-restore-success" before the "bookmarks-restore-begin" promise resolves. While that specific case probably isn't a big deal, I can certainly see that it would be in general, and I don't want a foot-gun.

This is roughly the same issue I mentioned above about sync needing to wait for the queues as it start syncing, and possibly also at the end - if a promise is alive, say, adding an item to the tracker when Sync starts, it's going to be very difficult to rationalize how things will behave.

> Once concern I'd have is that a single 'stuck' promise here would break
> things forever.

IIUC, that's also true if we promiseSpinningly in an observer (and more generally, true almost any time we await a promise)

> 2. Recursive use of the queue. E.g. won't `q.enqueueCall(async() =>
> q.enqueueCall(() => console.log("foo")))` be stuck forever? It seems that
> way to me, and it seems like this could happen accidentally or something,
> IDK.

That's an interesting point.
Comment on attachment 8933481 [details]
Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.

https://reviewboard.mozilla.org/r/204414/#review209978

> Do we want to return here? Answer is probably 'no', but maybe tests will want to wait on it? Thoughts?

Actually we don't want to return both here and in enqueueCall. Calling enqueueCall recursively is fine if  enqueueCall is not awaiting on a promise returned by itself.
Comment on attachment 8933481 [details]
Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.

https://reviewboard.mozilla.org/r/204414/#review209986

> and here, maybe promiseObserversComplete()?
> 
> However, this appears to only be called in tests, which seems very racey and a bit of a smell IMO - sync itself should be waiting. I'd think we certainly want to ensure all observers fired before we start syncing (eg, those which track changes) have completed before we actually start syncing. You can simulate this issue by awaiting a 1s timer in enqueueCall() and running TPS - it should pass (but obviously slower), but it fails.

I have added calls to wait on these observers in engine's _syncStartups, but it is still necessary to wait when testing the trackers and co in tests sadly.
Comment on attachment 8933481 [details]
Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.

https://reviewboard.mozilla.org/r/204414/#review209986

> Didn't the promiseSpinningly ensure we didn't return control to the sender of the observer until the promise was resolved, whereas now it doesn't?

Yup but this is fine since _finishRepair doesn't mess with the bookmark engine itself and only adds/remove commands in the client engine.
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review213260

::: services/sync/modules/browserid_identity.js:386
(Diff revision 1)
>          });
>        }
>      } break;
>  
>      case fxAccountsCommon.ONLOGOUT_NOTIFICATION:
>        Async.promiseSpinningly(Weave.Service.startOver());

I'm not entirely sure what to do here: removing the event loop spinning here will make some tests (eg. test_oauth_tokens.js) fail.
We are calling startOver() in the background while shutting-down, because we clean-up after tests using FxAccounts#signOut().
We could wait explicitly for "start-over:finish" in tests, but it doesn't look great.
Comment on attachment 8933481 [details]
Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.

https://reviewboard.mozilla.org/r/204414/#review214070

This looks good, but I wouldn't mind another look

::: services/common/tests/unit/test_tokenserverclient.js:93
(Diff revision 2)
>    let client = new TokenServerClient();
>    let url = server.baseURI + "/1.0/foo/1.0";
>  
> -  function onResponse(error, token) {
> +  try {
> +    await client.getTokenFromBrowserIDAssertion(url, "assertion");
> +    do_throw("Should never get here.");

Here and below we should probably use Assert.rejects here with the 2nd param being a function that tests the exception (see bug 1357625 for a description of this)

::: services/common/tokenserverclient.js:258
(Diff revision 2)
> -          return;
> -        }
>  
> +    let response;
> -        try {
> +    try {
> -          cb(error, result);
> +      response = await getPromise;

can't you just await the promise directly?

::: services/common/tokenserverclient.js:264
(Diff revision 2)
> -
> -        cb = null;
> -      }
> +    }
>  
> -      try {
> +    try {
> -        client._processTokenResponse(this.response, callCallback);
> +      return this._processTokenResponse(response);

We should probably await here, otherwise exceptions thrown after the first await in that function will not be caught by the exception handler (but see below :)

::: services/common/tokenserverclient.js:279
(Diff revision 2)
>     * @param response
>     *        RESTResponse from token HTTP request.
> -   * @param cb
> -   *        The original callback passed to the public API.
>     */
> -  _processTokenResponse: function processTokenResponse(response, cb) {
> +  async _processTokenResponse(response) {

I don't think this is actually async (so the above comment probably doesn't apply if you can remove async here)
Attachment #8933481 - Flags: review?(markh)
Comment on attachment 8936637 [details]
Bug 633062 p3 - Remove event loop spinning from the EngineManager.

https://reviewboard.mozilla.org/r/207372/#review214074
Attachment #8936637 - Flags: review?(markh) → review+
Comment on attachment 8936638 [details]
Bug 633062 p1 - Introduce AsyncQueueCaller and AsyncObserver.

https://reviewboard.mozilla.org/r/207374/#review214076
Attachment #8936638 - Flags: review?(markh) → review+
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review214078

::: services/sync/modules/bookmark_repair.js:704
(Diff revision 1)
>      Svc.Obs.remove("weave:engine:sync:uploaded", this.onUploaded, this);
>      if (subject.failed) {
>        return;
>      }
>      log.debug(`bookmarks engine has uploaded stuff - creating a repair response`, subject);
> -    Async.promiseSpinningly(this._finishRepair());
> +    this._finishRepair();

I don't understand this change given `_finishRepair` is async

::: services/sync/modules/engines/addons.js:801
(Diff revision 1)
>  
> -  syncedByClient(item) {
> +  async syncedByClient(item) {
>      return !item.original.hidden &&
>             !item.original.isSystem &&
>             !(item.original.pendingOperations & AddonManager.PENDING_UNINSTALL) &&
>             this.engine.isAddonSyncable(item.original, true);

don't we need an await here?

::: services/sync/modules/service.js:444
(Diff revision 1)
>          break;
>        case "fxaccounts:device_disconnected":
>          data = JSON.parse(data);
>          if (!data.isLocalDevice) {
> -          Async.promiseSpinningly(this.clientsEngine.updateKnownStaleClients());
> +          // Refresh the known stale clients list in the background.
> +          this.clientsEngine.updateKnownStaleClients();

this looks suspect - is it really OK to return before the promise resolves, and what happens if it rejects?

::: toolkit/modules/tests/xpcshell/test_sqlite.js:17
(Diff revision 1)
>  Cu.import("resource://gre/modules/Sqlite.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/PromiseUtils.jsm");
>  
>  // To spin the event loop in test.
>  Cu.import("resource://services-common/async.js");

Can we kill this import?

::: toolkit/modules/tests/xpcshell/test_sqlite.js:628
(Diff revision 1)
>      expectOne = c._connectionData._pendingStatements.size;
>  
>      // Start another query, checking that after its statement has been created
>      // there are two statements in progress.
> -    let p = c.executeCached("SELECT 10, path from dirs");
> +    c.executeCached("SELECT 10, path from dirs").then(inner.resolve);
>      expectTwo = c._connectionData._pendingStatements.size;

This looks like a semantic change?
Attachment #8936639 - Flags: review?(markh)
Comment on attachment 8936640 [details]
Bug 633062 p5 - Remove event loop spinning from addonutils.js.

https://reviewboard.mozilla.org/r/207378/#review214088
Attachment #8936640 - Flags: review?(markh) → review+
Comment on attachment 8936641 [details]
Bug 633062 p6 - Remove event loop spinning from head_helper.js addon-related methods.

https://reviewboard.mozilla.org/r/207380/#review214090
Attachment #8936641 - Flags: review?(markh) → review+
Comment on attachment 8936642 [details]
Bug 633062 p7 - Make most of the addon reconciler async.

https://reviewboard.mozilla.org/r/207382/#review214092
Attachment #8936642 - Flags: review?(markh) → review+
Great work Ed, and thanks for splitting the patch up - it makes it much easier to review (and talk about one extreme to another - 1 patch -> 8 patches :)

I left the last one for later which I'll do ASAP
Comment on attachment 8936638 [details]
Bug 633062 p1 - Introduce AsyncQueueCaller and AsyncObserver.

https://reviewboard.mozilla.org/r/207374/#review215048

::: services/sync/modules/engines.js:137
(Diff revision 1)
>      let index = this._ignored.indexOf(id);
>      if (index != -1)
>        this._ignored.splice(index, 1);
>    },
>  
> -  _saveChangedID(id, when) {
> +  async getChangedIDs() {

On the one hand, since `SyncEngine.prototype.initialize` exists, I wonder if we could call `await this._tracker._storage.load()` there, and keep this all synchronous. On the other, you've already done the work to convert the observers here and in part 8, so it's OK if you want to leave it as is.
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review215510

::: services/sync/modules/bookmark_repair.js:704
(Diff revision 1)
>      Svc.Obs.remove("weave:engine:sync:uploaded", this.onUploaded, this);
>      if (subject.failed) {
>        return;
>      }
>      log.debug(`bookmarks engine has uploaded stuff - creating a repair response`, subject);
> -    Async.promiseSpinningly(this._finishRepair());
> +    this._finishRepair();

Did you mean to make this an async observer for `weave:engine:sync:uploaded` in part 8?
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review215524

::: services/sync/modules/engines/addons.js:604
(Diff revision 1)
>      if (ignoreRepoCheck || !AddonRepository.cacheEnabled) {
>        return true;
>      }
>  
> -    let cb = Async.makeSyncCallback();
> -    AddonRepository.getCachedAddonByID(addon.id, cb);
> +    let result = await new Promise(res => {
> +      AddonRepository.getCachedAddonByID(addon.id, res);

Not at all related to your patch, but I'm curious now, after seeing part 5...why do we use `AddonRepository` in some places, and `AddonManager` in others? It looks like `AddonManager` has promisified APIs, and similarly-named methods. And the comments mention `XPIProvider`, too, which we don't call directly...
Comment on attachment 8936638 [details]
Bug 633062 p1 - Introduce AsyncQueueCaller and AsyncObserver.

https://reviewboard.mozilla.org/r/207374/#review215562

::: services/sync/modules/engines.js:196
(Diff revision 1)
>      }
>      this._saveChangedIDs();
>      return true;
>    },
>  
>    clearChangedIDs() {

After looking at part 8, I'm also vaguely concerned this could race with `getChangedIDs`. In theory, it shouldn't; setting `data` also sets `dataReady`, and we don't call `{add, remove}ChangedID` and `clearChangedIDs` concurrently. We could also get confused if we call `addChangedID` and `removeChangedID` without waiting...
Comment on attachment 8936643 [details]
Bug 633062 p8 - Add Async observers.

https://reviewboard.mozilla.org/r/207384/#review215538

Thanks for doing this this work, Ed, and for splitting the changes up into multiple patches! That made it a lot easier to review.

Aside from the nits and questions, I think my biggest worry is the potential for deadlocks if an enqueued function waits on a nested function that calls `enqueueCall`, or if two queues are waiting on each other to proceed. It looks like we mainly need to queue observers to update changed IDs in the tracker, and I wonder if we could simplify and make that synchronous again. That leaves bookmark restores and the add-ons reconciler, which seems eaiser to audit.

Clearing review for now, but, if you think that this will be OK, and I'm worrying too much, feel free to re-flag. :-)

::: services/common/async.js:228
(Diff revision 1)
> +  constructor(log) {
> +    this._log = log;
> +    this._queue = Promise.resolve();
> +  }
> +
> +  enqueueCall(func) {

Let's add a comment that `func` should never await another function that calls `enqueueCall` on the same queue, or we'll deadlock.

::: services/sync/modules/engines/bookmarks.js:437
(Diff revision 1)
>      return undefined;
>    },
>  
>    async _syncStartup() {
> +    // Wait until all the events have been handled.
> +    await this._tracker.asyncObserver.promiseCallsComplete();

What does this do? The tracker can still keep receiving observer notifications during the sync; is this an optimization?

::: services/sync/modules/engines/clients.js:1058
(Diff revision 1)
>    },
>  };
>  
>  function ClientsTracker(name, engine) {
>    Tracker.call(this, name, engine);
> +  this.asyncObserver = Async.asyncObserver(this, this._log);

Do you think it's worth adding this to the base tracker?

::: services/sync/modules/engines/clients.js:1060
(Diff revision 1)
>  
>  function ClientsTracker(name, engine) {
>    Tracker.call(this, name, engine);
> +  this.asyncObserver = Async.asyncObserver(this, this._log);
>    Svc.Obs.add("weave:engine:start-tracking", this);
>    Svc.Obs.add("weave:engine:stop-tracking", this);

IIUC, https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/services/sync/modules/engines.js#69-70 adds these observers already. While you're here, could you double-check and remove if so, please? Also, if we move `asyncObserver` to the base, should it also pass `this.asyncObserver` instead of `this`?

::: services/sync/modules/engines/history.js:55
(Diff revision 1)
>  
>    syncPriority: 7,
>  
> +  async _syncStartup() {
> +    // Wait until all the events have been handled.
> +    await this._tracker.queueCaller.promiseCallsComplete();

So a tracker can have a `queueCaller` and an `asyncObserver`, and they'll use separate queues? Could we get in a situation where each is waiting on the other?
Attachment #8936643 - Flags: review?(kit)
Comment on attachment 8936643 [details]
Bug 633062 p8 - Add Async observers.

https://reviewboard.mozilla.org/r/207384/#review215592

This is looking good in general, but I think there's still scope to tighten and clean this up a little.

::: services/common/async.js:228
(Diff revision 1)
> +  constructor(log) {
> +    this._log = log;
> +    this._queue = Promise.resolve();
> +  }
> +
> +  enqueueCall(func) {

I need to think more about this, but the risk here seems real. IIUC, the key benefit of this approach is that new promise don't *start* until old ones have completed, whereas a simpler approach of (say) an array of promises we sequentually await on doesn't have that benefit.

I wonder if that matters in practice though - the biggest concern is that at key points we want to know that all promises are resolved, not necessarily that multiple might be in-flight at once.

(Although I'm probably contradicting myself here :)

::: services/sync/modules/engines/addons.js:210
(Diff revision 1)
>     * are complicated and we force a full refresh, just in case the listeners
>     * missed something.
>     */
>    async _syncStartup() {
> +    // Wait until all the events have been handled.
> +    await this._reconciler.queueCaller.promiseCallsComplete();

ISTM that we might need to ensure the queue is empty before (or as part of?) startListening/stopListening, as promises might be in-flight as these are called, which would cause unexpected behaviour (eg, a notification received due to us updating the addon during a sync is expected to be ignored, but now might complete after sync is complete, causing our tracker to get into the wrong state?)

::: services/sync/modules/engines/bookmarks.js:437
(Diff revision 1)
>      return undefined;
>    },
>  
>    async _syncStartup() {
> +    // Wait until all the events have been handled.
> +    await this._tracker.asyncObserver.promiseCallsComplete();

I think that doing this in the general case is correct - if promises from trackers are in-flight as Sync starts, we probably want them to complete before we (say) look at the list of changed IDs. Bookmarks are somewhat special though so probably don't specifically need this (although if a tracker promise bumps the score after Sync has started we might still get unexpected behavior).

IOW, I was expecting to see a call like this in the engine base-class rather than specifically for bookmarks.

::: services/sync/modules/engines/bookmarks.js:860
(Diff revision 1)
>  function BookmarksTracker(name, engine) {
> +  Tracker.call(this, name, engine);
>    this._batchDepth = 0;
>    this._batchSawScoreIncrement = false;
>    this._migratedOldEntries = false;
> -  Tracker.call(this, name, engine);
> +  this.asyncObserver = Async.asyncObserver(this, this._log);

should this be on the base tracker? I'm thinking of a future where Tracker.prototype.observe itself becomes async (eg, I could imagine startTracking ends up async)

::: services/sync/modules/engines/bookmarks.js:964
(Diff revision 1)
>    _needsMigration() {
>      return this.engine && this.engineIsEnabled() && this.engine.lastSync > 0;
>    },
>  
> -  observe: function observe(subject, topic, data) {
> +  async observe(subject, topic, data) {
>      Tracker.prototype.observe.call(this, subject, topic, data);

ie, the base class should also implement async observe, meaning we should await here.

::: services/sync/modules/engines/clients.js:378
(Diff revision 1)
>      this._knownStaleFxADeviceIds = Utils.arraySub(localClients, fxaClients);
>    },
>  
>    async _syncStartup() {
> +    // Wait until all the events have been handled.
> +    await this._tracker.asyncObserver.promiseCallsComplete();

As above, I think we should aim to get this into the base classes (and we probably want a similar wait as sync completes?)

::: services/sync/modules/engines/clients.js:1060
(Diff revision 1)
>  
>  function ClientsTracker(name, engine) {
>    Tracker.call(this, name, engine);
> +  this.asyncObserver = Async.asyncObserver(this, this._log);
>    Svc.Obs.add("weave:engine:start-tracking", this);
>    Svc.Obs.add("weave:engine:stop-tracking", this);

Strangely, the ClientsTracker observe() method doesn't seem to call the base, which seems wrong (that base impl also handles engines being disabled, which should remain impossible for the clients engine, but that seems tractable)

::: services/sync/modules/engines/history.js:55
(Diff revision 1)
>  
>    syncPriority: 7,
>  
> +  async _syncStartup() {
> +    // Wait until all the events have been handled.
> +    await this._tracker.queueCaller.promiseCallsComplete();

Yeah, I'm starting to think that queueCaller and asyncObserver are a little too smart for their own good TBH. Maybe asyncObserver should take the queueCaller as a param or something?

::: services/sync/tests/unit/test_addons_engine.js:128
(Diff revision 1)
>  
>    tracker.clearChangedIDs();
>  
>    _("Ensure reconciler changes are populated.");
>    let addon = await installAddon("test_bootstrap1_1");
> +  await reconciler.queueCaller.promiseCallsComplete();

these changes imply that install/uninstallAddon should just wait for the queueCaller, which also makes logicial sense (ie, that install/uninstall only resolve once all notifications related to the install have completed)

::: services/sync/tests/unit/test_password_engine.js:45
(Diff revision 1)
>    try {
>      let nonSyncableProps = new PropertyBag();
>      nonSyncableProps.setProperty("timeLastUsed", Date.now());
>      nonSyncableProps.setProperty("timesUsed", 3);
>      Services.logins.modifyLogin(login, nonSyncableProps);
> +    await engine._tracker.asyncObserver.promiseObserversComplete();

similarly here - pullNewChanges might be a good place to wait for the tracker's observers to be complete, to ensure that observers which might change the set of IDs are complete.
Attachment #8936643 - Flags: review?(markh)
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review214078

> don't we need an await here?

I thought that too! After eslint yelled at me I remembered that a AND in JS will return the 2nd expr if the first one is true, so we end up returning the promise :)

> this looks suspect - is it really OK to return before the promise resolves, and what happens if it rejects?

I think this is fine as nothing is really dependent on that list to be updated synchronously. We could be very unlucky and sync the client collection at the same time, but I think this risk is negligable.

> This looks like a semantic change?

I believe we are ok here since _pendingStatements.set will be called synchronously when calling executeCached[0].

[0] https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/toolkit/modules/Sqlite.jsm#823
Comment on attachment 8936643 [details]
Bug 633062 p8 - Add Async observers.

https://reviewboard.mozilla.org/r/207384/#review216216

::: services/common/async.js:228
(Diff revision 1)
> +  constructor(log) {
> +    this._log = log;
> +    this._queue = Promise.resolve();
> +  }
> +
> +  enqueueCall(func) {

I think it's important to await on promises sequentially for semantic reasons: we never wrote observer methods thinking they could be called concurently and I'm affraid it might bite us later.

::: services/sync/modules/engines/clients.js:378
(Diff revision 1)
>      this._knownStaleFxADeviceIds = Utils.arraySub(localClients, fxaClients);
>    },
>  
>    async _syncStartup() {
> +    // Wait until all the events have been handled.
> +    await this._tracker.asyncObserver.promiseCallsComplete();

Do we still want that?
With kit's suggestion to make the tracker base impl synchronous (load() in initialize()), only the bookmarks engine is left with an async observer.
In my opinion we should try to move away as much as possible from async observers since they add a bit more complexity.
Comment on attachment 8940319 [details]
Bug 633062 p3 - Remove event loop spinning from the tracker base implementation.

https://reviewboard.mozilla.org/r/210600/#review216314

::: services/sync/modules/engines.js
(Diff revision 1)
>    _beforeSave() {
>      return ensureDirectory(this._storage.path);
>    },
>  
>    get changedIDs() {
> -    Async.promiseSpinningly(this._storage.load());

I think we should add `this._storage.ensureDataReady()` here, to load the data synchronously as a last resort. OTOH, since that could mask a real bug where we're not waiting for an engine to initialize, maybe the right thing to do is leave this, and see if we get "Data is not ready." errors in telemety. Your call.
Comment on attachment 8936640 [details]
Bug 633062 p5 - Remove event loop spinning from addonutils.js.

https://reviewboard.mozilla.org/r/207378/#review216320

::: services/sync/modules/addonutils.js:44
(Diff revision 2)
>      // it is available. However, the addon.sourceURI rewriting won't be
>      // reflected in the AddonInstall, so we can't use it. If we ever get rid
>      // of sourceURI rewriting, we can avoid having to reconstruct the
>      // AddonInstall.
> +    return new Promise(res => {
> -    AddonManager.getInstallForURL(
> +      AddonManager.getInstallForURL(

Nit: Looks like bug 987512 changed `getInstallForURL` to support callback and promise usage; do you still need the wrapper?
Comment on attachment 8936643 [details]
Bug 633062 p8 - Add Async observers.

https://reviewboard.mozilla.org/r/207384/#review216328

Looks great, Ed, thanks for doing this! \o/ We talked at standup about keeping async observers close to where they're used, so it makes sense to keep them out of the base tracker. Ed also explained on IRC that it's good practice to wait on the queues at startup and when fetching changes, even though we don't always do that today.
Attachment #8936643 - Flags: review?(kit) → review+
Comment on attachment 8936643 [details]
Bug 633062 p8 - Add Async observers.

https://reviewboard.mozilla.org/r/207384/#review215956

I think the distinction between asyncObserver and asyncQueueCaller is a bit confusing and not really necessary, but oh well, this is probably fine.

::: services/sync/modules/engines/bookmarks.js:887
(Diff revision 2)
> -    Svc.Obs.add("bookmarks-restore-begin", this);
> -    Svc.Obs.add("bookmarks-restore-success", this);
> -    Svc.Obs.add("bookmarks-restore-failed", this);
> +    Svc.Obs.add("bookmarks-restore-begin", this.asyncObserver);
> +    Svc.Obs.add("bookmarks-restore-success", this.asyncObserver);
> +    Svc.Obs.add("bookmarks-restore-failed", this.asyncObserver);
>    },
>  
>    stopTracking() {

Do we need to promiseCallsComplete in here before stopping, as we do for the reconciler? (I think the answer is "no", but just to make sure)

More generally, do we really want to completely ignore bookmark restores that happen during a sync? (I guess that would probably fall under a different bug though...)

::: services/sync/tests/unit/test_addons_store.js:174
(Diff revision 2)
>  });
>  
>  add_task(async function test_apply_enabled_appDisabled() {
>    _("Ensures that changes to the userEnabled flag apply when the addon is appDisabled.");
>  
> -  let addon = await installAddon("test_install3"); // this addon is appDisabled by default.
> +  let addon = await installAddon("test_install3"); // this addon is appDisabled by defaul, reconcilert.

Typo in comment, the `t` should be after `defaul`, and not `reconciler`.
Attachment #8936643 - Flags: review?(tchiovoloni) → review+
Oh, please make sure this passes all tests in a full TPS run before landing.
Comment on attachment 8936643 [details]
Bug 633062 p8 - Add Async observers.

https://reviewboard.mozilla.org/r/207384/#review215956

> Typo in comment, the `t` should be after `defaul`, and not `reconciler`.

Actually, I think you should just delete `, reconciler`?
Comment on attachment 8933481 [details]
Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async.

https://reviewboard.mozilla.org/r/204414/#review218720

Thanks! And extra thanks for converting all the tests to use Assert.rejects!

::: services/common/tokenserverclient.js:215
(Diff revision 3)
> -   *       // Do other error handling.
> -   *       return;
> -   *     }
> -   *
>     *     let {
>     *       id: id, key: key, uid: uid, endpoint: endpoint, duration: duration

total nit, but we might as well change this comment to use modern destructuring (ie, let {id, key, ...} = result;)

::: services/common/tokenserverclient.js:253
(Diff revision 3)
> -
> -        try {
> +    try {
> -          cb(error, result);
> +      response = await new Promise((resolve, reject) => {
> +        req.get(function(err) {
> +          // Yes this is weird, the callback's |this| gets bound to the RESTRequest object.
> +          err ? reject(err) : resolve(this.response);

ISTM you could `reject(new TokenServerClientNetworkError(ex))` here and avoid the try/catch completely? I don't really care though.
Attachment #8933481 - Flags: review?(markh) → review+
Comment on attachment 8940319 [details]
Bug 633062 p3 - Remove event loop spinning from the tracker base implementation.

https://reviewboard.mozilla.org/r/210600/#review218722

::: services/sync/modules/engines.js:75
(Diff revision 1)
>    Svc.Obs.add("weave:engine:stop-tracking", this);
>  
>  };
>  
>  Tracker.prototype = {
> +  // Always call this at least once before using any of the other methods here!

Convert this to a `/*` style comment and indicate it returns a promise.

::: services/sync/modules/engines.js
(Diff revision 1)
>    _beforeSave() {
>      return ensureDirectory(this._storage.path);
>    },
>  
>    get changedIDs() {
> -    Async.promiseSpinningly(this._storage.load());

IMO, we should just change this getter to an async getChangedIDs() - otherwise, at the end of the patch series we have a tracker with a sync `changedIDs` getter, while engines have an async `getChangedIDs` function, which seems wrong and a bit of a foot-gun.

If we do that, then it would be fine to just `await this._storage.load()` here and we could presumably drop the new async initialize completely? (it seems that the engines will still have an async initialize, but the tracker doesn't need one IIUC)
Attachment #8940319 - Flags: review?(markh)
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review214078

> I think this is fine as nothing is really dependent on that list to be updated synchronously. We could be very unlucky and sync the client collection at the same time, but I think this risk is negligable.

I think at a minimum we want a .catch which logs an error.
(In reply to Edouard Oger [:eoger] from comment #36)
> ::: services/common/async.js:228
> (Diff revision 1)
> > +  constructor(log) {
> > +    this._log = log;
> > +    this._queue = Promise.resolve();
> > +  }
> > +
> > +  enqueueCall(func) {
> 
> I think it's important to await on promises sequentially for semantic
> reasons: we never wrote observer methods thinking they could be called
> concurently and I'm affraid it might bite us later.

That argument implies multiple asyncObservers is a problem and that there should be a single queue?
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review214078

> I think at a minimum we want a .catch which logs an error.

oops - I see you did add the catch in a later patch
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review218726

This is looking good, apart from `_finishRepair`. I *think* (but might be wrong) that you don't have the observer for the engines in this patch, so it would be fine to promiseSpinningly() here and fix it in a later patch.

::: services/sync/modules/bookmark_repair.js:705
(Diff revision 2)
>      }
>      Svc.Obs.remove("weave:engine:sync:uploaded", this.onUploaded, this);
>      if (subject.failed) {
>        return;
>      }
> +    queueCaller.enqueueCall(() => {

This worries me as although the queueCaller ensures there will not be 2 `_finishRepair()` calls in-flight, it doesn't ensure we wait for it to complete - so there's a risk that a next sync might start before it does. This is a problem as it manipulates the client engine's commands.

So I think we need to borrow asyncObserver() from the bookmarks engine so that we ensure the queue is drained before sync completes (which I hope I'll see happen in later patches :)
Attachment #8936639 - Flags: review?(markh)
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review218726

Yup this is starting to get too complicated for very little benefits (the responder runs rarely).
I reverted back to the promiseSpinningly() call, like you said we can address this later.
Comment on attachment 8940319 [details]
Bug 633062 p3 - Remove event loop spinning from the tracker base implementation.

https://reviewboard.mozilla.org/r/210600/#review218722

> IMO, we should just change this getter to an async getChangedIDs() - otherwise, at the end of the patch series we have a tracker with a sync `changedIDs` getter, while engines have an async `getChangedIDs` function, which seems wrong and a bit of a foot-gun.
> 
> If we do that, then it would be fine to just `await this._storage.load()` here and we could presumably drop the new async initialize completely? (it seems that the engines will still have an async initialize, but the tracker doesn't need one IIUC)

If we change changedIDs to an async function, that means our tracker base implementation is not synchronous anymore and we have to revert back to async tracker observers on pretty much all of our engines.
Imo we should try our best to avoid using async observers.
(In reply to Edouard Oger [:eoger] from comment #58)
> Yup this is starting to get too complicated for very little benefits (the
> responder runs rarely).
> I reverted back to the promiseSpinningly() call, like you said we can
> address this later.

Can't we address it later in the same patch queue by using the bookmark engine's asyncObserver? IIUC, that should solve the problem correctly and doesn't seem difficult.
(In reply to Edouard Oger [:eoger] from comment #59)
> If we change changedIDs to an async function, that means our tracker base
> implementation is not synchronous anymore and we have to revert back to
> async tracker observers on pretty much all of our engines.
> Imo we should try our best to avoid using async observers.

I don't really get that - either async observers are OK, or they aren't. Our most critical engines now use them, so I don't really see what problems are being solved by having the simple engines avoid it.
Attachment #8940319 - Attachment is obsolete: true
Comment on attachment 8936643 [details]
Bug 633062 p8 - Add Async observers.

https://reviewboard.mozilla.org/r/207384/#review221820

::: services/sync/modules/engines.js
(Diff revision 3)
>      beforeSave: () => this._beforeSave(),
>    });
>    this.ignoreAll = false;
>  
> -  Svc.Obs.add("weave:engine:start-tracking", this);
> +  this.asyncObserver = Async.asyncObserver(this, this._log);
> -  Svc.Obs.add("weave:engine:stop-tracking", this);

Quick note:
I simplified tracker activation/de-activations by removing these observer messages and instead relying on start()/stop() methods.

::: services/sync/modules/engines.js:690
(Diff revision 3)
>    set enabled(val) {
> -    if (!!val != this._enabled) {
> +    if (!!val == this._enabled) {
> +      return;
> +    }
> -      Svc.Prefs.set("engine." + this.prefName, !!val);
> +    Svc.Prefs.set("engine." + this.prefName, !!val);
> +    if (Svc.Prefs.get("engineStatusChanged." + this.prefName, false)) {

This used to be done in service.js using a pref observer.
At the time it was written it made sense, but now it doesn't so I moved the code here (simplifies a lot of things!).
Comment on attachment 8936639 [details]
Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/.

https://reviewboard.mozilla.org/r/207376/#review222616

Awesome, thanks!

::: services/sync/modules/engines/addons.js:801
(Diff revision 3)
>  
> -  syncedByClient(item) {
> +  async syncedByClient(item) {
>      return !item.original.hidden &&
>             !item.original.isSystem &&
>             !(item.original.pendingOperations & AddonManager.PENDING_UNINSTALL) &&
>             this.engine.isAddonSyncable(item.original, true);

I think we should comment the promise-returning magic here
Attachment #8936639 - Flags: review?(markh) → review+
Comment on attachment 8936643 [details]
Bug 633062 p8 - Add Async observers.

https://reviewboard.mozilla.org/r/207384/#review222618

Sorry for the delay and for going around so many revisions of this, but I think it was worthwhile - I think this version is excellent and seems less hacky than the others - nice work Ed!

::: services/sync/modules/addonsreconciler.js:60
(Diff revision 3)
>   * likely exists as a singleton. To AddonsEngine, it functions as a store and
>   * an entity which emits events for tracking.
>   *
>   * The usage pattern for instances of this class is:
>   *
>   *   let reconciler = new AddonsReconciler();

We should add ellipses here (ie, to imply that AddonsReconciler now takes args)

::: services/sync/modules/addonsreconciler.js:116
(Diff revision 3)
>   * events only come after the Addon Manager is closed or another view is
>   * switched to. In the case of Sync performing the uninstall, the uninstall
>   * events will occur immediately. However, we still see disabling events and
>   * heed them like they were normal. In the end, the state is proper.
>   */
> -this.AddonsReconciler = function AddonsReconciler() {
> +this.AddonsReconciler = function AddonsReconciler(queueCaller = null) {

It appears that only a single test file wants the ability to call this without that param - IMO it's probably better to just change that test and make the param mandatory (possibly via a test helper, such as NewAddonsReconciler or similar?)
Attachment #8936643 - Flags: review?(markh) → review+
TPS is passing, let's go!
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d69d0fddd09489439e867422da5fb35f5b1dacee -d f7e4fd351759: rebasing 444900:d69d0fddd094 "Bug 633062 p1 - Introduce AsyncQueueCaller and AsyncObserver. r=markh"
rebasing 444901:a346699911ac "Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async. r=markh"
rebasing 444902:0f17d66eede5 "Bug 633062 p3 - Remove event loop spinning from the EngineManager. r=markh"
rebasing 444903:e17b1fff4e2e "Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/. r=markh"
merging services/sync/modules/engines/addons.js
merging services/sync/tests/unit/test_addons_store.js
warning: conflicts while merging services/sync/tests/unit/test_addons_store.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/669c76bd2618
p1 - Introduce AsyncQueueCaller and AsyncObserver. r=markh
https://hg.mozilla.org/integration/autoland/rev/9c4cd2a6965c
p2 - Make getTokenFromBrowserIDAssertion async. r=markh
https://hg.mozilla.org/integration/autoland/rev/bb1030bdab48
p3 - Remove event loop spinning from the EngineManager. r=markh
https://hg.mozilla.org/integration/autoland/rev/a55a3647ea49
p4 - Remove miscellaneous uses of event loop spinning in services/. r=markh
https://hg.mozilla.org/integration/autoland/rev/968db19986aa
p5 - Remove event loop spinning from addonutils.js. r=markh
https://hg.mozilla.org/integration/autoland/rev/33f832b59e86
p6 - Remove event loop spinning from head_helper.js addon-related methods. r=markh
https://hg.mozilla.org/integration/autoland/rev/d80494d5bc36
p7 - Make most of the addon reconciler async. r=markh
https://hg.mozilla.org/integration/autoland/rev/d60ca66f2b62
p8 - Add Async observers. r=kitcambridge,markh,tcsc
Thank you, sorry I couldn't fix it directly on Autoland.
Flags: needinfo?(eoger)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbbacd08392c
p1 - Introduce AsyncQueueCaller and AsyncObserver. r=markh
https://hg.mozilla.org/integration/autoland/rev/2e9dc98eabf0
p2 - Make getTokenFromBrowserIDAssertion async. r=markh
https://hg.mozilla.org/integration/autoland/rev/35058a83dab0
p3 - Remove event loop spinning from the EngineManager. r=markh
https://hg.mozilla.org/integration/autoland/rev/549e1282ae8e
p4 - Remove miscellaneous uses of event loop spinning in services/. r=markh
https://hg.mozilla.org/integration/autoland/rev/c053f43da9e8
p5 - Remove event loop spinning from addonutils.js. r=markh
https://hg.mozilla.org/integration/autoland/rev/3d1ad6b3bbee
p6 - Remove event loop spinning from head_helper.js addon-related methods. r=markh
https://hg.mozilla.org/integration/autoland/rev/ee294cebe485
p7 - Make most of the addon reconciler async. r=markh
https://hg.mozilla.org/integration/autoland/rev/b4e9cd0d61ab
p8 - Add Async observers. r=kitcambridge,markh,tcsc
Flags: needinfo?(eoger)
Flags: needinfo?(eoger)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f2f6a6f69029e8ee383ddfd97930b93c3ab1ba59 -d ae4c9da146ee: rebasing 445090:f2f6a6f69029 "Bug 633062 p1 - Introduce AsyncQueueCaller and AsyncObserver. r=markh"
merging services/common/async.js
rebasing 445091:c1b360dc3b19 "Bug 633062 p2 - Make getTokenFromBrowserIDAssertion async. r=markh"
merging services/common/tests/unit/head_helpers.js
merging services/common/tests/unit/test_hawkclient.js
merging services/common/tests/unit/test_tokenserverclient.js
merging services/common/tokenserverclient.js
merging services/fxaccounts/tests/xpcshell/test_client.js
merging services/fxaccounts/tests/xpcshell/test_oauth_grant_client_server.js
merging services/sync/modules-testing/utils.js
merging services/sync/modules/browserid_identity.js
merging services/sync/tests/unit/head_helpers.js
merging services/sync/tests/unit/test_errorhandler_1.js
merging services/sync/tests/unit/test_errorhandler_2.js
merging services/sync/tests/unit/test_fxa_node_reassignment.js
merging services/sync/tests/unit/test_node_reassignment.js
rebasing 445092:da5eaf23c2e6 "Bug 633062 p3 - Remove event loop spinning from the EngineManager. r=markh"
merging services/sync/modules/engines.js
merging services/sync/tests/unit/head_helpers.js
merging services/sync/tests/unit/test_collections_recovery.js
merging services/sync/tests/unit/test_corrupt_keys.js
merging services/sync/tests/unit/test_enginemanager.js
merging services/sync/tests/unit/test_errorhandler_1.js
merging services/sync/tests/unit/test_errorhandler_2.js
merging services/sync/tests/unit/test_errorhandler_sync_checkServerError.js
merging services/sync/tests/unit/test_fxa_node_reassignment.js
merging services/sync/tests/unit/test_history_tracker.js
merging services/sync/tests/unit/test_hmac_error.js
merging services/sync/tests/unit/test_node_reassignment.js
merging services/sync/tests/unit/test_password_engine.js
merging services/sync/tests/unit/test_score_triggers.js
merging services/sync/tests/unit/test_service_sync_specified.js
merging services/sync/tests/unit/test_service_sync_updateEnabledEngines.js
merging services/sync/tests/unit/test_service_wipeClient.js
merging services/sync/tests/unit/test_syncedtabs.js
merging services/sync/tests/unit/test_syncscheduler.js
merging services/sync/tests/unit/test_telemetry.js
merging services/sync/tps/extensions/tps/resource/tps.jsm
rebasing 445093:fef7864e7a61 "Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/. r=markh"
merging services/common/tests/unit/test_tokenauthenticatedrequest.js
merging services/sync/modules/browserid_identity.js
merging services/sync/modules/collection_validator.js
merging services/sync/modules/engines/addons.js
merging services/sync/modules/resource.js
merging services/sync/modules/service.js
merging services/sync/modules/stages/enginesync.js
merging services/sync/tests/unit/test_addons_store.js
merging services/sync/tests/unit/test_addons_tracker.js
merging services/sync/tests/unit/test_browserid_identity.js
merging services/sync/tests/unit/test_clients_engine.js
merging services/sync/tests/unit/test_fxa_service_cluster.js
merging services/sync/tests/unit/test_history_store.js
merging services/sync/tests/unit/test_hmac_error.js
merging services/sync/tests/unit/test_service_cluster.js
merging services/sync/tests/unit/test_telemetry.js
merging services/sync/tps/extensions/tps/resource/modules/addons.jsm
merging services/sync/tps/extensions/tps/resource/modules/history.jsm
merging services/sync/tps/extensions/tps/resource/tps.jsm
merging toolkit/modules/tests/xpcshell/test_sqlite.js
rebasing 445094:596d1ce05ee8 "Bug 633062 p5 - Remove event loop spinning from addonutils.js. r=markh"
merging services/sync/modules/addonutils.js
merging services/sync/modules/engines/addons.js
merging services/sync/tests/unit/test_addon_utils.js
merging services/sync/tps/extensions/tps/resource/modules/addons.jsm
merging services/sync/tps/extensions/tps/resource/tps.jsm
rebasing 445095:4a8f97c9e321 "Bug 633062 p6 - Remove event loop spinning from head_helper.js addon-related methods. r=markh"
merging services/sync/tests/unit/head_helpers.js
merging services/sync/tests/unit/test_addons_engine.js
merging services/sync/tests/unit/test_addons_reconciler.js
merging services/sync/tests/unit/test_addons_store.js
merging services/sync/tests/unit/test_addons_tracker.js
rebasing 445096:e81b5bbfc8b8 "Bug 633062 p7 - Make most of the addon reconciler async. r=markh"
merging services/sync/modules/addonsreconciler.js
merging services/sync/modules/engines/addons.js
merging services/sync/tests/unit/test_addons_engine.js
merging services/sync/tests/unit/test_addons_store.js
rebasing 445097:26ad615fdd51 "Bug 633062 p8 - Add Async observers. r=kitcambridge,markh,tcsc" (tip)
merging browser/extensions/formautofill/FormAutofillSync.jsm
merging browser/extensions/formautofill/test/unit/test_sync.js
merging services/sync/modules/addonsreconciler.js
merging services/sync/modules/bookmark_repair.js
merging services/sync/modules/engines.js
merging services/sync/modules/engines/addons.js
merging services/sync/modules/engines/bookmarks.js
merging services/sync/modules/engines/clients.js
merging services/sync/modules/engines/extension-storage.js
merging services/sync/modules/engines/forms.js
merging services/sync/modules/engines/history.js
merging services/sync/modules/engines/passwords.js
merging services/sync/modules/engines/prefs.js
merging services/sync/modules/engines/tabs.js
merging services/sync/modules/service.js
merging services/sync/modules/stages/enginesync.js
merging services/sync/tests/unit/head_helpers.js
merging services/sync/tests/unit/test_412.js
merging services/sync/tests/unit/test_addons_engine.js
merging services/sync/tests/unit/test_addons_reconciler.js
merging services/sync/tests/unit/test_addons_store.js
merging services/sync/tests/unit/test_addons_tracker.js
merging services/sync/tests/unit/test_bookmark_duping.js
merging services/sync/tests/unit/test_bookmark_engine.js
merging services/sync/tests/unit/test_bookmark_store.js
merging services/sync/tests/unit/test_bookmark_tracker.js
merging services/sync/tests/unit/test_clients_engine.js
merging services/sync/tests/unit/test_engine.js
merging services/sync/tests/unit/test_engine_abort.js
merging services/sync/tests/unit/test_engine_changes_during_sync.js
merging services/sync/tests/unit/test_extension_storage_tracker.js
merging services/sync/tests/unit/test_forms_tracker.js
merging services/sync/tests/unit/test_fxa_node_reassignment.js
merging services/sync/tests/unit/test_history_engine.js
merging services/sync/tests/unit/test_history_tracker.js
merging services/sync/tests/unit/test_hmac_error.js
merging services/sync/tests/unit/test_node_reassignment.js
merging services/sync/tests/unit/test_password_engine.js
merging services/sync/tests/unit/test_password_tracker.js
merging services/sync/tests/unit/test_prefs_tracker.js
merging services/sync/tests/unit/test_score_triggers.js
merging services/sync/tests/unit/test_service_sync_updateEnabledEngines.js
merging services/sync/tests/unit/test_syncengine_sync.js
merging services/sync/tests/unit/test_tab_tracker.js
merging services/sync/tests/unit/test_telemetry.js
merging services/sync/tests/unit/test_tracker_addChanged.js
merging services/sync/tps/extensions/tps/resource/tps.jsm
merging toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js
warning: conflicts while merging services/sync/tests/unit/test_bookmark_tracker.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e52125316e1
p1 - Introduce AsyncQueueCaller and AsyncObserver. r=markh
https://hg.mozilla.org/integration/autoland/rev/20ff83289564
p2 - Make getTokenFromBrowserIDAssertion async. r=markh
https://hg.mozilla.org/integration/autoland/rev/0a8866062136
p3 - Remove event loop spinning from the EngineManager. r=markh
https://hg.mozilla.org/integration/autoland/rev/4b31c73a585f
p4 - Remove miscellaneous uses of event loop spinning in services/. r=markh
https://hg.mozilla.org/integration/autoland/rev/9798cbd03027
p5 - Remove event loop spinning from addonutils.js. r=markh
https://hg.mozilla.org/integration/autoland/rev/0780cf391c52
p6 - Remove event loop spinning from head_helper.js addon-related methods. r=markh
https://hg.mozilla.org/integration/autoland/rev/0888aa122056
p7 - Make most of the addon reconciler async. r=markh
https://hg.mozilla.org/integration/autoland/rev/590656f7b5c5
p8 - Add Async observers. r=kitcambridge,markh,tcsc
Backout by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58e3a1ab2815
Backed out 8 changesets as requested by eoger on a CLOSED TREE
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96c99982370c
p1 - Introduce AsyncQueueCaller and AsyncObserver. r=markh
https://hg.mozilla.org/integration/autoland/rev/da51899ad527
p2 - Make getTokenFromBrowserIDAssertion async. r=markh
https://hg.mozilla.org/integration/autoland/rev/1bfcacd0e174
p3 - Remove event loop spinning from the EngineManager. r=markh
https://hg.mozilla.org/integration/autoland/rev/9b01e0e6fb03
p4 - Remove miscellaneous uses of event loop spinning in services/. r=markh
https://hg.mozilla.org/integration/autoland/rev/b3c8b491dc8a
p5 - Remove event loop spinning from addonutils.js. r=markh
https://hg.mozilla.org/integration/autoland/rev/caa37a46c3c8
p6 - Remove event loop spinning from head_helper.js addon-related methods. r=markh
https://hg.mozilla.org/integration/autoland/rev/cf69aa2974a3
p7 - Make most of the addon reconciler async. r=markh
https://hg.mozilla.org/integration/autoland/rev/538c00dc842d
p8 - Add Async observers. r=kitcambridge,markh,tcsc
Well done, Ed! :-) Congrats on getting this over the finish line.
Thanks everyone for the guidance and the reviews! :)
Depends on: 1436636
Depends on: 1436637
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: