From 75802f9afce0d2bc8825608732d04804b8e7f4e6 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Thu, 15 Mar 2018 11:18:57 +0100 Subject: [PATCH] [FIX] web: ActionManager: concurrent window actions This rev. strengthens the handling of concurrent actions of type 'ir.actions.act_window' in the ActionManager. In particular, the two following cases are now handled: - execute an action A, its 'load_views' RPC takes a while, during that time, execute an action B: A can be dropped, meaning that it doesn't need to fetch its data, and thus it should not be appended to the DOM (nor appear in the breadcrumbs) ; - execute an action A, its 'read' or 'search_read' RPC takes a while, during that time, execute an action B: A can be dropped and should not be appended to the DOM (nor appear in the breadcrumbs). --- .../static/src/js/chrome/action_manager.js | 17 +- .../js/chrome/action_manager_act_window.js | 48 +++-- .../tests/chrome/action_manager_tests.js | 191 ++++++++++++++++++ 3 files changed, 228 insertions(+), 28 deletions(-) diff --git a/addons/web/static/src/js/chrome/action_manager.js b/addons/web/static/src/js/chrome/action_manager.js index 19750915b0b..b8d66bed42d 100644 --- a/addons/web/static/src/js/chrome/action_manager.js +++ b/addons/web/static/src/js/chrome/action_manager.js @@ -61,7 +61,6 @@ var ActionManager = Widget.extend({ // dialog (i.e. coming from an action with target='new') this.currentDialogController = null; }, - /** * @override */ @@ -144,7 +143,7 @@ var ActionManager = Widget.extend({ * loaded and appended to the DOM ; rejected if the action can't be * executed (e.g. if doAction has been called to execute another action * before this one was complete). - */ + */ doAction: function (action, options) { var self = this; options = _.defaults({}, options, { @@ -907,13 +906,13 @@ var ActionManager = Widget.extend({ } }, /** - * Intercepts and triggers a redirection on a link - * - * @private - * @param {OdooEvent} ev - * @param {integer} ev.data.res_id - * @param {string} ev.data.res_model - */ + * Intercepts and triggers a redirection on a link. + * + * @private + * @param {OdooEvent} ev + * @param {integer} ev.data.res_id + * @param {string} ev.data.res_model + */ _onRedirect: function (ev) { this.do_action({ type:'ir.actions.act_window', diff --git a/addons/web/static/src/js/chrome/action_manager_act_window.js b/addons/web/static/src/js/chrome/action_manager_act_window.js index dfb512900e8..4e636de6355 100644 --- a/addons/web/static/src/js/chrome/action_manager_act_window.js +++ b/addons/web/static/src/js/chrome/action_manager_act_window.js @@ -223,6 +223,31 @@ ActionManager.include({ return action.controllers[viewType]; }, + /** + * Destroys the controllers and search view of a given action of type + * 'ir.actions.act_window'. + * + * @private + * @param {Object} action + */ + _destroyWindowAction: function (action) { + var self = this; + _.each(action.controllers, function (controllerDef) { + controllerDef.then(function (controller) { + delete self.controllers[controller.jsID]; + if (controller.widget) { + controller.widget.destroy(); + } + }); + // reject the deferred if it is not yet resolved, so that the + // controller is correctly destroyed as soon as it is ready, and + // its reference is removed + controllerDef.reject(); + }); + if (action.searchView) { + action.searchView.destroy(); + } + }, /** * Executes actions of type 'ir.actions.act_window'. * @@ -245,7 +270,7 @@ ActionManager.include({ } action.flags = this._generateActionFlags(action); - return this._loadViews(action).then(function (fieldsViews) { + return this.dp.add(this._loadViews(action)).then(function (fieldsViews) { var views = self._generateActionViews(action, fieldsViews); action._views = action.views; // save the initial attribute action.views = views; @@ -291,7 +316,7 @@ ActionManager.include({ if (mainView) { defs.push(self._createViewController(action, mainView.type, {}, {lazy: true})); } - return $.when.apply($, defs); + return self.dp.add($.when.apply($, defs)); }).then(function (controller, lazyLoadedController) { action.controllerID = controller.jsID; return self._executeAction(action, options).done(function () { @@ -302,7 +327,7 @@ ActionManager.include({ }, {clear: false}); } }); - }); + }).fail(self._destroyWindowAction.bind(self, action)); }); }, /** @@ -490,25 +515,10 @@ ActionManager.include({ * @private */ _removeAction: function (actionID) { - var self = this; var action = this.actions[actionID]; if (action.type === 'ir.actions.act_window') { delete this.actions[action.jsID]; - _.each(action.controllers, function (controllerDef) { - controllerDef.then(function (controller) { - delete self.controllers[controller.jsID]; - if (controller.widget) { - controller.widget.destroy(); - } - }); - // reject the deferred if it is not yet resolved, so that the - // controller is correctly destroyed as soon as it is ready, and - // its reference is removed - controllerDef.reject(); - }); - if (action.searchView) { - action.searchView.destroy(); - } + this._destroyWindowAction(action); } else { this._super.apply(this, arguments); } diff --git a/addons/web/static/tests/chrome/action_manager_tests.js b/addons/web/static/tests/chrome/action_manager_tests.js index 1769570808f..26c5f0af537 100644 --- a/addons/web/static/tests/chrome/action_manager_tests.js +++ b/addons/web/static/tests/chrome/action_manager_tests.js @@ -276,6 +276,102 @@ QUnit.module('ActionManager', { testUtils.unpatch(Widget); }); + QUnit.test('no memory leaks when executing an action while loading views', function (assert) { + assert.expect(1); + + var def; + var delta = 0; + testUtils.patch(Widget, { + init: function () { + delta += 1; + this._super.apply(this, arguments); + }, + destroy: function () { + delta -= 1; + this._super.apply(this, arguments); + }, + }); + + var actionManager = createActionManager({ + actions: this.actions, + archs: this.archs, + data: this.data, + mockRPC: function (route, args) { + var result = this._super.apply(this, arguments); + if (args.method === 'load_views') { + return $.when(def).then(_.constant(result)); + } + return result; + }, + }); + + // execute action 4 to know the number of widgets it instantiates + actionManager.doAction(4); + var n = delta; + + // execute a first action (its 'load_views' RPC is blocked) + def = $.Deferred(); + actionManager.doAction(3, {clear_breadcrumbs: true}); + + // execute another action meanwhile (and unlock the RPC) + actionManager.doAction(4, {clear_breadcrumbs: true}); + def.resolve(); + + assert.strictEqual(n, delta, + "all widgets of action 3 should have been destroyed"); + + actionManager.destroy(); + testUtils.unpatch(Widget); + }); + + QUnit.test('no memory leaks when executing an action while loading data of default view', function (assert) { + assert.expect(1); + + var def; + var delta = 0; + testUtils.patch(Widget, { + init: function () { + delta += 1; + this._super.apply(this, arguments); + }, + destroy: function () { + delta -= 1; + this._super.apply(this, arguments); + }, + }); + + var actionManager = createActionManager({ + actions: this.actions, + archs: this.archs, + data: this.data, + mockRPC: function (route) { + var result = this._super.apply(this, arguments); + if (route === '/web/dataset/search_read') { + return $.when(def).then(_.constant(result)); + } + return result; + }, + }); + + // execute action 4 to know the number of widgets it instantiates + actionManager.doAction(4); + var n = delta; + + // execute a first action (its 'search_read' RPC is blocked) + def = $.Deferred(); + actionManager.doAction(3, {clear_breadcrumbs: true}); + + // execute another action meanwhile (and unlock the RPC) + actionManager.doAction(4, {clear_breadcrumbs: true}); + def.resolve(); + + assert.strictEqual(n, delta, + "all widgets of action 3 should have been destroyed"); + + actionManager.destroy(); + testUtils.unpatch(Widget); + }); + QUnit.test('action with "no_breadcrumbs" set to true', function (assert) { assert.expect(2); @@ -1256,6 +1352,101 @@ QUnit.module('ActionManager', { actionManager.destroy(); }); + QUnit.test('execute a new action while loading views', function (assert) { + assert.expect(10); + + var def; + var actionManager = createActionManager({ + actions: this.actions, + archs: this.archs, + data: this.data, + mockRPC: function (route, args) { + var result = this._super.apply(this, arguments); + assert.step(args.method || route); + if (args.method === 'load_views') { + return $.when(def).then(_.constant(result)); + } + return result; + }, + }); + + // execute a first action (its 'load_views' RPC is blocked) + def = $.Deferred(); + actionManager.doAction(3); + + assert.strictEqual(actionManager.$('.o_list_view').length, 0, + "should not display the list view of action 3"); + + // execute another action meanwhile (and unlock the RPC) + actionManager.doAction(4); + def.resolve(); + + assert.strictEqual(actionManager.$('.o_kanban_view').length, 1, + "should display the kanban view of action 4"); + assert.strictEqual(actionManager.$('.o_list_view').length, 0, + "should not display the list view of action 3"); + assert.strictEqual($('.o_control_panel .breadcrumb li').length, 1, + "there should be one controller in the breadcrumbs"); + + assert.verifySteps([ + '/web/action/load', // action 3 + 'load_views', // action 3 + '/web/action/load', // action 4 + 'load_views', // action 4 + '/web/dataset/search_read', // search read action 4 + ]); + + actionManager.destroy(); + }); + + QUnit.test('execute a new action while loading data of default view', function (assert) { + assert.expect(11); + + var def; + var actionManager = createActionManager({ + actions: this.actions, + archs: this.archs, + data: this.data, + mockRPC: function (route, args) { + var result = this._super.apply(this, arguments); + assert.step(args.method || route); + if (route === '/web/dataset/search_read') { + return $.when(def).then(_.constant(result)); + } + return result; + }, + }); + + // execute a first action (its 'search_read' RPC is blocked) + def = $.Deferred(); + actionManager.doAction(3); + + assert.strictEqual(actionManager.$('.o_list_view').length, 0, + "should not display the list view of action 3"); + + // execute another action meanwhile (and unlock the RPC) + actionManager.doAction(4); + def.resolve(); + + assert.strictEqual(actionManager.$('.o_kanban_view').length, 1, + "should display the kanban view of action 4"); + assert.strictEqual(actionManager.$('.o_list_view').length, 0, + "should not display the list view of action 3"); + assert.strictEqual($('.o_control_panel .breadcrumb li').length, 1, + "there should be one controller in the breadcrumbs"); + + assert.verifySteps([ + '/web/action/load', // action 3 + 'load_views', // action 3 + '/web/dataset/search_read', // search read action 3 + '/web/action/load', // action 4 + 'load_views', // action 4 + '/web/dataset/search_read', // search read action 4 + ]); + + actionManager.destroy(); + }); + QUnit.module('Client Actions'); QUnit.test('can execute client actions from tag name', function (assert) {