From ae4e8d54499fbbd8407f0b13772e56a9f7030271 Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Sun, 23 Jun 2024 16:10:27 +0200 Subject: [PATCH] Fix a race condition involving the `waitForEvent` integration test helper function Debugging #17931 uncovered a race condition in the way we use the `waitForEvent` function. Currently the following happens: 1. We call `waitForEvent`, which starts execution of the function body and immediately returns a promise. 2. We do actions to trigger the event. 3. We await the promise, which resolves if the event is triggered or the timeout is reached. The problem is in step 1: function body execution has started, but not necessarily completed. Given that we don't await the promise, we immediately trigger step 2 and it's not unlikely that the event we trigger arrives before the event listener is actually registered in the function body of `waitForEvent` (which is slower because it needs to be evaluated in the page context and there is some other logic before the actual `addEventListener` call). This commit fixes the issue by turning `waitForEvent` into a generator, which allows us to only give control back to the caller when the event listener is fully registered. Now the following happens: 1. We call `waitForEvent`, which starts execution of the function body and immediately returns a promise. 2. We call `next()` to advance to the `yield` in `waitForEvent`, which effectively waits until the event listener is registered and it's safe to return control to the caller. 3. We do actions to trigger the event. 4. We call `generator.next()` to finish the logic after the `yield` in `waitForEvent` so that the generator is exhausted and we wait for the event or a timeout. This should make sure that we always register the event listener before triggering the event, which in itself fixes intermittent issues, but because we shouldn't miss events anymore we can also remove the retry logic for e.g. pasting, which fixes the issues we have seen in #17931 where pasting could happen multiple times. --- test/integration/copy_paste_spec.mjs | 5 ++-- test/integration/test_utils.mjs | 35 +++++++++++++++++----------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/test/integration/copy_paste_spec.mjs b/test/integration/copy_paste_spec.mjs index 94d6be9a76fd75..30b32978860255 100644 --- a/test/integration/copy_paste_spec.mjs +++ b/test/integration/copy_paste_spec.mjs @@ -23,9 +23,10 @@ import { } from "./test_utils.mjs"; const selectAll = async page => { - const promise = waitForEvent(page, "selectionchange"); + const generator = waitForEvent(page, "selectionchange"); + await generator.next(); // Wait until the event listener is registered. await kbSelectAll(page); - await promise; + await generator.next(); // Wait for the event or a timeout. await page.waitForFunction(() => { const selection = document.getSelection(); diff --git a/test/integration/test_utils.mjs b/test/integration/test_utils.mjs index 4fd4e9e35db844..fc365d387a27b8 100644 --- a/test/integration/test_utils.mjs +++ b/test/integration/test_utils.mjs @@ -186,13 +186,14 @@ async function getSpanRectFromText(page, pageNumber, text) { ); } -async function waitForEvent( +async function* waitForEvent( page, eventName, selector = null, validator = null, timeout = 5000 ) { + // Register the event listener in the page context. const handle = await page.evaluateHandle( (name, sel, validate, timeOut) => { let callback = null, @@ -227,13 +228,22 @@ async function waitForEvent( validator ? validator.toString() : null, timeout ); + + // Give back control to the caller to trigger the event. Note that the event + // listener registration above *must* be fully completed (and is guaranteed + // to be at this point) before it's safe to hand back control to the caller, + // otherwise the caller could trigger the event before the event listener + // is in place and therefore miss the event here and get in the timeout. + yield; + + // The caller indicated that it's done triggering the event by calling + // `next()`, so we can now safely wait for the result of `Promise.race` above. const success = await awaitPromise(handle); if (success === null) { console.log(`waitForEvent: ${eventName} didn't trigger within the timeout`); } else if (!success) { console.log(`waitForEvent: ${eventName} triggered, but validation failed`); } - return success; } async function waitForStorageEntries(page, nEntries) { @@ -293,9 +303,10 @@ async function mockClipboard(pages) { } async function copy(page) { - const promise = waitForEvent(page, "copy"); + const generator = waitForEvent(page, "copy"); + await generator.next(); // Wait until the event listener is registered. await kbCopy(page); - await promise; + await generator.next(); // Wait for the event or a timeout. } async function copyData(page, data) { @@ -314,20 +325,18 @@ async function copyData(page, data) { } async function paste(page) { - const promise = waitForEvent(page, "paste"); + const generator = waitForEvent(page, "paste"); + await generator.next(); // Wait until the event listener is registered. await kbPaste(page); - await promise; + await generator.next(); // Wait for the event or a timeout. } async function pasteData(page, selector = null) { const validator = e => e.clipboardData.items.length !== 0; - let hasPasteEvent = false; - while (!hasPasteEvent) { - // We retry to paste if nothing has been pasted before the timeout. - const promise = waitForEvent(page, "paste", selector, validator); - await kbPaste(page); - hasPasteEvent = await promise; - } + const generator = waitForEvent(page, "paste", selector, validator); + await generator.next(); // Wait until the event listener is registered. + await kbPaste(page); + await generator.next(); // Wait for the event or a timeout. } async function getSerialized(page, filter = undefined) {