From b28ceec009e0e7fd7efa82177540ff7489112f5e Mon Sep 17 00:00:00 2001
From: Christophe Matthieu <chm@odoo.com>
Date: Mon, 5 Feb 2018 16:55:15 +0100
Subject: [PATCH] [FIX] web: Views: always destroy BasicModel instances

Before this rev., it might happen that the parent of BasicModel instances
wasn't correctly set to the Controller instance. It was actually set to the
ActionManager instance, which is never destroyed. So when this happened,
the BasicModel instance wasn't destroyed when it's controller was.
---
 .../web/static/src/js/views/abstract_view.js  | 12 +--
 addons/web/static/tests/helpers/test_utils.js |  1 +
 .../web/static/tests/views/calendar_tests.js  | 59 ++++++++++++++
 addons/web/static/tests/views/form_tests.js   | 80 +++++++++++++++++++
 addons/web/static/tests/views/kanban_tests.js | 58 ++++++++++++++
 addons/web/static/tests/views/list_tests.js   | 57 +++++++++++++
 6 files changed, 262 insertions(+), 5 deletions(-)

diff --git a/addons/web/static/src/js/views/abstract_view.js b/addons/web/static/src/js/views/abstract_view.js
index 6cc8a3c8143..bb945d6fbb0 100644
--- a/addons/web/static/src/js/views/abstract_view.js
+++ b/addons/web/static/src/js/views/abstract_view.js
@@ -138,21 +138,23 @@ var AbstractView = Class.extend({
      */
     getController: function (parent) {
         var self = this;
+        // check if a model already exists, as if not, one will be created and
+        // we'll have to set the controller as its parent
+        var alreadyHasModel = !!this.model;
         return $.when(this._loadData(parent), ajax.loadLibs(this)).then(function () {
-            var model = self.getModel();
-            var state = model.get(arguments[0]);
+            var state = self.model.get(arguments[0]);
             var renderer = self.getRenderer(parent, state);
             var Controller = self.Controller || self.config.Controller;
             var controllerParams = _.extend({
                 initialState: state,
             }, self.controllerParams);
-            var controller = new Controller(parent, model, renderer, controllerParams);
+            var controller = new Controller(parent, self.model, renderer, controllerParams);
             renderer.setParent(controller);
 
-            if (!self.model) {
+            if (!alreadyHasModel) {
                 // if we have a model, it already has a parent. Otherwise, we
                 // set the controller, so the rpcs from the model actually work
-                model.setParent(controller);
+                self.model.setParent(controller);
             }
             return controller;
         });
diff --git a/addons/web/static/tests/helpers/test_utils.js b/addons/web/static/tests/helpers/test_utils.js
index 2ff472335ef..15478d6dff6 100644
--- a/addons/web/static/tests/helpers/test_utils.js
+++ b/addons/web/static/tests/helpers/test_utils.js
@@ -174,6 +174,7 @@ function createAsyncView(params) {
     return view.getController(widget).then(function (view) {
         // override the view's 'destroy' so that it calls 'destroy' on the widget
         // instead, as the widget is the parent of the view and the mockServer.
+        view.__destroy = view.destroy;
         view.destroy = function () {
             // remove the override to properly destroy the view and its children
             // when it will be called the second time (by its parent)
diff --git a/addons/web/static/tests/views/calendar_tests.js b/addons/web/static/tests/views/calendar_tests.js
index b6cbcd7e03f..df8495b1a18 100644
--- a/addons/web/static/tests/views/calendar_tests.js
+++ b/addons/web/static/tests/views/calendar_tests.js
@@ -4,6 +4,7 @@ odoo.define('web.calendar_tests', function (require) {
 var CalendarView = require('web.CalendarView');
 var CalendarRenderer = require('web.CalendarRenderer');
 var fieldUtils = require('web.field_utils');
+var mixins = require('web.mixins');
 var testUtils = require('web.test_utils');
 var session = require('web.session');
 
@@ -1650,6 +1651,64 @@ QUnit.module('Views', {
 
         calendar.destroy();
     });
+
+    QUnit.test('check if the view destroys all widgets and instances', function (assert) {
+        assert.expect(1);
+
+        var instanceNumber = 0;
+        var initWidget = mixins.ParentedMixin.init;
+        mixins.ParentedMixin.init = function () {
+            instanceNumber++;
+            return initWidget.apply(this, arguments);
+        };
+        var destroyWidget = mixins.ParentedMixin.destroy;
+        mixins.ParentedMixin.destroy = function () {
+            if (!this.isDestroyed()) {
+                instanceNumber--;
+            }
+            return destroyWidget.apply(this, arguments);
+        };
+
+        var params = {
+            View: CalendarView,
+            model: 'event',
+            data: this.data,
+            arch:
+            '<calendar class="o_calendar_test" '+
+                'event_open_popup="true" '+
+                'date_start="start_date" '+
+                'all_day="allday" '+
+                'mode="week" '+
+                'attendee="partner_ids" '+
+                'color="partner_id">'+
+                    '<field name="name"/>'+
+                    '<filter name="user_id" avatar_field="image"/>'+
+                    '<field name="partner_ids" write_model="filter_partner" write_field="partner_id"/>'+
+            '</calendar>',
+            archs: archs,
+            viewOptions: {
+                initialDate: initialDate,
+            },
+        };
+
+        var calendar = createView(params);
+        calendar.destroy();
+
+        var initialInstanceNumber = instanceNumber;
+        instanceNumber = 0;
+
+        calendar = createView(params);
+
+        // call destroy function of controller to ensure that it correctly destroys everything
+        calendar.__destroy();
+
+        assert.strictEqual(instanceNumber, initialInstanceNumber+1, "every widget must be destroyed exept the parent");
+
+        calendar.destroy();
+
+        mixins.ParentedMixin.init = initWidget;
+        mixins.ParentedMixin.destroy = destroyWidget;
+    });
 });
 
 });
diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js
index 672c7a7151d..9e440deda85 100644
--- a/addons/web/static/tests/views/form_tests.js
+++ b/addons/web/static/tests/views/form_tests.js
@@ -6,6 +6,7 @@ var config = require('web.config');
 var core = require('web.core');
 var fieldRegistry = require('web.field_registry');
 var FormView = require('web.FormView');
+var mixins = require('web.mixins');
 var pyeval = require('web.pyeval');
 var RainbowMan = require('web.rainbow_man');
 var testUtils = require('web.test_utils');
@@ -6337,5 +6338,84 @@ QUnit.module('Views', {
         form.destroy();
     });
 
+    QUnit.test('check if the view destroys all widgets and instances', function (assert) {
+        assert.expect(1);
+
+        var instanceNumber = 0;
+        var initWidget = mixins.ParentedMixin.init;
+        mixins.ParentedMixin.init = function () {
+            instanceNumber++;
+            return initWidget.apply(this, arguments);
+        };
+        var destroyWidget = mixins.ParentedMixin.destroy;
+        mixins.ParentedMixin.destroy = function () {
+            if (!this.isDestroyed()) {
+                instanceNumber--;
+            }
+            return destroyWidget.apply(this, arguments);
+        };
+
+        var params = {
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form string="Partners">' +
+                    '<sheet>' +
+                        '<field name="display_name"/>' +
+                        '<field name="foo"/>' +
+                        '<field name="bar"/>' +
+                        '<field name="int_field"/>' +
+                        '<field name="qux"/>' +
+                        '<field name="trululu"/>' +
+                        '<field name="timmy"/>' +
+                        '<field name="product_id"/>' +
+                        '<field name="priority"/>' +
+                        '<field name="state"/>' +
+                        '<field name="date"/>' +
+                        '<field name="datetime"/>' +
+                        '<field name="product_ids"/>' +
+                        '<field name="p">' +
+                            '<tree default_order="foo desc">' +
+                                '<field name="display_name"/>' +
+                                '<field name="foo"/>' +
+                            '</tree>' +
+                        '</field>' +
+                    '</sheet>' +
+                '</form>',
+            archs: {
+                'partner,false,form':
+                    '<form string="Partner">' +
+                        '<sheet>' +
+                            '<group>' +
+                                '<field name="foo"/>' +
+                            '</group>' +
+                        '</sheet>' +
+                    '</form>',
+                "partner_type,false,list": '<tree><field name="name"/></tree>',
+                'product,false,list': '<tree><field name="display_name"/></tree>',
+
+            },
+            res_id: 1,
+        };
+
+        var form = createView(params);
+        form.destroy();
+
+        var initialInstanceNumber = instanceNumber;
+        instanceNumber = 0;
+
+        form = createView(params);
+
+        // call destroy function of controller to ensure that it correctly destroys everything
+        form.__destroy();
+
+        assert.strictEqual(instanceNumber, initialInstanceNumber+1, "every widget must be destroyed exept the parent");
+
+        form.destroy();
+
+        mixins.ParentedMixin.init = initWidget;
+        mixins.ParentedMixin.destroy = destroyWidget;
+    });
+
 });
 });
diff --git a/addons/web/static/tests/views/kanban_tests.js b/addons/web/static/tests/views/kanban_tests.js
index 250bb8ef64f..ed697c32712 100644
--- a/addons/web/static/tests/views/kanban_tests.js
+++ b/addons/web/static/tests/views/kanban_tests.js
@@ -5,6 +5,7 @@ var core = require('web.core');
 var KanbanColumnProgressBar = require('web.KanbanColumnProgressBar');
 var KanbanRenderer = require('web.KanbanRenderer');
 var KanbanView = require('web.KanbanView');
+var mixins = require('web.mixins');
 var testUtils = require('web.test_utils');
 var Widget = require('web.Widget');
 var widgetRegistry = require('web.widget_registry');
@@ -2484,6 +2485,63 @@ QUnit.module('Views', {
 
         kanban.destroy();
     });
+
+    QUnit.test('check if the view destroys all widgets and instances', function (assert) {
+        assert.expect(1);
+
+        var instanceNumber = 0;
+        var initWidget = mixins.ParentedMixin.init;
+        mixins.ParentedMixin.init = function () {
+            instanceNumber++;
+            return initWidget.apply(this, arguments);
+        };
+        var destroyWidget = mixins.ParentedMixin.destroy;
+        mixins.ParentedMixin.destroy = function () {
+            if (!this.isDestroyed()) {
+                instanceNumber--;
+            }
+            return destroyWidget.apply(this, arguments);
+        };
+
+        var params = {
+            View: KanbanView,
+            model: 'partner',
+            data: this.data,
+            arch: '<kanban string="Partners">' +
+                    '<field name="foo"/>' +
+                    '<field name="bar"/>' +
+                    '<field name="int_field"/>' +
+                    '<field name="qux"/>' +
+                    '<field name="product_id"/>' +
+                    '<field name="category_ids"/>' +
+                    '<field name="state"/>' +
+                    '<field name="date"/>' +
+                    '<field name="datetime"/>' +
+                    '<templates><t t-name="kanban-box">' +
+                        '<div><field name="foo"/></div>' +
+                    '</t></templates>' +
+                '</kanban>',
+        };
+
+        var kanban = createView(params);
+        kanban.destroy();
+
+        var initialInstanceNumber = instanceNumber;
+        instanceNumber = 0;
+
+        kanban = createView(params);
+
+        // call destroy function of controller to ensure that it correctly destroys everything
+        kanban.__destroy();
+
+        assert.strictEqual(instanceNumber, initialInstanceNumber+1, "every widget must be destroyed exept the parent");
+
+        kanban.destroy();
+
+        mixins.ParentedMixin.init = initWidget;
+        mixins.ParentedMixin.destroy = destroyWidget;
+    });
+
 });
 
 });
diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js
index b03e3ff534d..6657c711b95 100644
--- a/addons/web/static/tests/views/list_tests.js
+++ b/addons/web/static/tests/views/list_tests.js
@@ -5,6 +5,7 @@ var config = require('web.config');
 var basicFields = require('web.basic_fields');
 var FormView = require('web.FormView');
 var ListView = require('web.ListView');
+var mixins = require('web.mixins');
 var testUtils = require('web.test_utils');
 var widgetRegistry = require('web.widget_registry');
 var Widget = require('web.Widget');
@@ -3246,6 +3247,62 @@ QUnit.module('Views', {
         list.destroy();
     });
 
+    QUnit.test('check if the view destroys all widgets and instances', function (assert) {
+        assert.expect(1);
+
+        var instanceNumber = 0;
+        var initWidget = mixins.ParentedMixin.init;
+        mixins.ParentedMixin.init = function () {
+            instanceNumber++;
+            return initWidget.apply(this, arguments);
+        };
+        var destroyWidget = mixins.ParentedMixin.destroy;
+        mixins.ParentedMixin.destroy = function () {
+            if (!this.isDestroyed()) {
+                instanceNumber--;
+            }
+            return destroyWidget.apply(this, arguments);
+        };
+
+        var params = {
+            View: ListView,
+            model: 'foo',
+            data: this.data,
+            arch: '<tree string="Partners">' +
+                    '<field name="foo"/>' +
+                    '<field name="bar"/>' +
+                    '<field name="date"/>' +
+                    '<field name="int_field"/>' +
+                    '<field name="qux"/>' +
+                    '<field name="m2o"/>' +
+                    '<field name="o2m"/>' +
+                    '<field name="m2m"/>' +
+                    '<field name="amount"/>' +
+                    '<field name="currency_id"/>' +
+                    '<field name="datetime"/>' +
+                    '<field name="reference"/>' +
+                '</tree>',
+        };
+
+        var list = createView(params);
+        list.destroy();
+
+        var initialInstanceNumber = instanceNumber;
+        instanceNumber = 0;
+
+        list = createView(params);
+
+        // call destroy function of controller to ensure that it correctly destroys everything
+        list.__destroy();
+
+        assert.strictEqual(instanceNumber, initialInstanceNumber+1, "every widget must be destroyed exept the parent");
+
+        list.destroy();
+
+        mixins.ParentedMixin.init = initWidget;
+        mixins.ParentedMixin.destroy = destroyWidget;
+    });
+
 });
 
 });