From db9b91ffe684dbc2c7504680daed3238b2c875a4 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Thu, 30 Aug 2018 15:00:02 +0200 Subject: [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. --- addons/web/static/src/js/views/basic/basic_controller.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'addons/web/static/src/js/views/basic/basic_controller.js') 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 -- cgit v1.2.3 From a131373fac22bc1b7c57acd423f526bf73a0d31b Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Mon, 10 Sep 2018 10:03:47 +0200 Subject: [FIX] web: wait for onchanges before saving Rev. db9b91ff removes a use of a mutex where it could end up in a deadlock situation (and where the use of the mutex wasn't necessary). However, it caused an issue as the following scenario didn't work anymore: - in a form view with a one2many with fields A and B (required), with an onchange on A setting B, - add a row and set a value for A, - click on 'Save' directly (before the onchange RPC returns) - whereas it should wait for the onchange to return before trying to save, it tries to save directly but faces an unset required field, and displays a warning to the user. The reason is that, before saving, the FormController asks all its widgets to commit their value (through a call to commitChange). This call was, prior to db9b91ff, done in the mutex. The onchange RPC being itself done in that mutex, we thus waited for the onchange to return before calling commitChanges. After db9b91ff, since there was no longer a mutex, commitChanges was called directly, and the editable list returned a warning as one of its required field was unset. This commit ensures that commitChanges is called once all onchange RPCs are done, by waiting the mutex to be idle. --- addons/web/static/src/js/views/basic/basic_controller.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'addons/web/static/src/js/views/basic/basic_controller.js') 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 767a6511ce3..43d604eac0b 100644 --- a/addons/web/static/src/js/views/basic/basic_controller.js +++ b/addons/web/static/src/js/views/basic/basic_controller.js @@ -173,15 +173,20 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { // may have to validate them before notifying them, so we ask them to // commit their current value before saving. This has to be done outside // of the mutex protection of saving because commitChanges will trigger - // changes and these are also protected. So the actual saving has to be + // changes and these are also protected. However, we must wait for the + // mutex to be idle to ensure that onchange RPCs returned before asking + // field widgets to commit their value (and validate it, for instance + // for one2many with required fields). So the actual saving has to be // done after these changes. Also the commitChanges operation might not // be synchronous for other reason (e.g. the x2m fields will ask the // user if some discarding has to be made). This operation must also be // mutex-protected as commitChanges function of x2m has to be aware of // all final changes made to a row. var self = this; - return this.renderer.commitChanges(recordID || this.handle).then(function () { - return self.mutex.exec(self._saveRecord.bind(self, recordID, options)); + return this.mutex.getUnlockedDef().then(function () { + return self.renderer.commitChanges(recordID || self.handle).then(function () { + return self.mutex.exec(self._saveRecord.bind(self, recordID, options)); + }); }); }, /** -- cgit v1.2.3