[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

@@ -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));
+            });
         });
     },
     /**

addons/web/static/tests/views/form_tests.js

@@ -7107,6 +7107,68 @@ QUnit.module('Views', {
         form.destroy();
     });
 
+    QUnit.test('save record with onchange on one2many with required field', function (assert) {
+        // in this test, we have a one2many with a required field, whose value is
+        // set by an onchange on another field ; we manually set the value of that
+        // first field, and directly click on Save (before the onchange RPC returns
+        // and sets the value of the required field)
+        assert.expect(6);
+
+        this.data.partner.fields.foo.default = undefined;
+        this.data.partner.onchanges = {
+            display_name: function (obj) {
+                obj.foo = obj.display_name ? 'foo value' : undefined;
+            },
+        };
+
+        var onchangeDef;
+        var form = createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form>' +
+                    '<field name="p">' +
+                        '<tree editable="top">' +
+                            '<field name="display_name"/>' +
+                            '<field name="foo" required="1"/>' +
+                        '</tree>' +
+                    '</field>' +
+                '</form>',
+            mockRPC: function (route, args) {
+                var result = this._super.apply(this, arguments);
+                if (args.method === 'onchange') {
+                    return $.when(onchangeDef).then(_.constant(result));
+                }
+                if (args.method === 'create') {
+                    assert.step('create');
+                    assert.strictEqual(args.args[0].p[0][2].foo, 'foo value',
+                        "should have wait for the onchange to return before saving");
+                }
+                return result;
+            },
+        });
+
+        form.$('.o_field_x2many_list_row_add a').click();
+
+        assert.strictEqual(form.$('.o_field_widget[name=display_name]').val(), '',
+            "display_name should be the empty string by default");
+        assert.strictEqual(form.$('.o_field_widget[name=foo]').val(), '',
+            "foo should be the empty string by default");
+
+        onchangeDef = $.Deferred(); // delay the onchange
+
+        form.$('.o_field_widget[name=display_name]').val('some value').trigger('input');
+
+        form.$buttons.find('.o_form_button_save').click();
+
+        assert.step('resolve');
+        onchangeDef.resolve();
+
+        assert.verifySteps(['resolve', 'create']);
+
+        form.destroy();
+    });
+
     QUnit.module('FormViewTABMainButtons');
 
     QUnit.test('using tab in an empty required string field should not move to the next field',function(assert) {