Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not wait for events to be dispatched to IDBRequest #47517

Conversation

szewai
Copy link
Contributor

@szewai szewai commented Aug 7, 2024

The test expects event to be dispatched to IDBRequest after its execution context is stopped, but this is not standard behavior. WebKit and Blink can stop dispatching event as soon as iframe is detached (iframe.remove() is invoked),
which leads to test timeout. Since the goal of the test is to verify the accessor is still valid after execution context is destroyed (no exception is thrown), we can just check readyState without wait.

@szewai
Copy link
Contributor Author

szewai commented Aug 7, 2024

The problem is mentioned in w3c/IndexedDB#416.
I verified the updated test passes in Chrome, Firefox and Safari.

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

openRequest.onsuccess = resolve;
});
assert_equals(openRequest.readyState, 'done');
await load_iframe();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to await anything here?

The original test was authored here:

https://chromium-review.googlesource.com/c/chromium/src/+/3125334

... and the intention was just to ensure that readyState() could be called without hitting a CHECK in Chromium after the context was destroyed.

In the PR description, you say "make the test wait on a new iframe with the same id to be loaded, so that there is enough time for request to be completed." - but we don't actually care about the request being completed -- or we'd assert that it was 'done'.

So what about just removing the await entirely (added in https://phabricator.services.mozilla.com/D203270) and just keeping the assert that readyState returns a string (i.e. keep the next line as you suggest)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the wait is unnecessary if we don't care about the value. I will update the PR.

The test expects event to be dispatched to IDBRequest after its execution
context is stopped, but this is not standard behavior. WebKit and Blink can stop
dispatching event as soon as iframe is detached (iframe.remove() is invoked),
which leads to test timeout. Since the goal of the test is to verify the accessor
is still valid after execution context is destroyed (no exception is thrown), we
can just check readyState without wait. We only check the type instead of value
as different engines handle the destruction differently, so the state may not be
updated right away.
@szewai szewai force-pushed the ready-state-destroyed-execution-context-html-is-failing branch from bf7538b to 39c6d72 Compare August 7, 2024 22:59
@szewai szewai requested a review from inexorabletash August 7, 2024 23:00
Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@szewai szewai merged commit fee8ca2 into web-platform-tests:master Aug 7, 2024
19 checks passed
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 15, 2024
This test is passing in Chromium after the latest update in the WPT repo
which removed the Promise that was causing a timeout in Chromium.
PR into WPT Repo: web-platform-tests/wpt#47517

Bug: 358257259
Change-Id: I531146593203076fa2f25d376d9cedbdd6518046
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789538
Auto-Submit: Rahul Singh <rahsin@microsoft.com>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Rahul Singh <rahsin@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1342187}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants