Skip to content

Commit

Permalink
Fix a race condition involving the waitForEvent integration test he…
Browse files Browse the repository at this point in the history
…lper 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.
  • Loading branch information
timvandermeij committed Jun 25, 2024
1 parent d13c1ff commit 488e47b
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 15 deletions.
5 changes: 3 additions & 2 deletions test/integration/copy_paste_spec.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
35 changes: 22 additions & 13 deletions test/integration/test_utils.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down

0 comments on commit 488e47b

Please sign in to comment.