From 3e4633d6e3557f8916b9c23336788a8ff6c0c97c Mon Sep 17 00:00:00 2001 From: Edouard Oger Date: Fri, 8 Dec 2017 14:41:02 -0500 Subject: [PATCH] Bug 633062 p4 - Remove miscellaneous uses of event loop spinning in services/. r=markh MozReview-Commit-ID: IDGWJevEHLK --HG-- extra : rebase_source : 8471048a68b9b6204402f3ea9039a21af8c3740c --- .../unit/test_tokenauthenticatedrequest.js | 10 ++- services/sync/modules/browserid_identity.js | 64 +++++++------------ services/sync/modules/collection_validator.js | 4 +- services/sync/modules/engines/addons.js | 21 +++--- services/sync/modules/resource.js | 11 ++-- services/sync/modules/service.js | 9 ++- services/sync/modules/stages/enginesync.js | 2 +- services/sync/tests/unit/test_addons_store.js | 23 ++++--- .../sync/tests/unit/test_addons_tracker.js | 30 ++++----- .../tests/unit/test_browserid_identity.js | 22 +++---- .../sync/tests/unit/test_clients_engine.js | 30 ++++----- .../tests/unit/test_fxa_service_cluster.js | 8 +-- .../sync/tests/unit/test_history_store.js | 12 ++-- services/sync/tests/unit/test_hmac_error.js | 14 ++-- .../sync/tests/unit/test_service_cluster.js | 30 ++------- services/sync/tests/unit/test_telemetry.js | 2 +- .../tps/resource/modules/addons.jsm | 14 ++-- .../tps/resource/modules/history.jsm | 1 - .../sync/tps/extensions/tps/resource/tps.jsm | 8 +-- toolkit/modules/tests/xpcshell/test_sqlite.js | 23 ++----- 20 files changed, 142 insertions(+), 196 deletions(-) diff --git a/services/common/tests/unit/test_tokenauthenticatedrequest.js b/services/common/tests/unit/test_tokenauthenticatedrequest.js index e59c58bd4f80..829c2e475e3d 100644 --- a/services/common/tests/unit/test_tokenauthenticatedrequest.js +++ b/services/common/tests/unit/test_tokenauthenticatedrequest.js @@ -11,7 +11,7 @@ function run_test() { run_next_test(); } -add_test(function test_authenticated_request() { +add_task(async function test_authenticated_request() { _("Ensure that sending a MAC authenticated GET request works as expected."); let message = "Great Success!"; @@ -41,12 +41,10 @@ add_test(function test_authenticated_request() { auth = sig.getHeader(); let req = new TokenAuthenticatedRESTRequest(uri, {id, key}, extra); - let cb = Async.makeSpinningCallback(); - req.get(cb); - let result = cb.wait(); + let error = await new Promise(res => req.get(res)); - Assert.equal(null, result); + Assert.equal(null, error); Assert.equal(message, req.response.body); - server.stop(run_next_test); + await promiseStopServer(server); }); diff --git a/services/sync/modules/browserid_identity.js b/services/sync/modules/browserid_identity.js index 0e859c737b7e..7805eefbf819 100644 --- a/services/sync/modules/browserid_identity.js +++ b/services/sync/modules/browserid_identity.js @@ -728,19 +728,14 @@ this.BrowserIDManager.prototype = { * @return a Hawk HTTP Authorization Header, lightly wrapped, for the .uri * of a RESTRequest or AsyncResponse object. */ - _getAuthenticationHeader(httpObject, method) { - let cb = Async.makeSpinningCallback(); - this._ensureValidToken().then(cb, cb); + async _getAuthenticationHeader(httpObject, method) { // Note that in failure states we return null, causing the request to be // made without authorization headers, thereby presumably causing a 401, // which causes Sync to log out. If we throw, this may not happen as // expected. try { - cb.wait(); + await this._ensureValidToken(); } catch (ex) { - if (Async.isShutdownException(ex)) { - throw ex; - } this._log.error("Failed to fetch a token for authentication", ex); return null; } @@ -796,9 +791,9 @@ BrowserIDClusterManager.prototype = { /** * Determine the cluster for the current user and update state. */ - setCluster() { + async setCluster() { // Make sure we didn't get some unexpected response for the cluster. - let cluster = this._findCluster(); + let cluster = await this._findCluster(); this._log.debug("Cluster value = " + cluster); if (cluster == null) { return false; @@ -818,8 +813,20 @@ BrowserIDClusterManager.prototype = { return true; }, - _findCluster() { - let endPointFromIdentityToken = () => { + async _findCluster() { + try { + // Ensure we are ready to authenticate and have a valid token. + await this.identity.whenReadyToAuthenticate.promise; + // We need to handle node reassignment here. If we are being asked + // for a clusterURL while the service already has a clusterURL, then + // it's likely a 401 was received using the existing token - in which + // case we just discard the existing token and fetch a new one. + if (this.service.clusterURL) { + log.debug("_findCluster has a pre-existing clusterURL, so discarding the current token"); + this.identity._token = null; + } + await this.identity._ensureValidToken(); + // The only reason (in theory ;) that we can end up with a null token // is when this._fxaService.canGetKeys() returned false. In turn, this // should only happen if the master-password is locked or the credentials @@ -840,30 +847,7 @@ BrowserIDClusterManager.prototype = { } log.debug("_findCluster returning " + endpoint); return endpoint; - }; - - // Spinningly ensure we are ready to authenticate and have a valid token. - let promiseClusterURL = () => { - return this.identity.whenReadyToAuthenticate.promise.then( - () => { - // We need to handle node reassignment here. If we are being asked - // for a clusterURL while the service already has a clusterURL, then - // it's likely a 401 was received using the existing token - in which - // case we just discard the existing token and fetch a new one. - if (this.service.clusterURL) { - log.debug("_findCluster has a pre-existing clusterURL, so discarding the current token"); - this.identity._token = null; - } - return this.identity._ensureValidToken(); - } - ).then(endPointFromIdentityToken - ); - }; - - let cb = Async.makeSpinningCallback(); - promiseClusterURL().then(function(clusterURL) { - cb(null, clusterURL); - }).catch(err => { + } catch (err) { log.info("Failed to fetch the cluster URL", err); // service.js's verifyLogin() method will attempt to fetch a cluster // URL when it sees a 401. If it gets null, it treats it as a "real" @@ -878,14 +862,10 @@ BrowserIDClusterManager.prototype = { // * On a real 401, we must return null. // * On any other problem we must let an exception bubble up. if (err instanceof AuthenticationError) { - // callback with no error and a null result - cb.wait() returns null. - cb(null, null); - } else { - // callback with an error - cb.wait() completes by raising an exception. - cb(err); + return null; } - }); - return cb.wait(); + throw err; + } }, getUserBaseURL() { diff --git a/services/sync/modules/collection_validator.js b/services/sync/modules/collection_validator.js index a39ffaed9210..f54209485e3a 100644 --- a/services/sync/modules/collection_validator.js +++ b/services/sync/modules/collection_validator.js @@ -123,7 +123,7 @@ class CollectionValidator { // Return whether or not a client item should be present on the server. Expected // to be overridden - syncedByClient(item) { + async syncedByClient(item) { return true; } @@ -186,7 +186,7 @@ class CollectionValidator { let seenClient = new Map(); for (let record of clientRecords) { let id = record[this.idProp]; - record.shouldSync = this.syncedByClient(record); + record.shouldSync = await this.syncedByClient(record); let clientHasPossibleDupe = seenClient.has(id); if (clientHasPossibleDupe && record.shouldSync) { // Only report duplicate client IDs for syncable records. diff --git a/services/sync/modules/engines/addons.js b/services/sync/modules/engines/addons.js index 4a3434d22239..b877e9eb53a9 100644 --- a/services/sync/modules/engines/addons.js +++ b/services/sync/modules/engines/addons.js @@ -183,7 +183,7 @@ AddonsEngine.prototype = { continue; } - if (!this.isAddonSyncable(addons[id])) { + if (!(await this.isAddonSyncable(addons[id]))) { continue; } @@ -239,6 +239,7 @@ AddonsEngine.prototype = { return this._reconciler.refreshGlobalState(); }, + // Returns a promise isAddonSyncable(addon, ignoreRepoCheck) { return this._store.isAddonSyncable(addon, ignoreRepoCheck); } @@ -291,7 +292,7 @@ AddonsStore.prototype = { // Ignore incoming records for which an existing non-syncable addon // exists. let existingMeta = this.reconciler.addons[record.addonID]; - if (existingMeta && !this.isAddonSyncable(existingMeta)) { + if (existingMeta && !(await this.isAddonSyncable(existingMeta))) { this._log.info("Ignoring incoming record for an existing but non-syncable addon", record.addonID); return; } @@ -465,7 +466,7 @@ AddonsStore.prototype = { let addons = this.reconciler.addons; for (let id in addons) { let addon = addons[id]; - if (this.isAddonSyncable(addon)) { + if ((await this.isAddonSyncable(addon))) { ids[addon.guid] = true; } } @@ -536,7 +537,7 @@ AddonsStore.prototype = { * for testing and validation). * @return Boolean indicating whether it is appropriate for Sync */ - isAddonSyncable: function isAddonSyncable(addon, ignoreRepoCheck = false) { + async isAddonSyncable(addon, ignoreRepoCheck = false) { // Currently, we limit syncable add-ons to those that are: // 1) In a well-defined set of types // 2) Installed in the current profile @@ -589,9 +590,9 @@ AddonsStore.prototype = { return true; } - let cb = Async.makeSyncCallback(); - AddonRepository.getCachedAddonByID(addon.id, cb); - let result = Async.waitForSyncCallback(cb); + let result = await new Promise(res => { + AddonRepository.getCachedAddonByID(addon.id, res); + }); if (!result) { this._log.debug(addon.id + " not syncable: add-on not found in add-on " + @@ -705,7 +706,7 @@ AddonsTracker.prototype = { return; } - if (!this.store.isAddonSyncable(addon)) { + if (!Async.promiseSpinningly(this.store.isAddonSyncable(addon))) { this._log.debug("Ignoring change because add-on isn't syncable: " + addon.id); return; @@ -783,10 +784,12 @@ class AddonValidator extends CollectionValidator { return item.applicationID === Services.appinfo.ID; } - syncedByClient(item) { + async syncedByClient(item) { return !item.original.hidden && !item.original.isSystem && !(item.original.pendingOperations & AddonManager.PENDING_UNINSTALL) && + // No need to await the returned promise explicitely: + // |expr1 && expr2| evaluates to expr2 if expr1 is true. this.engine.isAddonSyncable(item.original, true); } } diff --git a/services/sync/modules/resource.js b/services/sync/modules/resource.js index 9ae79fd7ac04..960011992d05 100644 --- a/services/sync/modules/resource.js +++ b/services/sync/modules/resource.js @@ -47,6 +47,7 @@ Resource.prototype = { /** * Callback to be invoked at request time to add authentication details. + * If the callback returns a promise, it will be awaited upon. * * By default, a global authenticator is provided. If this is set, it will * be used instead of the global one. @@ -93,7 +94,7 @@ Resource.prototype = { * @param {string} method HTTP method * @returns {Headers} */ - _buildHeaders(method) { + async _buildHeaders(method) { const headers = new Headers(this._headers); if (Resource.SEND_VERSION_INFO) { @@ -101,7 +102,7 @@ Resource.prototype = { } if (this.authenticator) { - const result = this.authenticator(this, method); + const result = await this.authenticator(this, method); if (result && result.headers) { for (const [k, v] of Object.entries(result.headers)) { headers.append(k.toLowerCase(), v); @@ -135,8 +136,8 @@ Resource.prototype = { * @param {object} signal AbortSignal instance * @returns {Request} */ - _createRequest(method, data, signal) { - const headers = this._buildHeaders(method); + async _createRequest(method, data, signal) { + const headers = await this._buildHeaders(method); const init = { cache: "no-store", // No cache. headers, @@ -164,7 +165,7 @@ Resource.prototype = { */ async _doRequest(method, data = null) { const controller = new AbortController(); - const request = this._createRequest(method, data, controller.signal); + const request = await this._createRequest(method, data, controller.signal); const responsePromise = fetch(request); // Rejects on network failure. let didTimeout = false; const timeoutId = setTimeout(() => { diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js index 1adfb12b42f7..cf529c4b3daf 100644 --- a/services/sync/modules/service.js +++ b/services/sync/modules/service.js @@ -441,7 +441,10 @@ Sync11Service.prototype = { 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().catch(e => { + this._log.error(e); + }); } break; case "weave:service:setup-complete": @@ -651,7 +654,7 @@ Sync11Service.prototype = { // Make sure we have a cluster to verify against. // This is a little weird, if we don't get a node we pretend // to succeed, since that probably means we just don't have storage. - if (this.clusterURL == "" && !this._clusterManager.setCluster()) { + if (this.clusterURL == "" && !(await this._clusterManager.setCluster())) { this.status.sync = NO_SYNC_NODE_FOUND; return true; } @@ -690,7 +693,7 @@ Sync11Service.prototype = { case 404: // Check that we're verifying with the correct cluster - if (allow40XRecovery && this._clusterManager.setCluster()) { + if (allow40XRecovery && (await this._clusterManager.setCluster())) { return await this.verifyLogin(false); } diff --git a/services/sync/modules/stages/enginesync.js b/services/sync/modules/stages/enginesync.js index 3c91a83f5d1b..04fc4180d6f1 100644 --- a/services/sync/modules/stages/enginesync.js +++ b/services/sync/modules/stages/enginesync.js @@ -51,7 +51,7 @@ EngineSynchronizer.prototype = { } // If we don't have a node, get one. If that fails, retry in 10 minutes. - if (!this.service.clusterURL && !this.service._clusterManager.setCluster()) { + if (!this.service.clusterURL && !(await this.service._clusterManager.setCluster())) { this.service.status.sync = NO_SYNC_NODE_FOUND; this._log.info("No cluster URL found. Cannot sync."); return; diff --git a/services/sync/tests/unit/test_addons_store.js b/services/sync/tests/unit/test_addons_store.js index a4ffd37d2706..0fecc1aebae0 100644 --- a/services/sync/tests/unit/test_addons_store.js +++ b/services/sync/tests/unit/test_addons_store.js @@ -250,16 +250,16 @@ add_task(async function test_apply_uninstall() { Assert.equal(null, addon); }); -add_test(function test_addon_syncability() { +add_task(async function test_addon_syncability() { _("Ensure isAddonSyncable functions properly."); Svc.Prefs.set("addons.trustedSourceHostnames", "addons.mozilla.org,other.example.com"); - Assert.ok(!store.isAddonSyncable(null)); + Assert.ok(!(await store.isAddonSyncable(null))); let addon = installAddon("test_bootstrap1_1"); - Assert.ok(store.isAddonSyncable(addon)); + Assert.ok((await store.isAddonSyncable(addon))); let dummy = {}; const KEYS = ["id", "syncGUID", "type", "scope", "foreignInstall", "isSyncable"]; @@ -267,22 +267,22 @@ add_test(function test_addon_syncability() { dummy[k] = addon[k]; } - Assert.ok(store.isAddonSyncable(dummy)); + Assert.ok((await store.isAddonSyncable(dummy))); dummy.type = "UNSUPPORTED"; - Assert.ok(!store.isAddonSyncable(dummy)); + Assert.ok(!(await store.isAddonSyncable(dummy))); dummy.type = addon.type; dummy.scope = 0; - Assert.ok(!store.isAddonSyncable(dummy)); + Assert.ok(!(await store.isAddonSyncable(dummy))); dummy.scope = addon.scope; dummy.isSyncable = false; - Assert.ok(!store.isAddonSyncable(dummy)); + Assert.ok(!(await store.isAddonSyncable(dummy))); dummy.isSyncable = addon.isSyncable; dummy.foreignInstall = true; - Assert.ok(!store.isAddonSyncable(dummy)); + Assert.ok(!(await store.isAddonSyncable(dummy))); dummy.foreignInstall = false; uninstallAddon(addon); @@ -318,7 +318,6 @@ add_test(function test_addon_syncability() { Svc.Prefs.reset("addons.trustedSourceHostnames"); - run_next_test(); }); add_task(async function test_get_all_ids() { @@ -336,9 +335,9 @@ add_task(async function test_get_all_ids() { let addon3 = installAddon("test_install3"); _("Ensure they're syncable."); - Assert.ok(store.isAddonSyncable(addon1)); - Assert.ok(store.isAddonSyncable(addon2)); - Assert.ok(store.isAddonSyncable(addon3)); + Assert.ok((await store.isAddonSyncable(addon1))); + Assert.ok((await store.isAddonSyncable(addon2))); + Assert.ok((await store.isAddonSyncable(addon3))); let ids = await store.getAllIDs(); diff --git a/services/sync/tests/unit/test_addons_tracker.js b/services/sync/tests/unit/test_addons_tracker.js index 52c44f447e47..73dedc96aa4e 100644 --- a/services/sync/tests/unit/test_addons_tracker.js +++ b/services/sync/tests/unit/test_addons_tracker.js @@ -118,26 +118,26 @@ add_task(async function test_track_user_disable() { Svc.Obs.notify("weave:engine:start-tracking"); Assert.equal(0, tracker.score); - let cb = Async.makeSyncCallback(); - - let listener = { - onDisabled(disabled) { - _("onDisabled"); - if (disabled.id == addon.id) { - AddonManager.removeAddonListener(listener); - cb(); + let disabledPromise = new Promise(res => { + let listener = { + onDisabled(disabled) { + _("onDisabled"); + if (disabled.id == addon.id) { + AddonManager.removeAddonListener(listener); + res(); + } + }, + onDisabling(disabling) { + _("onDisabling add-on"); } - }, - onDisabling(disabling) { - _("onDisabling add-on"); - } - }; - AddonManager.addAddonListener(listener); + }; + AddonManager.addAddonListener(listener); + }); _("Disabling add-on"); addon.userDisabled = true; _("Disabling started..."); - Async.waitForSyncCallback(cb); + await disabledPromise; let changed = tracker.changedIDs; Assert.equal(1, Object.keys(changed).length); diff --git a/services/sync/tests/unit/test_browserid_identity.js b/services/sync/tests/unit/test_browserid_identity.js index 28455d1ca9ec..1e5964201ace 100644 --- a/services/sync/tests/unit/test_browserid_identity.js +++ b/services/sync/tests/unit/test_browserid_identity.js @@ -144,7 +144,7 @@ add_task(async function test_initialializeWithNoKeys() { Assert.equal(globalBrowseridManager._token, null, "we don't have a token"); }); -add_test(function test_getResourceAuthenticator() { +add_task(async function test_getResourceAuthenticator() { _("BrowserIDManager supplies a Resource Authenticator callback which returns a Hawk header."); configureFxAccountIdentity(globalBrowseridManager); let authenticator = globalBrowseridManager.getResourceAuthenticator(); @@ -152,17 +152,16 @@ add_test(function test_getResourceAuthenticator() { let req = {uri: CommonUtils.makeURI( "https://example.net/somewhere/over/the/rainbow"), method: "GET"}; - let output = authenticator(req, "GET"); + let output = await authenticator(req, "GET"); Assert.ok("headers" in output); Assert.ok("authorization" in output.headers); Assert.ok(output.headers.authorization.startsWith("Hawk")); _("Expected internal state after successful call."); Assert.equal(globalBrowseridManager._token.uid, globalIdentityConfig.fxaccount.token.uid); - run_next_test(); } ); -add_test(function test_resourceAuthenticatorSkew() { +add_task(async function test_resourceAuthenticatorSkew() { _("BrowserIDManager Resource Authenticator compensates for clock skew in Hawk header."); // Clock is skewed 12 hours into the future @@ -211,7 +210,7 @@ add_test(function test_resourceAuthenticatorSkew() { let request = new Resource("https://example.net/i/like/pie/"); let authenticator = browseridManager.getResourceAuthenticator(); - let output = authenticator(request, "GET"); + let output = await authenticator(request, "GET"); dump("output" + JSON.stringify(output)); let authHeader = output.headers.authorization; Assert.ok(authHeader.startsWith("Hawk")); @@ -221,11 +220,9 @@ add_test(function test_resourceAuthenticatorSkew() { Assert.equal(getTimestamp(authHeader), now - 12 * HOUR_MS); Assert.ok( (getTimestampDelta(authHeader, now) - 12 * HOUR_MS) < 2 * MINUTE_MS); - - run_next_test(); }); -add_test(function test_RESTResourceAuthenticatorSkew() { +add_task(async function test_RESTResourceAuthenticatorSkew() { _("BrowserIDManager REST Resource Authenticator compensates for clock skew in Hawk header."); // Clock is skewed 12 hours into the future from our arbitary date @@ -255,7 +252,7 @@ add_test(function test_RESTResourceAuthenticatorSkew() { let request = new Resource("https://example.net/i/like/pie/"); let authenticator = browseridManager.getResourceAuthenticator(); - let output = authenticator(request, "GET"); + let output = await authenticator(request, "GET"); dump("output" + JSON.stringify(output)); let authHeader = output.headers.authorization; Assert.ok(authHeader.startsWith("Hawk")); @@ -265,8 +262,6 @@ add_test(function test_RESTResourceAuthenticatorSkew() { Assert.equal(getTimestamp(authHeader), now - 12 * HOUR_MS); Assert.ok( (getTimestampDelta(authHeader, now) - 12 * HOUR_MS) < 2 * MINUTE_MS); - - run_next_test(); }); add_task(async function test_ensureLoggedIn() { @@ -302,7 +297,7 @@ add_task(async function test_ensureLoggedIn() { Assert.equal(Status.login, LOGIN_SUCCEEDED, "final ensureLoggedIn worked"); }); -add_test(function test_tokenExpiration() { +add_task(async function test_tokenExpiration() { _("BrowserIDManager notices token expiration:"); let bimExp = new BrowserIDManager(); configureFxAccountIdentity(bimExp, globalIdentityConfig); @@ -312,7 +307,7 @@ add_test(function test_tokenExpiration() { let req = {uri: CommonUtils.makeURI( "https://example.net/somewhere/over/the/rainbow"), method: "GET"}; - authenticator(req, "GET"); + await authenticator(req, "GET"); // Mock the clock. _("Forcing the token to expire ..."); @@ -325,7 +320,6 @@ add_test(function test_tokenExpiration() { Assert.ok(bimExp._token.expiration < bimExp._now()); _("... means BrowserIDManager knows to re-fetch it on the next call."); Assert.ok(!bimExp.hasValidToken()); - run_next_test(); } ); diff --git a/services/sync/tests/unit/test_clients_engine.js b/services/sync/tests/unit/test_clients_engine.js index 5fb46bc27603..4992d5b99a88 100644 --- a/services/sync/tests/unit/test_clients_engine.js +++ b/services/sync/tests/unit/test_clients_engine.js @@ -1745,6 +1745,20 @@ add_task(async function test_other_clients_notified_on_first_sync() { add_task(async function device_disconnected_notification_updates_known_stale_clients() { const spyUpdate = sinon.spy(engine, "updateKnownStaleClients"); + + Services.obs.notifyObservers(null, "fxaccounts:device_disconnected", + JSON.stringify({ isLocalDevice: false })); + ok(spyUpdate.calledOnce, "updateKnownStaleClients should be called"); + spyUpdate.reset(); + + Services.obs.notifyObservers(null, "fxaccounts:device_disconnected", + JSON.stringify({ isLocalDevice: true })); + ok(spyUpdate.notCalled, "updateKnownStaleClients should not be called"); + + spyUpdate.restore(); +}); + +add_task(async function update_known_stale_clients() { const makeFakeClient = (id) => ({ id, fxaDeviceId: `fxa-${id}` }); const clients = [makeFakeClient("one"), makeFakeClient("two"), makeFakeClient("three")]; const stubRemoteClients = sinon.stub(engine._store, "_remoteClients").get(() => { @@ -1755,26 +1769,12 @@ add_task(async function device_disconnected_notification_updates_known_stale_cli }); engine._knownStaleFxADeviceIds = null; - Services.obs.notifyObservers(null, "fxaccounts:device_disconnected", - JSON.stringify({ isLocalDevice: false })); - ok(spyUpdate.calledOnce, "updateKnownStaleClients should be called"); + await engine.updateKnownStaleClients(); ok(clients[0].stale); ok(clients[1].stale); ok(!clients[2].stale); - spyUpdate.reset(); - - ok(engine._knownStaleFxADeviceIds); - Services.obs.notifyObservers(null, "fxaccounts:device_disconnected", - JSON.stringify({ isLocalDevice: false })); - ok(spyUpdate.calledOnce, "updateKnownStaleClients should be called"); - spyUpdate.reset(); - - Services.obs.notifyObservers(null, "fxaccounts:device_disconnected", - JSON.stringify({ isLocalDevice: true })); - ok(spyUpdate.notCalled, "updateKnownStaleClients should not be called"); stubRemoteClients.restore(); - spyUpdate.restore(); stubRefresh.restore(); }); diff --git a/services/sync/tests/unit/test_fxa_service_cluster.js b/services/sync/tests/unit/test_fxa_service_cluster.js index 9e0df43db6f0..93bc8b963740 100644 --- a/services/sync/tests/unit/test_fxa_service_cluster.js +++ b/services/sync/tests/unit/test_fxa_service_cluster.js @@ -19,9 +19,7 @@ add_task(async function test_findCluster() { await Assert.rejects(Service.identity.whenReadyToAuthenticate.promise, "should reject due to 500"); - Assert.throws(function() { - Service._clusterManager._findCluster(); - }); + await Assert.rejects(Service._clusterManager._findCluster()); _("_findCluster() returns null on authentication errors."); initializeIdentityWithTokenServerResponse({ @@ -34,7 +32,7 @@ add_task(async function test_findCluster() { await Assert.rejects(Service.identity.whenReadyToAuthenticate.promise, "should reject due to 401"); - let cluster = Service._clusterManager._findCluster(); + let cluster = await Service._clusterManager._findCluster(); Assert.strictEqual(cluster, null); _("_findCluster() works with correct tokenserver response."); @@ -54,7 +52,7 @@ add_task(async function test_findCluster() { await Service.identity.initializeWithCurrentIdentity(); await Service.identity.whenReadyToAuthenticate.promise; - cluster = Service._clusterManager._findCluster(); + cluster = await Service._clusterManager._findCluster(); // The cluster manager ensures a trailing "/" Assert.strictEqual(cluster, endpoint + "/"); diff --git a/services/sync/tests/unit/test_history_store.js b/services/sync/tests/unit/test_history_store.js index b20ef55986b0..1861d98aabeb 100644 --- a/services/sync/tests/unit/test_history_store.js +++ b/services/sync/tests/unit/test_history_store.js @@ -2,7 +2,6 @@ http://creativecommons.org/publicdomain/zero/1.0/ */ ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); -ChromeUtils.import("resource://services-common/async.js"); ChromeUtils.import("resource://services-common/utils.js"); ChromeUtils.import("resource://services-sync/engines/history.js"); ChromeUtils.import("resource://services-sync/service.js"); @@ -43,14 +42,17 @@ function isDateApproximately(actual, expected, skewMillis = 1000) { return actual >= lowerBound && actual <= upperBound; } -var engine = new HistoryEngine(Service); -Async.promiseSpinningly(engine.initialize()); -var store = engine._store; +let engine, store, fxuri, fxguid, tburi, tbguid; + async function applyEnsureNoFailures(records) { Assert.equal((await store.applyIncomingBatch(records)).length, 0); } -var fxuri, fxguid, tburi, tbguid; +add_task(async function setup() { + engine = new HistoryEngine(Service); + await engine.initialize(); + store = engine._store; +}); add_task(async function test_store() { _("Verify that we've got an empty store to work with."); diff --git a/services/sync/tests/unit/test_hmac_error.js b/services/sync/tests/unit/test_hmac_error.js index 1c1bdb3e2de3..62b00eae6b21 100644 --- a/services/sync/tests/unit/test_hmac_error.js +++ b/services/sync/tests/unit/test_hmac_error.js @@ -178,9 +178,9 @@ add_task(async function hmac_error_during_node_reassignment() { // This kicks off the actual test. Split into a function here to allow this // source file to broadly follow actual execution order. - function onwards() { + async function onwards() { _("== Invoking first sync."); - Async.promiseSpinningly(Service.sync()); + await Service.sync(); _("We should not simultaneously have data but no keys on the server."); let hasData = rotaryColl.wbo("flying") || rotaryColl.wbo("scotsman"); @@ -193,7 +193,7 @@ add_task(async function hmac_error_during_node_reassignment() { } _("Make sure that syncing again causes recovery."); - await new Promise(resolve => { + let callbacksPromise = new Promise(resolve => { onSyncFinished = function() { _("== First sync done."); _("---------------------------"); @@ -213,7 +213,7 @@ add_task(async function hmac_error_during_node_reassignment() { engine.lastSync = 0; hmacErrorCount = 0; - onSyncFinished = function() { + onSyncFinished = async function() { // Two rotary items, one client record... no errors. Assert.equal(hmacErrorCount, 0); @@ -227,12 +227,12 @@ add_task(async function hmac_error_during_node_reassignment() { server.stop(resolve); }; - Async.promiseSpinningly(Service.sync()); + Service.sync(); }, this); }; }; - - onwards(); }); + await onwards(); + await callbacksPromise; }); diff --git a/services/sync/tests/unit/test_service_cluster.js b/services/sync/tests/unit/test_service_cluster.js index d098f65b3e75..9f63eeabc762 100644 --- a/services/sync/tests/unit/test_service_cluster.js +++ b/services/sync/tests/unit/test_service_cluster.js @@ -5,21 +5,10 @@ ChromeUtils.import("resource://gre/modules/PromiseUtils.jsm"); ChromeUtils.import("resource://services-sync/service.js"); ChromeUtils.import("resource://services-sync/util.js"); -function do_check_throws(func) { - var raised = false; - try { - func(); - } catch (ex) { - raised = true; - } - Assert.ok(raised); -} - -add_test(function test_findCluster() { +add_task(async function test_findCluster() { syncTestLogging(); _("Test Service._findCluster()"); try { - let whenReadyToAuthenticate = PromiseUtils.defer(); Service.identity.whenReadyToAuthenticate = whenReadyToAuthenticate; whenReadyToAuthenticate.resolve(true); @@ -27,24 +16,21 @@ add_test(function test_findCluster() { Service.identity._ensureValidToken = () => Promise.reject(new Error("Connection refused")); _("_findCluster() throws on network errors (e.g. connection refused)."); - do_check_throws(function() { - Service._clusterManager._findCluster(); - }); + await Assert.rejects(Service._clusterManager._findCluster()); Service.identity._ensureValidToken = () => Promise.resolve(true); Service.identity._token = { endpoint: "http://weave.user.node" }; _("_findCluster() returns the user's cluster node"); - let cluster = Service._clusterManager._findCluster(); + let cluster = await Service._clusterManager._findCluster(); Assert.equal(cluster, "http://weave.user.node/"); } finally { Svc.Prefs.resetBranch(""); - run_next_test(); } }); -add_test(function test_setCluster() { +add_task(async function test_setCluster() { syncTestLogging(); _("Test Service._setCluster()"); try { @@ -54,20 +40,18 @@ add_test(function test_setCluster() { Service._clusterManager._findCluster = () => "http://weave.user.node/"; _("Set the cluster URL."); - Assert.ok(Service._clusterManager.setCluster()); + Assert.ok((await Service._clusterManager.setCluster())); Assert.equal(Service.clusterURL, "http://weave.user.node/"); _("Setting it again won't make a difference if it's the same one."); - Assert.ok(!Service._clusterManager.setCluster()); + Assert.ok(!(await Service._clusterManager.setCluster())); Assert.equal(Service.clusterURL, "http://weave.user.node/"); _("A 'null' response won't make a difference either."); Service._clusterManager._findCluster = () => null; - Assert.ok(!Service._clusterManager.setCluster()); + Assert.ok(!(await Service._clusterManager.setCluster())); Assert.equal(Service.clusterURL, "http://weave.user.node/"); - } finally { Svc.Prefs.resetBranch(""); - run_next_test(); } }); diff --git a/services/sync/tests/unit/test_telemetry.js b/services/sync/tests/unit/test_telemetry.js index 45d83fd139a8..491e9065e2df 100644 --- a/services/sync/tests/unit/test_telemetry.js +++ b/services/sync/tests/unit/test_telemetry.js @@ -430,7 +430,7 @@ add_task(async function test_engine_fail_ioerror() { add_task(async function test_clean_urls() { enableValidationPrefs(); - Service.engineManager.register(SteamEngine); + await Service.engineManager.register(SteamEngine); let engine = Service.engineManager.get("steam"); engine.enabled = true; let server = await serverForFoo(engine); diff --git a/services/sync/tps/extensions/tps/resource/modules/addons.jsm b/services/sync/tps/extensions/tps/resource/modules/addons.jsm index e0ffaa3e66f5..8f042e3ec9be 100644 --- a/services/sync/tps/extensions/tps/resource/modules/addons.jsm +++ b/services/sync/tps/extensions/tps/resource/modules/addons.jsm @@ -52,15 +52,15 @@ function Addon(TPS, id) { Addon.prototype = { addon: null, - uninstall: function uninstall() { + async uninstall() { // find our addon locally - let addon = Async.promiseSpinningly(AddonManager.getAddonByID(this.id)); + let addon = await AddonManager.getAddonByID(this.id); Logger.AssertTrue(!!addon, "could not find addon " + this.id + " to uninstall"); - Async.promiseSpinningly(AddonUtils.uninstallAddon(addon)); + await AddonUtils.uninstallAddon(addon); }, - find: function find(state) { - let addon = Async.promiseSpinningly(AddonManager.getAddonByID(this.id)); + async find(state) { + let addon = await AddonManager.getAddonByID(this.id); if (!addon) { Logger.logInfo("Could not find add-on with ID: " + this.id); @@ -98,8 +98,8 @@ Addon.prototype = { "Add-on was installed successfully: " + this.id); }, - setEnabled: function setEnabled(flag) { - Logger.AssertTrue(this.find(), "Add-on is available."); + async setEnabled(flag) { + Logger.AssertTrue((await this.find()), "Add-on is available."); let userDisabled; if (flag == STATE_ENABLED) { diff --git a/services/sync/tps/extensions/tps/resource/modules/history.jsm b/services/sync/tps/extensions/tps/resource/modules/history.jsm index f1ffd3cb2404..e134a8ababe6 100644 --- a/services/sync/tps/extensions/tps/resource/modules/history.jsm +++ b/services/sync/tps/extensions/tps/resource/modules/history.jsm @@ -15,7 +15,6 @@ ChromeUtils.import("resource://gre/modules/Services.jsm"); ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm"); ChromeUtils.import("resource://gre/modules/PlacesSyncUtils.jsm"); ChromeUtils.import("resource://tps/logger.jsm"); -ChromeUtils.import("resource://services-common/async.js"); var DumpHistory = async function TPS_History__DumpHistory() { let query = PlacesUtils.history.getNewQuery(); diff --git a/services/sync/tps/extensions/tps/resource/tps.jsm b/services/sync/tps/extensions/tps/resource/tps.jsm index bccdd1bb8bf5..f1aa1cbfff6d 100644 --- a/services/sync/tps/extensions/tps/resource/tps.jsm +++ b/services/sync/tps/extensions/tps/resource/tps.jsm @@ -487,16 +487,16 @@ var TPS = { addon.install(); break; case ACTION_DELETE: - addon.uninstall(); + await addon.uninstall(); 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"); + Logger.AssertFalse((await addon.find(state)), "addon " + addon.id + " is present, but it shouldn't be"); break; case ACTION_SET_ENABLED: - Logger.AssertTrue(addon.setEnabled(state), "addon " + addon.id + " not found"); + Logger.AssertTrue((await addon.setEnabled(state)), "addon " + addon.id + " not found"); break; default: throw new Error("Unknown action for add-on: " + action); diff --git a/toolkit/modules/tests/xpcshell/test_sqlite.js b/toolkit/modules/tests/xpcshell/test_sqlite.js index de378aebf858..0d17e1178bbd 100644 --- a/toolkit/modules/tests/xpcshell/test_sqlite.js +++ b/toolkit/modules/tests/xpcshell/test_sqlite.js @@ -11,9 +11,7 @@ ChromeUtils.import("resource://gre/modules/FileUtils.jsm"); ChromeUtils.import("resource://gre/modules/Services.jsm"); ChromeUtils.import("resource://gre/modules/Sqlite.jsm"); ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); - -// To spin the event loop in test. -ChromeUtils.import("resource://services-common/async.js"); +ChromeUtils.import("resource://gre/modules/PromiseUtils.jsm"); function sleep(ms) { return new Promise(resolve => { @@ -645,9 +643,6 @@ add_task(async function test_in_progress_counts() { let expectOne; let expectTwo; - // Please forgive me. - let inner = Async.makeSpinningCallback(); - let outer = Async.makeSpinningCallback(); // We want to make sure that two queries executing simultaneously // result in `_pendingStatements.size` reaching 2, then dropping back to 0. @@ -655,6 +650,7 @@ add_task(async function test_in_progress_counts() { // To do so, we kick off a second statement within the row handler // of the first, then wait for both to finish. + let inner = PromiseUtils.defer(); await c.executeCached("SELECT * from dirs", null, function onRow() { // In the onRow handler, we're still an outstanding query. // Expect a single in-progress entry. @@ -662,22 +658,11 @@ add_task(async function test_in_progress_counts() { // 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; - - // Now wait for it to be done before we return from the row handler … - p.then(function onInner() { - inner(); - }); - }).then(function onOuter() { - // … and wait for the inner to be done before we finish … - inner.wait(); - outer(); }); - // … and wait for both queries to have finished before we go on and - // test postconditions. - outer.wait(); + await inner.promise; Assert.equal(expectOne, 1); Assert.equal(expectTwo, 2);