From 15f6170bed1cf64537339d698fb78b21b98baeae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Thu, 29 Aug 2019 12:39:00 +0000 Subject: [PATCH 1/3] [FIX] bus: background slave tabs should not loose notifications Before this commit, some notifications may be lost while forwarding them between browser tabs by means of the local storage (i.e. from the master tab to slave tabs). This happened when some slave tabs are in the background and are throttled by the browser: the master tab writes twice in the local storage (to notify slave tabs of new notifications), and slave tabs drops the 1st notification. This is caused by the mismatch between the new value in the local storage event and the actual value in the local storage. For example: ``` t(0) - Master tab receives `message` notification; t(1) - Master tab writes notifications (= `message`) in local storage; t(2) - Master tab receives `message_seen` notification; t(3) - Master tab writes notifications (= `message_seen`) in local storage; t(4) - Slave tab detects new notifications (= `message`) from local storage event, but drops it since it does not match actual local storage value (= `message_seen`); t(5) - Slave tab detects new notifications (= `message_seen`) from local storage event, and handles it since it matches actual local storage value (= `message_seen`); => Notifications received + handled: - master tab: `message`, `message_seen` - slave tab: `message_seen` => Slave tab lost "message" notification. ``` This commit fixes the issue by letting all tabs handle any local storage events, regardless of the actual value in the local storage. In the example above, this means the slave tab will not drop the notification `message` at t(4). Task-ID 2044231 Closes #36177 --- addons/web/static/src/js/core/local_storage.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/addons/web/static/src/js/core/local_storage.js b/addons/web/static/src/js/core/local_storage.js index 0ae98498b31..ecc0cb7b78f 100644 --- a/addons/web/static/src/js/core/local_storage.js +++ b/addons/web/static/src/js/core/local_storage.js @@ -35,12 +35,10 @@ try { var newValue = e.originalEvent.newValue; try { JSON.parse(newValue); - if (localStorage.getItem(key) === newValue) { - storage.trigger('storage', { - key: key, - newValue: newValue, - }); - } + storage.trigger('storage', { + key: key, + newValue: newValue, + }); } catch (error) {} }); return storage; From 7bdcef9c1c4d42f39302403e1c5e56682788022c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Thu, 29 Aug 2019 16:27:47 +0000 Subject: [PATCH 2/3] [FIX] web: add support for `localStorage.key()` and `.length` These operations on local storage are necessary in order to loop on all local storage keys. Closes #36177 --- .../static/src/js/core/abstract_storage_service.js | 13 +++++++++++++ addons/web/static/src/js/core/ram_storage.js | 14 ++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/addons/web/static/src/js/core/abstract_storage_service.js b/addons/web/static/src/js/core/abstract_storage_service.js index ef828b5dd5c..5d724f87911 100644 --- a/addons/web/static/src/js/core/abstract_storage_service.js +++ b/addons/web/static/src/js/core/abstract_storage_service.js @@ -44,6 +44,19 @@ var AbstractStorageService = AbstractService.extend({ var val = this.storage.getItem(key); return val ? JSON.parse(val) : defaultValue; }, + /** + * @param {integer} index + * @return {string} + */ + key: function (index) { + return this.storage.key(index); + }, + /** + * @return {integer} + */ + length: function () { + return this.storage.length; + }, /** * Removes the given key from the storage * diff --git a/addons/web/static/src/js/core/ram_storage.js b/addons/web/static/src/js/core/ram_storage.js index 9bf0fc29f93..abad86c4538 100644 --- a/addons/web/static/src/js/core/ram_storage.js +++ b/addons/web/static/src/js/core/ram_storage.js @@ -31,6 +31,7 @@ var RamStorage = Class.extend(mixins.EventDispatcherMixin, { */ clear: function () { this.storage = Object.create(null); + this.length = 0; }, /** * Returns the value associated with a given key in the storage @@ -41,12 +42,22 @@ var RamStorage = Class.extend(mixins.EventDispatcherMixin, { getItem: function (key) { return this.storage[key]; }, + /** + * @param {integer} index + * @return {string} + */ + key: function (index) { + return _.keys(this.storage)[index]; + }, /** * Removes the given key from the storage * * @param {string} key */ removeItem: function (key) { + if (key in this.storage) { + this.length--; + } delete this.storage[key]; this.trigger('storage', {key: key, newValue: null}); }, @@ -58,6 +69,9 @@ var RamStorage = Class.extend(mixins.EventDispatcherMixin, { * @param {string} value */ setItem: function (key, value) { + if (!(key in this.storage)) { + this.length++; + } this.storage[key] = value; this.trigger('storage', {key: key, newValue: value}); }, From 40b57ca824e967a9221f7834e4c9b3db7a4849d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Thu, 29 Aug 2019 16:28:07 +0000 Subject: [PATCH 3/3] [FIX] bus, mail: correctly synchronize document chat windows Before this commit, there could be an infinite loop when changing the chat window state of a document chat window. This is caused when mutiple tabs overwrite the chat window state with a different value everytime, resulting to an infinite loop. Accross multiple tabs, `setItem` and `onStorage` are not synchronous. That's why tabs should communicate through local storage events instead of actual value in the local storage. To illustrate the issue, suppose there are 2 tabs (T1 and T2) with one document chat windows (C) and the following user interactions: - T1 and T2 both have C open - on T1, user folds C - on T2, user folds C Now let's add an example of local storage (LS) logics that could apply from above example: - T1 and T2 both have C open - on T1, user folds C [1] T1 writes 'C folded' in LS - on T2, user folds C [2] T2 writes 'C folded' in LS [3] T2 detects 'C folded' in LS (from [1]) => T2 writes 'C folded' in LS [4] T1 detects 'C folded' in LS (from [2]) => T1 writes 'C folded' in LS [5] T1 detects 'C folded' in LS (from [3]) => T1 writes 'C folded' in LS [6] T2 detects 'C folded' in LS (from [4]) => T2 writes 'C folded' in LS etc... This scenario could have been easily prevented by not writing on local storage when the chat window state has not changed. However, this wouldn't fix this scenario that also introduces an infinite loop: - T1 and T2 both have C open - on T1, user folds C [1] T1 writes 'C folded' in LS - on T2, user folds C [2] T2 writes 'C folded' in LS - on T2, user unfolds C [3] T2 writes 'C unfolded' in LS [4] T2 detects 'C folded' in LS (from [1]) => T2 writes 'C folded' in LS [5] T1 detects 'C unfolded' in LS (from [3]) => T1 writes 'C unfolded' in LS [6] T2 detects 'C unfolded' in LS (from [5]) => T2 writes 'C unfolded' in LS [7] T1 detects 'C folded' in LS (from [4]) => T1 writes 'C folded' in LS [8] T2 detects 'C folded' in LS (from [7]) => T2 writes 'C folded' in LS [9] T1 detects 'C unfolded' in LS (from [6]) => T1 writes 'C folded' in LS etc... This commit fixes the infinite loop issue: - by turning local storage read during cross-tab communication into local storage event read instead. This should prevent race conditions based on read/write operations in local, since their order is not guaranteed accros tabs. - by isolating document chat window state to their own local storage entry. This should prevent a tab from notifying a chat window state that it didn't change, which may also result in an infinite loop. Some tests were adapted from the changes above. Also, some tests did pass thanks to local storage being mocked by a ram storage: the handler `_onStorage` should have been called, but it didn't because it was registered on the actual local storage instead of the ram storage. In order to make tests pass again, the following additional changes were required: - Cross-tab bus service now exports the unique tab ID with `getTabId`. - document thread entries in local storage now track the tab ID, so that self-tab ignore their own local storage writes. Task-Id 2034997 Closes #36177 --- addons/bus/static/src/js/crosstab_bus.js | 6 ++ .../src/js/models/threads/document_thread.js | 27 ++++++- .../services/mail_document_thread_manager.js | 73 +++++++++++++------ .../src/js/services/mail_window_manager.js | 6 +- .../tests/document_thread_window_tests.js | 20 +++-- 5 files changed, 99 insertions(+), 33 deletions(-) diff --git a/addons/bus/static/src/js/crosstab_bus.js b/addons/bus/static/src/js/crosstab_bus.js index 46643cad0de..9d0e413ab94 100644 --- a/addons/bus/static/src/js/crosstab_bus.js +++ b/addons/bus/static/src/js/crosstab_bus.js @@ -79,6 +79,12 @@ var CrossTabBus = Longpolling.extend({ this._super.apply(this, arguments); this._callLocalStorage('setItem', 'channels', this._channels); }, + /** + * @return {string} + */ + getTabId: function () { + return this._id; + }, /** * Tells whether this bus is related to the master tab. * diff --git a/addons/mail/static/src/js/models/threads/document_thread.js b/addons/mail/static/src/js/models/threads/document_thread.js index 18f1e4fb762..3d91b786cef 100644 --- a/addons/mail/static/src/js/models/threads/document_thread.js +++ b/addons/mail/static/src/js/models/threads/document_thread.js @@ -62,9 +62,16 @@ var DocumentThread = Thread.extend({ * shared between tabs, and restored on F5. * * @override + * @param {Object} [options] + * @param {boolean} [options.skipCrossTabSync=false] if set, it should + * not notify other tabs from updating document thread state. This happens + * in case the `close` operation comes from local storage event. */ - close: function () { + close: function (options) { this._super.apply(this, arguments); + if (options && options.skipCrossTabSync) { + return; + } this.call('mail_service', 'updateDocumentThreadState', this._id, { name: this.getName(), windowState: 'closed', @@ -75,9 +82,16 @@ var DocumentThread = Thread.extend({ * shared between tabs, and restored on F5. * * @override + * @param {Object} [options] + * @param {boolean} [options.skipCrossTabSync=false] if set, it should + * not notify other tabs from updating document thread state. This happens + * in case the `detach` operation comes from local storage event. */ - detach: function () { + detach: function (options) { this._super.apply(this, arguments); + if (options && options.skipCrossTabSync) { + return; + } var windowState = this._folded ? 'folded' : 'open'; this.call('mail_service', 'updateDocumentThreadState', this._id, { name: this.getName(), @@ -105,9 +119,16 @@ var DocumentThread = Thread.extend({ * shared between tabs, and restored on F5. * * @override + * @param {Object} [options] + * @param {boolean} [options.skipCrossTabSync=false] if set, it should + * not notify other tabs from updating document thread state. This happens + * in case the `detach` operation comes from local storage event. */ - fold: function () { + fold: function (folded, options) { this._super.apply(this, arguments); + if (options && options.skipCrossTabSync) { + return; + } var windowState = this._folded ? 'folded' : 'open'; this.call('mail_service', 'updateDocumentThreadState', this._id, { name: this.getName(), diff --git a/addons/mail/static/src/js/services/mail_document_thread_manager.js b/addons/mail/static/src/js/services/mail_document_thread_manager.js index 000a1c15fa5..1af3fc337a3 100644 --- a/addons/mail/static/src/js/services/mail_document_thread_manager.js +++ b/addons/mail/static/src/js/services/mail_document_thread_manager.js @@ -21,17 +21,23 @@ MailManager.include({ start: function () { this._super.apply(this, arguments); - + this._documentThreadWindowStates = {}; + this._tabId = this.call('bus_service', 'getTabId'); // retrieve the open DocumentThreads from the localStorage - var state = this.call('local_storage', 'getItem', this.DOCUMENT_THREAD_STATE_KEY); - if (!state) { - this.call('local_storage', 'setItem', this.DOCUMENT_THREAD_STATE_KEY, {}); - } else { - this.isReady().then(this._updateDocumentThreadWindows.bind(this, state)); + var localStorageLength = this.call('local_storage', 'length'); + for (var i = 0; i < localStorageLength; i++) { + var key = this.call('local_storage', 'key', i); + if (key.indexOf(this.DOCUMENT_THREAD_STATE_KEY) === 0) { + // key format: DOCUMENT_THREAD_STATE_KEY + '/' + threadId + var documentThreadId = key.substring(this.DOCUMENT_THREAD_STATE_KEY.length+1); + this._documentThreadWindowStates[documentThreadId] = + this.call('local_storage', 'getItem', key); + } } + this.isReady().then(this._updateDocumentThreadWindows.bind(this, this._documentThreadWindowStates)); // listen to localStorage changes to synchronize DocumentThread's // windows between tabs - window.addEventListener('storage', this._onStorage.bind(this)); + this.call('local_storage', 'onStorage', this, this._onStorage); }, //-------------------------------------------------------------------------- @@ -60,7 +66,10 @@ MailManager.include({ options.postedFromDocumentThread ) { var key = this.DOCUMENT_THREAD_MESSAGE_KEY; - this.call('local_storage', 'setItem', key, data); + this.call('local_storage', 'setItem', key, { + messageData: data, + tabId: this._tabId, + }); } return this._super.apply(this, arguments); }, @@ -144,12 +153,11 @@ MailManager.include({ * @param {string} state.windowState ('closed', 'folded' or 'open') */ updateDocumentThreadState: function (threadID, state) { - var states = this.call('local_storage', 'getItem', this.DOCUMENT_THREAD_STATE_KEY); - states = _.omit(states, function (state) { - return state.windowState === 'closed'; + this._documentThreadWindowStates[threadID] = state; + this.call('local_storage', 'setItem', this.DOCUMENT_THREAD_STATE_KEY + '/' + threadID, { + state: state, + tabId: this._tabId, }); - states[threadID] = state; - this.call('local_storage', 'setItem', this.DOCUMENT_THREAD_STATE_KEY, states); }, //-------------------------------------------------------------------------- @@ -194,10 +202,17 @@ MailManager.include({ resModel: info[0], }); if (state.windowState === 'closed') { - documentThread.close(); + documentThread.close({ + skipCrossTabSync: true, + }); } else { - documentThread.fold(state.windowState === 'folded'); - self.openThreadWindow(documentThread.getID(), { keepFoldState: true }); + documentThread.fold(state.windowState === 'folded', { + skipCrossTabSync: true, + }); + self.openThreadWindow(documentThread.getID(), { + keepFoldState: true, + skipCrossTabSync: true, + }); } }); }, @@ -213,13 +228,27 @@ MailManager.include({ * @param {StorageEvent} ev */ _onStorage: function (ev) { - if (ev.key === this.DOCUMENT_THREAD_STATE_KEY) { - var state = this.call('local_storage', 'getItem', this.DOCUMENT_THREAD_STATE_KEY); - this._updateDocumentThreadWindows(state); + var value; + try { + value = JSON.parse(ev.newValue); + } catch (err) { + return; + } + if (ev.key.indexOf(this.DOCUMENT_THREAD_STATE_KEY) === 0) { + if (value.tabId === this._tabId) { + return; + } + // key format: DOCUMENT_THREAD_STATE_KEY + '/' + threadId + var documentThreadId = ev.key.substring(this.DOCUMENT_THREAD_STATE_KEY.length+1); + var param = {}; + param[documentThreadId] = value.state; + this._updateDocumentThreadWindows(param); } else if (ev.key === this.DOCUMENT_THREAD_MESSAGE_KEY) { - var message = this.call('local_storage', 'getItem', this.DOCUMENT_THREAD_MESSAGE_KEY); - if (message) { - this.addMessage(message); + if (value.tabId === this._tabId) { + return; + } + if (value.messageData) { + this.addMessage(value.messageData); } } }, diff --git a/addons/mail/static/src/js/services/mail_window_manager.js b/addons/mail/static/src/js/services/mail_window_manager.js index ebb6a0ff2e7..c5b9f2566c5 100644 --- a/addons/mail/static/src/js/services/mail_window_manager.js +++ b/addons/mail/static/src/js/services/mail_window_manager.js @@ -94,6 +94,8 @@ MailManager.include({ * is not open yet, and do nothing otherwise. * @param {boolean} [options.keepFoldState=false] if set to true, keep the * fold state of the thread + * @param {boolean} [options.skipCrossTabSync=false] if set, thread + * should not notify other tabs from new document thread chat window state. */ openThreadWindow: function (threadID, options) { var self = this; @@ -130,7 +132,9 @@ MailManager.include({ // thread window could not be open, which may happen due to // access error while fetching messages to the document. // abort opening the thread window in this case. - thread.close(); + thread.close({ + skipCrossTabSync: true, + }); }).always(function () { thread.isCreatingWindow = false; }); diff --git a/addons/mail/static/tests/document_thread_window_tests.js b/addons/mail/static/tests/document_thread_window_tests.js index bdab46e91ef..fe2b31b2811 100644 --- a/addons/mail/static/tests/document_thread_window_tests.js +++ b/addons/mail/static/tests/document_thread_window_tests.js @@ -201,7 +201,7 @@ QUnit.test('post messages in a document thread window', function (assert) { testUtils.intercept(messagingMenu, 'call_service', function (ev) { if (ev.data.service === 'local_storage' && ev.data.method === 'setItem' && ev.data.args[0] === 'mail.document_threads_last_message') { - assert.deepEqual(ev.data.args[1], newMessage, + assert.deepEqual(ev.data.args[1].messageData, newMessage, "should write sent message in local storage, to share info with other tabs"); } }, true); @@ -229,7 +229,7 @@ QUnit.test('post messages in a document thread window', function (assert) { }); QUnit.test('open, fold, unfold and close a document thread window', function (assert) { - assert.expect(8); + assert.expect(24); var messagingMenu = new MessagingMenu(); testUtils.addMockEnvironment(messagingMenu, { @@ -239,7 +239,13 @@ QUnit.test('open, fold, unfold and close a document thread window', function (as }); testUtils.intercept(messagingMenu, 'call_service', function (ev) { if (ev.data.service === 'local_storage' && ev.data.method === 'setItem') { - assert.step(ev.data.args); + const key = ev.data.args[0]; + const state = ev.data.args[1].state; + assert.strictEqual(key, 'mail.document_threads_state/some.res.model_1'); + assert.ok(state); + assert.ok(state.name); + assert.ok(state.windowState); + assert.step(`${key}: { name: "${state.name}", windowState: '${state.windowState}' }`); } }, true); messagingMenu.appendTo($('#qunit-fixture')); @@ -262,10 +268,10 @@ QUnit.test('open, fold, unfold and close a document thread window', function (as $('.o_thread_window .o_thread_window_close').click(); assert.verifySteps([ - ['mail.document_threads_state', {"some.res.model_1": {"name": "Some Record", "windowState": "open"}}], - ['mail.document_threads_state', {"some.res.model_1": {"name": "Some Record", "windowState": "folded"}}], - ['mail.document_threads_state', {"some.res.model_1": {"name": "Some Record", "windowState": "open"}}], - ['mail.document_threads_state', {"some.res.model_1": {"name": "Some Record", "windowState": "closed"}}], + `mail.document_threads_state/some.res.model_1: { name: "Some Record", windowState: 'open' }`, + `mail.document_threads_state/some.res.model_1: { name: "Some Record", windowState: 'folded' }`, + `mail.document_threads_state/some.res.model_1: { name: "Some Record", windowState: 'open' }`, + `mail.document_threads_state/some.res.model_1: { name: "Some Record", windowState: 'closed' }`, ]); messagingMenu.destroy();