From 827adcd8fc92139b2f25a1dfd714374cb2cc3cb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 13 Apr 2018 13:01:09 +0200 Subject: [PATCH] [FIX] mail: prevent race condition in discuss The discuss application starting process is strongly asynchronous. However, it relies implicitely on the fact that the widget is not destroyed in its starting process. For example, if the widget is detroyed before the updateChannel method is called, then any call to the chat manager service will return undefined and the widget will crash. When discuss is destroyed exactly between the start and the end of the creation of its search view (which is asynchronous), then it is possible to have a crash, because the search view will be destroyed when the do_search method is called. Note that we had to fix discuss mobile as well: The _setChannel method is supposed to return a deferred, but the mobile override did not return it. As a result, a test in the mobile suite failed because this.alive takes a deferred in argument. --- addons/mail/static/src/js/discuss.js | 12 +-- addons/mail/static/src/js/discuss_mobile.js | 4 +- addons/mail/static/tests/discuss_tests.js | 82 +++++++++++++++++++ .../mail/static/tests/helpers/test_utils.js | 3 +- 4 files changed, 92 insertions(+), 9 deletions(-) diff --git a/addons/mail/static/src/js/discuss.js b/addons/mail/static/src/js/discuss.js index 7f297f76298..3ac9e3b0ed5 100644 --- a/addons/mail/static/src/js/discuss.js +++ b/addons/mail/static/src/js/discuss.js @@ -170,18 +170,20 @@ var Discuss = AbstractAction.extend(ControlPanelMixin, { this.basicComposer.on('input_focused', this, this._onComposerFocused); this.extendedComposer.on('post_message', this, this._onPostMessage); this.extendedComposer.on('input_focused', this, this._onComposerFocused); + this._renderButtons(); var defs = []; - defs.push(this._renderButtons()); defs.push(this._renderThread()); defs.push(this.basicComposer.appendTo(this.$('.o_mail_chat_content'))); defs.push(this.extendedComposer.appendTo(this.$('.o_mail_chat_content'))); defs.push(this._renderSearchView()); - return $.when.apply($, defs) - .then(this._setChannel.bind(this, defaultChannel)) - .then(this._updateChannels.bind(this)) + return this.alive($.when.apply($, defs)) .then(function () { + return self.alive(self._setChannel(defaultChannel)); + }) + .then(function () { + self._updateChannels(); self._startListening(); self.thread.$el.on("scroll", null, _.debounce(function () { if (self.thread.get_scrolltop() < 20 && @@ -413,7 +415,7 @@ var Discuss = AbstractAction.extend(ControlPanelMixin, { disable_groupby: true, }; this.searchview = new SearchView(this, this.dataset, this.fields_view, options); - return this.searchview.appendTo($("
")).then(function () { + return this.alive(this.searchview.appendTo($("
"))).then(function () { self.$searchview_buttons = self.searchview.$buttons.contents(); // manually call do_search to generate the initial domain and filter // the messages in the default channel diff --git a/addons/mail/static/src/js/discuss_mobile.js b/addons/mail/static/src/js/discuss_mobile.js index b427ab4b98c..556651ec67e 100644 --- a/addons/mail/static/src/js/discuss_mobile.js +++ b/addons/mail/static/src/js/discuss_mobile.js @@ -105,9 +105,9 @@ Discuss.include({ */ _setChannel: function (channel) { if (channel.type !== 'static') { - this.call('chat_manager', 'detachChannel', channel.id); + return this.call('chat_manager', 'detachChannel', channel.id); } else { - this._super.apply(this, arguments); + return this._super.apply(this, arguments); } }, /** diff --git a/addons/mail/static/tests/discuss_tests.js b/addons/mail/static/tests/discuss_tests.js index 0d6ae016e51..8d3ac86824e 100644 --- a/addons/mail/static/tests/discuss_tests.js +++ b/addons/mail/static/tests/discuss_tests.js @@ -1,11 +1,13 @@ odoo.define('mail.discuss_test', function (require) { "use strict"; +var Discuss = require('mail.chat_discuss'); var ChatManager = require('mail.ChatManager'); var mailTestUtils = require('mail.testUtils'); var Bus = require('web.Bus'); var concurrency = require('web.concurrency'); +var SearchView = require('web.SearchView'); var testUtils = require('web.test_utils'); var createBusService = mailTestUtils.createBusService; @@ -457,6 +459,86 @@ QUnit.test('"Unstar all" button should reset the starred counter', function (ass }); }); +QUnit.test('do not crash when destroyed before start is completed', function (assert) { + assert.expect(3); + var discuss; + + testUtils.patch(Discuss, { + init: function () { + discuss = this; + this._super.apply(this, arguments); + }, + }); + + createDiscuss({ + id: 1, + context: {}, + params: {}, + data: this.data, + services: this.services, + mockRPC: function (route, args) { + if (args.method) { + assert.step(args.method); + } + var result = this._super.apply(this, arguments); + if (args.method === 'message_fetch') { + discuss.destroy(); + } + return result; + }, + }); + + assert.verifySteps([ + "load_views", + "message_fetch" + ]); + + testUtils.unpatch(Discuss); +}); + +QUnit.test('do not crash when destroyed between start en end of _renderSearchView', function (assert) { + assert.expect(2); + var discuss; + + testUtils.patch(Discuss, { + init: function () { + discuss = this; + this._super.apply(this, arguments); + }, + }); + + var def = $.Deferred(); + + testUtils.patch(SearchView, { + willStart: function () { + var result = this._super.apply(this, arguments); + return def.then($.when(result)); + }, + }); + + createDiscuss({ + id: 1, + context: {}, + params: {}, + data: this.data, + services: this.services, + mockRPC: function (route, args) { + if (args.method) { + assert.step(args.method); + } + return this._super.apply(this, arguments); + }, + }); + + discuss.destroy(); + def.resolve(); + assert.verifySteps([ + "load_views", + ]); + + testUtils.unpatch(Discuss); + testUtils.unpatch(SearchView); +}); }); }); diff --git a/addons/mail/static/tests/helpers/test_utils.js b/addons/mail/static/tests/helpers/test_utils.js index f7292c7a67e..3382fa90dd0 100644 --- a/addons/mail/static/tests/helpers/test_utils.js +++ b/addons/mail/static/tests/helpers/test_utils.js @@ -78,7 +78,6 @@ function createDiscuss(params) { var selector = params.debug ? 'body' : '#qunit-fixture'; var controlPanel = new ControlPanel(parent); controlPanel.appendTo($(selector)); - discuss.appendTo($(selector)); // override 'destroy' of discuss so that it calls 'destroy' on the parent // instead, which is the parent of discuss and the mockServer. @@ -92,7 +91,7 @@ function createDiscuss(params) { // link the view to the control panel discuss.set_cp_bus(controlPanel.get_bus()); - return discuss.call('chat_manager', 'isReady').then(function () { + return discuss.appendTo($(selector)).then(function () { return discuss; }); }