odoo
[FW][FIX] web_editor: prevent async code on link tools update
Starting from 1, the _updateColorpicker method was added on the
"link tools" widget to update colorpickers with the selected link colors
(and it will create the colorpicker for each CSS property on widget
start).
Following this update, the code on 2 and 3 added some fixes to
handle the async parts of _updateColorpicker. 3 was done very
recently, more than one year later after the introduction of 1. It
turned a synchronous method into an asynchronous one (rightfully
considering the async calls to _updateColorpicker that were inside).
As a fix, this commit will try to re-made it a synchronous method, and
other parts related to _updateColorpicker too.
The goal of this commit is to separate the link tools colorpicker
"creation" code (_addColorPicker now) from colorpicker's "update"
code. These changes are supposed to fix a race condition issue that
appeared on 16.0, but since the code is the same on 15.0 and subject to
the same issues, we target 15.0 with this fix. Hopefully, this prepares
for an easier refactoring in master.
runbot-5753
Forward-Port-Of: odoo/odoo#109250
This PR targets 16.0 and is part of the forward-port chain. Further PRs will be created up to master.
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
@xO-Tx @qsm-odoo this PR was modified / updated and has become a normal PR. It should be merged the normal way (via @robodoo)
Visibly not true. Did you have a look at it?
Visibly not true. Did you have a look at it?
@qsm-odoo actually, the error makes sense here: it's another "automated testing" race condition that happens when the editor is destroyed while starting the options after a snippet is dropped (following the code from callPostSnippetDrop())...
The strange thing is that only adding an async keyword to the function in .then() here (making it return a promise) will cause the race condition :thinking: But any way I think this can be solved simply by adding the code in callPostSnippetDrop() to the _mutex (see the temporary commit) since we wait for it before destroying the editors... what do you think?
@xO-Tx Make sense but I am afraid this could cause a deadlock: you lock the mutex for the duration of buildSnippet, which call the onBuilt on all options... so any use of the mutex inside of an onBuilt would be the cause of a deadlock. I don't think there are use of the mutex inside one of those but as it's a public abstract method developers are encourage to implement... that would be risky. A simply call to activate_snippet inside an onBuilt and everything would be broken.
But I also know that the drag and drop + mutex interaction is not as robust as it could be (if you try to save very quickly after a drop, which is not a normal usecase). I am hoping we can just ignore that until we reach a refactoring with OWL in the future.
Is not there another way? Maybe explicit "if not destroyed" at some places?
Is not there another way? Maybe explicit "if not destroyed" at some places?
Actually, the error occurs exactly on the Many2manyUserValueWidget where we try to access an already destroyed we-list widget on start... I think we can add a check on isDestroyed(), but It doesn't seem a "good enough" solution to me, is there something that prevents using a promise for the "post-drop updates" and await it on cleanForSave() ?
(I updated the temporary commit with an example of the code).
This one if silently blocked: the runbot is red.
@rdeodoo linked to runbot-17505