[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.

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($("<div>")).then(function () {
+        return this.alive(this.searchview.appendTo($("<div>"))).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;
     });
 }