From 67765f88587ebab69cf7db1a96b98090a3aaa34e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fernando=20Jim=C3=A9nez=20Moreno?= Date: Thu, 29 Oct 2015 17:52:44 +0100 Subject: [PATCH] Bug 1212874 - Losing connection while syncing disables Firefox Sync. r=michielbdejong,yifan --- apps/system/js/sync_manager.js | 41 +++++++-- apps/system/test/unit/sync_manager_test.js | 91 +++++++++++++------ shared/js/sync/errors.js | 13 ++- tv_apps/smart-system/js/sync_manager.js | 38 ++++++-- .../test/unit/sync_manager_test.js | 91 ++++++++++++++----- 5 files changed, 204 insertions(+), 70 deletions(-) diff --git a/apps/system/js/sync_manager.js b/apps/system/js/sync_manager.js index f508c82841d2..a114f9ab7490 100644 --- a/apps/system/js/sync_manager.js +++ b/apps/system/js/sync_manager.js @@ -258,6 +258,15 @@ _handle_onsyncenabled: function(event) { this.debug('onsyncenabled observed'); this.updateState(); + + // If we got to this point with no user, something went wrong, so we + // need to disable Sync. + if (!this.user) { + this.debug('No user'); + Service.request('SyncStateMachine:disable'); + return; + } + this.registerSyncRequest().then(() => { this.debug('Sync request registered'); var from = event.detail && event.detail.from; @@ -343,11 +352,14 @@ // We don't update the state until we set the error. this.updateState(); - // If the error is not recoverable, we disable Sync. - if (SyncRecoverableErrors.indexOf(error) <= -1) { - this.debug('Unrecoverable error'); - Service.request('SyncStateMachine:disable'); + // If the error is recoverable we go back to the enabled state, + // otherwise, we disable Sync. + if (SyncRecoverableErrors.indexOf(error) > -1) { + Service.request('SyncStateMachine:enable'); + return; } + this.debug('Unrecoverable error'); + Service.request('SyncStateMachine:disable'); }, _handle_onsyncsyncing: function() { @@ -610,9 +622,13 @@ collections: collections }).then(result => { if (result.error) { - console.error('Error while trying to sync', result.error); - // XXX The sync app needs to propagate a less general error. - Service.request('SyncStateMachine:error', ERROR_SYNC_APP_GENERIC); + var error = result.error; + error = error.message ? error.message : error; + console.error('Error while trying to sync', error); + + error = SyncErrors[error] || ERROR_SYNC_APP_GENERIC; + + Service.request('SyncStateMachine:error', error); return; } @@ -746,8 +762,15 @@ user: { set: function(user) { - asyncStorage.setItem(SYNC_USER, user, () => { - this.store.set(SYNC_USER, user); + // We need to check if there's a user logged in during the enabled + // state handler, so we cannot wait for the asyncStorage write. + this.store.set(SYNC_USER, user); + asyncStorage.setItem(SYNC_USER, user, () => {}, () => { + // This should never happen, but if something goes wrong storing the + // user we delete the user from memory and disable Sync to avoid + // inconsistent states. + this.store.delete(SYNC_USER); + Service.request('SyncStateMachine:disable'); }); }, get: function() { diff --git a/apps/system/test/unit/sync_manager_test.js b/apps/system/test/unit/sync_manager_test.js index 6ed225bc06fe..a5ed07e82215 100644 --- a/apps/system/test/unit/sync_manager_test.js +++ b/apps/system/test/unit/sync_manager_test.js @@ -11,7 +11,6 @@ /* global ERROR_GET_FXA_ASSERTION */ /* global ERROR_INVALID_SYNC_ACCOUNT */ /* global ERROR_SYNC_APP_KILLED */ -/* global ERROR_SYNC_REQUEST */ /* global ERROR_UNVERIFIED_ACCOUNT */ /* global expect */ /* global FxAccountsClient */ @@ -20,6 +19,7 @@ /* global MockNavigatormozSetMessageHandler */ /* global MockNavigatorSettings */ /* global MockService */ +/* global SyncRecoverableErrors */ requireApp('system/js/service.js'); requireApp('system/test/unit/mock_asyncStorage.js'); @@ -115,6 +115,11 @@ suite('system/SyncManager >', () => { requestStub = this.sinon.stub(MockService, 'request', () => { return Promise.resolve(); }); + this.sinon.stub(syncManager, 'getAccount', () => { + syncManager.user = 'someone'; + return Promise.resolve('someone'); + }); + syncManager.start().then(done); }); }); @@ -361,6 +366,7 @@ suite('system/SyncManager >', () => { requestStub = this.sinon.stub(MockService, 'request', () => { return Promise.resolve(); }); + syncManager.user = 'someone'; }); teardown(() => { @@ -427,6 +433,16 @@ suite('system/SyncManager >', () => { done(); }); }); + + test('onsyncenabled received - no user should disable sync', done => { + syncManager.user = null; + window.dispatchEvent(new CustomEvent('onsyncenabled')); + setTimeout(() => { + this.sinon.assert.calledOnce(requestStub); + assert.ok(requestStub.calledWith('SyncStateMachine:disable')); + done(); + }); + }); }); suite('onsyncenabling', () => { @@ -573,7 +589,6 @@ suite('system/SyncManager >', () => { done(); }); }); - }); suite('onsyncerrored', () => { @@ -582,6 +597,23 @@ suite('system/SyncManager >', () => { var updateStateSpy; var requestStub; + const ERROR_SYNC_APP_KILLED = 'fxsync-error-app-killed'; + const ERROR_SYNC_APP_SYNC_IN_PROGRESS = + 'fxsync-error-app-fxsync-in-progress'; + const ERROR_SYNC_APP_GENERIC = 'fxsync-error-app-generic'; + const ERROR_SYNC_APP_TRY_LATER = 'fxsync-error-app-try-later'; + const ERROR_UNVERIFIED_ACCOUNT = 'fxsync-error-unverified-account'; + const ERROR_DIALOG_CLOSED_BY_USER = 'fxsync-error-dialog-closed-by-user'; + const ERROR_GET_FXA_ASSERTION = 'fxsync-error-get-fxa-assertion'; + const ERROR_INVALID_SYNC_ACCOUNT = 'fxsync-error-invalid-account'; + const ERROR_OFFLINE = 'fxsync-error-offline'; + const ERROR_REQUEST_SYNC_REGISTRATION = + 'fxsync-error-request-fxsync-registration'; + const ERROR_SYNC_INVALID_REQUEST_OPTIONS = + 'fxsync-error-invalid-request-options'; + const ERROR_SYNC_REQUEST = 'fxsync-error-request-failed'; + const ERROR_UNKNOWN = 'fxsync-error-unknown'; + suiteSetup(() => { syncManager = BaseModule.instantiate('SyncManager'); syncManager.start(); @@ -603,32 +635,35 @@ suite('system/SyncManager >', () => { requestStub.restore(); }); - test('onsyncerrored received - recoverable error', done => { - window.dispatchEvent(new CustomEvent('onsyncerrored', { - detail: { - args: [ERROR_SYNC_APP_KILLED] - } - })); - setTimeout(() => { - this.sinon.assert.calledOnce(updateStateSpy); - assert.equal(syncManager.error, ERROR_SYNC_APP_KILLED); - this.sinon.assert.notCalled(requestStub); - done(); - }); - }); - - test('onsyncerrored received - unrecoverable error', done => { - window.dispatchEvent(new CustomEvent('onsyncerrored', { - detail: { - args: [ERROR_SYNC_REQUEST] - } - })); - setTimeout(() => { - this.sinon.assert.calledOnce(updateStateSpy); - assert.equal(syncManager.error, ERROR_SYNC_REQUEST); - this.sinon.assert.calledOnce(requestStub); - assert.ok(requestStub.calledWith('SyncStateMachine:disable')); - done(); + [ERROR_SYNC_APP_KILLED, + ERROR_SYNC_APP_SYNC_IN_PROGRESS, + ERROR_SYNC_APP_GENERIC, + ERROR_SYNC_APP_TRY_LATER, + ERROR_UNVERIFIED_ACCOUNT, + ERROR_DIALOG_CLOSED_BY_USER, + ERROR_GET_FXA_ASSERTION, + ERROR_INVALID_SYNC_ACCOUNT, + ERROR_OFFLINE, + ERROR_REQUEST_SYNC_REGISTRATION, + ERROR_SYNC_INVALID_REQUEST_OPTIONS, + ERROR_SYNC_REQUEST, + ERROR_UNKNOWN].forEach(error => { + test(`onerrored received - ${error}`, done => { + window.dispatchEvent(new CustomEvent('onsyncerrored', { + detail: { + args: [error] + } + })); + setTimeout(() => { + this.sinon.assert.calledOnce(updateStateSpy); + assert.equal(syncManager.error, error); + if (SyncRecoverableErrors.indexOf(error) > -1) { + assert.ok(requestStub.calledWith('SyncStateMachine:enable')); + } else { + assert.ok(requestStub.calledWith('SyncStateMachine:disable')); + } + done(); + }); }); }); }); diff --git a/shared/js/sync/errors.js b/shared/js/sync/errors.js index e5cbb4e7c489..87002c108d9d 100644 --- a/shared/js/sync/errors.js +++ b/shared/js/sync/errors.js @@ -4,9 +4,11 @@ 'use strict'; +/* global ERROR_DIALOG_CLOSED_BY_USER */ /* global ERROR_INVALID_SYNC_ACCOUNT */ /* global ERROR_OFFLINE */ -/* global ERROR_DIALOG_CLOSED_BY_USER */ +/* global ERROR_SYNC_APP_TRY_LATER */ +/* global ERROR_UNKNOWN */ /** * These errors are shared by all Firefox Sync related modules. The key should @@ -45,6 +47,8 @@ ERROR_SYNC_APP_SYNC_IN_PROGRESS: 'fxsync-error-app-fxsync-in-progress', // Error while trying to sync. ERROR_SYNC_APP_GENERIC: 'fxsync-error-app-generic', + // The server cannot be reached. + ERROR_SYNC_APP_TRY_LATER: 'fxsync-error-app-try-later', // The user is logged in with an unverified account. ERROR_UNVERIFIED_ACCOUNT: 'fxsync-error-unverified-account' }; @@ -64,10 +68,13 @@ // Map between external (FxA, Syncto, kinto.js) and Sync errors. exports.SyncErrors = { 'invalid account': ERROR_INVALID_SYNC_ACCOUNT, - 'UNVERIFIED_ACCOUNT': ERROR_INVALID_SYNC_ACCOUNT, 'No keyFetchToken': ERROR_INVALID_SYNC_ACCOUNT, 'OFFLINE': ERROR_OFFLINE, - 'UI_ERROR': ERROR_DIALOG_CLOSED_BY_USER + 'try later': ERROR_SYNC_APP_TRY_LATER, + 'UI_ERROR': ERROR_DIALOG_CLOSED_BY_USER, + 'unauthorized': ERROR_INVALID_SYNC_ACCOUNT, + 'UNVERIFIED_ACCOUNT': ERROR_INVALID_SYNC_ACCOUNT, + 'unrecoverable': ERROR_UNKNOWN }; }(window)); diff --git a/tv_apps/smart-system/js/sync_manager.js b/tv_apps/smart-system/js/sync_manager.js index cfd7d051b3db..93e149757afe 100644 --- a/tv_apps/smart-system/js/sync_manager.js +++ b/tv_apps/smart-system/js/sync_manager.js @@ -216,8 +216,15 @@ }, set user(user) { - asyncStorage.setItem(SYNC_USER, user, () => { - this.store.set(SYNC_USER, user); + // We need to check if there's an user logged in during the enabled + // state handler, so we cannot wait for the asyncStorage write. + this.store.set(SYNC_USER, user); + asyncStorage.setItem(SYNC_USER, user, () => {}, () => { + // This should never happen, but if something goes wrong storing the + // user we delete the user from memory and disable Sync to avoid + // inconsistent states. + this.store.delete(SYNC_USER); + SyncStateMachine.disable(); }); }, @@ -303,6 +310,15 @@ onenabled(from) { this.debug('onenabled observed'); this.updateState(); + + // If we got to this point with no user, something went wrong, so we + // need to disable Sync. + if (!this.user) { + this.debug('No user'); + SyncStateMachine.disable(); + return; + } + this.registerSyncRequest().then(() => { this.debug('Sync request registered'); // After login in with a new account or rebooting the device, @@ -385,11 +401,14 @@ // We don't update the state until we set the error. this.updateState(); - // If the error is not recoverable, we disable Sync. - if (SyncRecoverableErrors.indexOf(error) <= -1) { - this.debug('Unrecoverable error'); - SyncStateMachine.disable(); + // If the error is recoverable we go back to the enabled state, + // otherwise, we disable Sync. + if (SyncRecoverableErrors.indexOf(error) > -1) { + SyncStateMachine.enable(); + return; } + this.debug('Unrecoverable error'); + SyncStateMachine.disable(); }, onsyncing() { @@ -682,9 +701,10 @@ var error = result.error; error = error.message ? error.message : error; console.error('Error while trying to sync', error); - // XXX The sync app needs to propagate a less general error. - // Bug 1210412 - SyncStateMachine.error(ERROR_SYNC_APP_GENERIC); + + error = SyncErrors[error] || ERROR_SYNC_APP_GENERIC; + + SyncStateMachine.error(error); return; } this.debug('Sync succeded'); diff --git a/tv_apps/smart-system/test/unit/sync_manager_test.js b/tv_apps/smart-system/test/unit/sync_manager_test.js index 9bea604f5ba0..04d8e2a7a119 100644 --- a/tv_apps/smart-system/test/unit/sync_manager_test.js +++ b/tv_apps/smart-system/test/unit/sync_manager_test.js @@ -9,7 +9,6 @@ /* global asyncStorage */ /* global ERROR_GET_FXA_ASSERTION */ /* global ERROR_SYNC_APP_KILLED */ -/* global ERROR_SYNC_REQUEST */ /* global ERROR_UNVERIFIED_ACCOUNT */ /* global expect */ /* global FxAccountsClient */ @@ -19,6 +18,7 @@ /* global MockNavigatorSettings */ /* global SettingsListener */ /* global SyncManager */ +/* global SyncRecoverableErrors */ /* global SyncStateMachine */ require('/shared/js/sync/errors.js'); @@ -156,6 +156,10 @@ suite('smart-system/SyncManager >', () => { this.sinon.stub(syncManager, 'observeSettings', () => { return Promise.resolve(); }); + this.sinon.stub(syncManager, 'getAccount', () => { + syncManager.user = 'someone'; + return Promise.resolve('someone'); + }); syncManager.start().then(done); }); }); @@ -166,6 +170,7 @@ suite('smart-system/SyncManager >', () => { enableSpy.reset(); syncSpy.reset(); unregisterSyncRequestStub.restore(); + registerSyncRequestStub.restore(); syncManager.stop(); }); @@ -199,7 +204,7 @@ suite('smart-system/SyncManager >', () => { nextSyncStateValue: 'disabled', shouldDisable: true }].forEach(config => { - test('sync.state ' + config.syncStateValue, () => { + test('sync.state ' + config.syncStateValue, done => { if (config.shouldDisable) { this.sinon.assert.calledOnce(updateStateSpy); assert.ok(updateStateSpy.calledWith('disabled')); @@ -212,6 +217,7 @@ suite('smart-system/SyncManager >', () => { setTimeout(() => { shouldDisable ? this.sinon.assert.notCalled(syncSpy) : this.sinon.assert.calledOnce(syncSpy); + done(); }); })(config.shouldDisable); @@ -440,6 +446,7 @@ suite('smart-system/SyncManager >', () => { registerSyncSpy = this.sinon.spy(navigator.sync, 'register'); updateStateSpy = this.sinon.spy(syncManager, 'updateState'); errorSpy = this.sinon.spy(SyncStateMachine, 'error'); + syncManager.user = 'someone'; }); teardown(() => { @@ -507,6 +514,19 @@ suite('smart-system/SyncManager >', () => { done(); }); }); + + test('onenabled received - no user should disable sync', done => { + teardown(() => { + disableStub.restore(); + }); + syncManager.user = null; + var disableStub = this.sinon.stub(SyncStateMachine, 'disable'); + SyncStateMachine.onenabled(); + setTimeout(() => { + this.sinon.assert.calledOnce(disableStub); + done(); + }); + }); }); suite('onenabling', () => { @@ -654,6 +674,24 @@ suite('smart-system/SyncManager >', () => { var updateStateSpy; var disableStub; + var enableStub; + + const ERROR_SYNC_APP_KILLED = 'fxsync-error-app-killed'; + const ERROR_SYNC_APP_SYNC_IN_PROGRESS = + 'fxsync-error-app-fxsync-in-progress'; + const ERROR_SYNC_APP_GENERIC = 'fxsync-error-app-generic'; + const ERROR_SYNC_APP_TRY_LATER = 'fxsync-error-app-try-later'; + const ERROR_UNVERIFIED_ACCOUNT = 'fxsync-error-unverified-account'; + const ERROR_DIALOG_CLOSED_BY_USER = 'fxsync-error-dialog-closed-by-user'; + const ERROR_GET_FXA_ASSERTION = 'fxsync-error-get-fxa-assertion'; + const ERROR_INVALID_SYNC_ACCOUNT = 'fxsync-error-invalid-account'; + const ERROR_OFFLINE = 'fxsync-error-offline'; + const ERROR_REQUEST_SYNC_REGISTRATION = + 'fxsync-error-request-fxsync-registration'; + const ERROR_SYNC_INVALID_REQUEST_OPTIONS = + 'fxsync-error-invalid-request-options'; + const ERROR_SYNC_REQUEST = 'fxsync-error-request-failed'; + const ERROR_UNKNOWN = 'fxsync-error-unknown'; suiteSetup(() => { syncManager = new SyncManager(); @@ -668,32 +706,43 @@ suite('smart-system/SyncManager >', () => { setup(() => { updateStateSpy = this.sinon.spy(syncManager, 'updateState'); disableStub = this.sinon.stub(SyncStateMachine, 'disable'); + enableStub = this.sinon.stub(SyncStateMachine, 'enable'); }); teardown(() => { updateStateSpy.restore(); disableStub.restore(); + enableStub.restore(); }); - test('onerrored received - recoverable error', done => { - SyncStateMachine.state = 'enabling'; - SyncStateMachine.error(ERROR_SYNC_APP_KILLED); - setTimeout(() => { - this.sinon.assert.calledOnce(updateStateSpy); - assert.equal(syncManager.error, ERROR_SYNC_APP_KILLED); - this.sinon.assert.notCalled(disableStub); - done(); - }); - }); - - test('onerrored received - unrecoverable error', done => { - SyncStateMachine.state = 'enabling'; - SyncStateMachine.error(ERROR_SYNC_REQUEST); - setTimeout(() => { - this.sinon.assert.calledOnce(updateStateSpy); - assert.equal(syncManager.error, ERROR_SYNC_REQUEST); - this.sinon.assert.calledOnce(disableStub); - done(); + [ERROR_SYNC_APP_KILLED, + ERROR_SYNC_APP_SYNC_IN_PROGRESS, + ERROR_SYNC_APP_GENERIC, + ERROR_SYNC_APP_TRY_LATER, + ERROR_UNVERIFIED_ACCOUNT, + ERROR_DIALOG_CLOSED_BY_USER, + ERROR_GET_FXA_ASSERTION, + ERROR_INVALID_SYNC_ACCOUNT, + ERROR_OFFLINE, + ERROR_REQUEST_SYNC_REGISTRATION, + ERROR_SYNC_INVALID_REQUEST_OPTIONS, + ERROR_SYNC_REQUEST, + ERROR_UNKNOWN].forEach(error => { + test(`onerrored received - ${error}`, done => { + SyncStateMachine.state = 'enabling'; + SyncStateMachine.error(error); + setTimeout(() => { + this.sinon.assert.calledOnce(updateStateSpy); + assert.equal(syncManager.error, error); + if (SyncRecoverableErrors.indexOf(error) > -1) { + this.sinon.assert.called(enableStub); + this.sinon.assert.notCalled(disableStub); + } else { + this.sinon.assert.called(disableStub); + this.sinon.assert.notCalled(enableStub); + } + done(); + }); }); }); });