From 708573b0f5dc07a63692bcbadfd0d43883a263de Mon Sep 17 00:00:00 2001 From: abd-msyukyu-odoo Date: Tue, 31 Oct 2023 14:53:24 +0100 Subject: [PATCH] [FIX] web: prevent command palette deadlock Impacted versions: 16.0+ How to reproduce: - create a lot of articles in Knowledge (i.e. 3000+) - click twice on the `Search Articles` fake search bar in the Knowledge Form view Current behavior: - the command palette does not open - the command palette is broken and won't open again (even with the `CTRL+K` shortcut) Expected behavior: - the command palette open once and should not break even if the user clicks multiple times. Technical explanation: - The promise given to the `onWillStart` hook of the `command_palette` can sometimes never be resolved by design (see `KeepLast`). i.e.: - `setCommandPaletteConfig` is called `onWillStart` - `setCommandPaletteConfig` is then called as a result of the `SET-CONFIG` bus event - the first promise will never be resolved since it uses a `KeepLast`, therefore `onWillStart` will never be resolved either. - In `command_service`, the variable `isPaletteOpened` is set to `true` before the command palette dialog is actually mounted. - If `onWillStart` is never resolved, the component will never be mounted, and as such the dialog can never be closed, therefore `isPaletteOpened` stays `true` forever resulting in a deadlock preventing any further opening of the command palette. The issue can be resolved by adding all `KeepLast` promises in a race, which is what `onWillStart` should actually be waiting for, since it will be resolved as soon as any `KeepLast` promise is resolved. task-3554068 closes odoo/odoo#141613 X-original-commit: 6b5fc71e67569b93b699090b225eea147ef463d2 Related: odoo/enterprise#50466 Signed-off-by: Francois Georis (fge) Signed-off-by: Damien Abeloos (abd) --- .../src/core/commands/command_palette.js | 5 +- .../core/commands/command_service_tests.js | 61 +++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/addons/web/static/src/core/commands/command_palette.js b/addons/web/static/src/core/commands/command_palette.js index b8349269cd7..607df17fbdb 100644 --- a/addons/web/static/src/core/commands/command_palette.js +++ b/addons/web/static/src/core/commands/command_palette.js @@ -3,7 +3,7 @@ import { Dialog } from "@web/core/dialog/dialog"; import { useHotkey } from "@web/core/hotkeys/hotkey_hook"; import { _t } from "@web/core/l10n/translation"; -import { KeepLast } from "@web/core/utils/concurrency"; +import { KeepLast, Race } from "@web/core/utils/concurrency"; import { useAutofocus, useService } from "@web/core/utils/hooks"; import { scrollTo } from "@web/core/utils/scrolling"; import { fuzzyLookup } from "@web/core/utils/search"; @@ -107,6 +107,7 @@ export class CommandPalette extends Component { } this.keyId = 1; + this.race = new Race(); this.keepLast = new KeepLast(); this._sessionId = CommandPalette.lastSessionId++; this.DefaultCommandItem = DefaultCommandItem; @@ -181,7 +182,7 @@ export class CommandPalette extends Component { const { namespace, searchValue } = this.processSearchValue(config.searchValue || ""); this.switchNamespace(namespace); - await this.search(searchValue); + await this.race.add(this.search(searchValue)); } /** diff --git a/addons/web/static/tests/core/commands/command_service_tests.js b/addons/web/static/tests/core/commands/command_service_tests.js index 8753e9326f5..d0a82929753 100644 --- a/addons/web/static/tests/core/commands/command_service_tests.js +++ b/addons/web/static/tests/core/commands/command_service_tests.js @@ -15,6 +15,7 @@ import { click, destroy, getFixture, + makeDeferred, mount, nextTick, patchWithCleanup, @@ -1267,3 +1268,63 @@ QUnit.test("uses openPalette to modify the config used by the command palette", ); assert.deepEqual(target.querySelector(".o_command_palette_search input").value, "Command"); }); + +QUnit.test( + "ensure that calling openPalette multiple times successfully loads the last config for the command palette", + async (assert) => { + const providePromise1 = makeDeferred(); + const providePromise2 = makeDeferred(); + const action = () => {}; + + await mount(TestComponent, target, { env }); + + const provide = [ + async () => { + await providePromise1; + return [ + { + name: "Command1", + action, + }, + ]; + }, + async () => { + await providePromise2; + return [ + { + name: "Command2", + action, + }, + ]; + }, + ]; + const configCustom1 = { + searchValue: "Command", + providers: [{ provide: provide[0] }], + }; + const configCustom2 = { + searchValue: "Command", + providers: [{ provide: provide[1] }], + }; + + env.services.command.openPalette(configCustom1); + await nextTick(); + assert.containsNone(target, ".o_command_palette"); + env.services.command.openPalette(configCustom2); + await nextTick(); + assert.containsNone(target, ".o_command_palette"); + providePromise1.resolve(); + await nextTick(); + // First config should not be loaded since a second config was sent. + assert.containsNone(target, ".o_command_palette"); + providePromise2.resolve(); + await nextTick(); + // Second config should be loaded properly. + assert.containsN(target, ".o_command", 1); + assert.deepEqual( + [...target.querySelectorAll(".o_command span:first-child")].map((el) => el.textContent), + ["Command2"] + ); + assert.deepEqual(target.querySelector(".o_command_palette_search input").value, "Command"); + } +);