From db9b91ffe684dbc2c7504680daed3238b2c875a4 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Thu, 30 Aug 2018 15:00:02 +0200 Subject: [PATCH] [FIX] web: BasicController: remove deadlock situation This rev. removes a deadlock situation that could *theoretically* occur, but that was actually hard to reach in practice. Before saving a record (e.g. in a form view), all field widgets are asked to commit their value (in case they wouldn't have notified the model yet, e.g. because the user just changed them). For instance, let's assume that the user fills an input field, and then triggers the save. In practice, by triggering the save, it will force the widget to notify the model of its new value (because it looses the focus). This is also the case using keyboard navigation, as it actually simulates clicks. So when the widget is asked to commit its value right after, it has nothing more to commit. This is fortunate, because if it actually had something to commit, we would end up in a deadlock ('commitChanges' is executed in a mutex, and if the widget triggers a new value, '_applyChange' is called, and is executed in that mutex as well). In fact, executing 'commitChanges' in a mutex is useless, as '_applyChange', which is called when a widget has something to commit, is already executed in the mutex. So this rev. removes the use of the mutex in 'commitChanges'. Doing so broke several Many2One tests, because they wrongly relied on the presence of the mutex for scenarios where the user quick creates a relational record, and then directly saves (before the relational record is actually created). We thus handled that usecase properly to make the tests pass. Task 1878254. --- .../static/src/js/fields/relational_fields.js | 67 +++++++++++++------ .../src/js/views/basic/basic_controller.js | 8 +-- addons/web/static/tests/views/form_tests.js | 41 ++++++++++++ 3 files changed, 92 insertions(+), 24 deletions(-) diff --git a/addons/web/static/src/js/fields/relational_fields.js b/addons/web/static/src/js/fields/relational_fields.js index 9f958ec57e0..ee511a390b0 100644 --- a/addons/web/static/src/js/fields/relational_fields.js +++ b/addons/web/static/src/js/fields/relational_fields.js @@ -137,6 +137,12 @@ var FieldMany2One = AbstractField.extend({ // coming by an onchange on another field) this.isDirty = false; this.lastChangeEvent = undefined; + + // use a DropPrevious to properly handle related record quick creations, + // and store a createDef to be able to notify the environment that there + // is pending quick create operation + this.dp = new concurrency.DropPrevious(); + this.createDef = undefined; }, start: function () { // booleean indicating that the content of the input isn't synchronized @@ -153,6 +159,18 @@ var FieldMany2One = AbstractField.extend({ // Public //-------------------------------------------------------------------------- + /** + * Override to make the caller wait for potential ongoing record creation. + * This ensures that the correct many2one value is set when the main record + * is saved. + * + * @override + * @returns {Deferred} resolved as soon as there is no longer record being + * (quick) created + */ + commitChanges: function () { + return $.when(this.createDef); + }, /** * @override * @returns {jQuery} @@ -166,7 +184,7 @@ var FieldMany2One = AbstractField.extend({ reinitialize: function (value) { this.isDirty = false; this.floating = false; - this._setValue(value); + return this._setValue(value); }, /** * Re-renders the widget if it isn't dirty. The widget is dirty if the user @@ -291,29 +309,40 @@ var FieldMany2One = AbstractField.extend({ _quickCreate: function (name) { var self = this; var def = $.Deferred(); + this.createDef = this.createDef || $.Deferred(); + // called when the record has been quick created, or when the dialog has + // been closed (in the case of a 'slow' create), meaning that the job is + // done + var createDone = function () { + def.resolve(); + self.createDef.resolve(); + self.createDef = undefined; + }; + // called if the quick create is disabled on this many2one, or if the + // quick creation failed (probably because there are mandatory fields on + // the model) var slowCreate = function () { var dialog = self._searchCreatePopup("form", false, self._createContext(name)); - dialog.on('closed', self, def.resolve.bind(def)); + dialog.on('closed', self, createDone); }; if (this.nodeOptions.quick_create) { - this.trigger_up('mutexify', { - action: function () { - return self._rpc({ - model: self.field.relation, - method: 'name_create', - args: [name], - context: self.record.getContext(self.recordParams), - }).then(function (result) { - if (self.mode === "edit") { - self.reinitialize({id: result[0], display_name: result[1]}); - } - def.resolve(); - }).fail(function (error, event) { - event.preventDefault(); - slowCreate(); - }); - }, + var nameCreateDef = this._rpc({ + model: this.field.relation, + method: 'name_create', + args: [name], + context: this.record.getContext(this.recordParams), + }).fail(function (error, ev) { + ev.preventDefault(); + slowCreate(); }); + this.dp.add(nameCreateDef) + .then(function (result) { + if (self.mode === "edit") { + self.reinitialize({id: result[0], display_name: result[1]}); + } + createDone(); + }) + .fail(def.reject.bind(def)); } else { slowCreate(); } diff --git a/addons/web/static/src/js/views/basic/basic_controller.js b/addons/web/static/src/js/views/basic/basic_controller.js index 5767969d15c..767a6511ce3 100644 --- a/addons/web/static/src/js/views/basic/basic_controller.js +++ b/addons/web/static/src/js/views/basic/basic_controller.js @@ -180,11 +180,9 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { // mutex-protected as commitChanges function of x2m has to be aware of // all final changes made to a row. var self = this; - return this.mutex - .exec(this.renderer.commitChanges.bind(this.renderer, recordID || this.handle)) - .then(function () { - return self.mutex.exec(self._saveRecord.bind(self, recordID, options)); - }); + return this.renderer.commitChanges(recordID || this.handle).then(function () { + return self.mutex.exec(self._saveRecord.bind(self, recordID, options)); + }); }, /** * @override diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js index f985062966c..1d89375c547 100644 --- a/addons/web/static/tests/views/form_tests.js +++ b/addons/web/static/tests/views/form_tests.js @@ -7066,6 +7066,47 @@ QUnit.module('Views', { def1.resolve(); }); + QUnit.test('no deadlock when saving with uncommitted changes', function (assert) { + // Before saving a record, all field widgets are asked to commit their changes (new values + // that they wouldn't have sent to the model yet). This test is added alongside a bug fix + // ensuring that we don't end up in a deadlock when a widget actually has some changes to + // commit at that moment. By chance, this situation isn't reached when the user clicks on + // 'Save' (which is the natural way to save a record), because by clicking outside the + // widget, the 'change' event (this is mainly for InputFields) is triggered, and the widget + // notifies the model of its new value on its own initiative, before being requested to. + // In this test, we try to reproduce the deadlock situation by forcing the field widget to + // commit changes before the save. We thus manually call 'saveRecord', instead of clicking + // on 'Save'. + assert.expect(6); + + var form = createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: '
', + mockRPC: function (route, args) { + assert.step(args.method); + return this._super.apply(this, arguments); + }, + // we set a fieldDebounce to precisely mock the behavior of the webclient: changes are + // not sent to the model at keystrokes, but when the input is left + fieldDebounce: 5000, + }); + + form.$('input').val('some foo value').trigger('input'); + // manually save the record, to prevent the field widget to notify the model of its new + // value before being requested to + form.saveRecord(); + + assert.strictEqual(form.$('.o_form_readonly').length, 1, + "form view should be in readonly"); + assert.strictEqual(form.$el.text().trim(), 'some foo value', + "foo field should have correct value"); + assert.verifySteps(['default_get', 'create', 'read']); + + form.destroy(); + }); + QUnit.module('FormViewTABMainButtons'); QUnit.test('using tab in an empty required string field should not move to the next field',function(assert) {