From 3b9dda6106f9dd3eabd5dcb14f484a6072937dd6 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 2a1ba16a33e..dea984adb64 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 e049ba968b2..fec53fc8ded 100644
--- a/addons/web/static/tests/helpers/test_utils.js
+++ b/addons/web/static/tests/helpers/test_utils.js
@@ -152,6 +152,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 9e7c89628c4..a9efb9d59f9 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.RainbowMan');
 var testUtils = require('web.test_utils');
@@ -6339,5 +6340,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 33b44948fca..115140bb162 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');
@@ -3224,6 +3225,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;
+    });
+
 });
 
 });