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

[IndexedDB] Avoid race condition redux #19985

Closed
wants to merge 1 commit into from

Conversation

@inexorabletash
Copy link
Contributor

inexorabletash commented Oct 29, 2019

One of the async subtests leaves a getter on Object.prototype across turns of the event loop, which can lead to flaky behavior. Scope the lifetime of the getter to a synchronous call following a pattern used elsewhere in the test file.

Pulling the test case into a separate file is insufficient as the getter can interfere with both the test harness and the webdriver implementation (specifically, chromedriver).

@jugglinmike
Copy link
Contributor

jugglinmike commented Oct 30, 2019

It seems like this test was intentionally authored to defer cleanup until after the asynchronous operation completed (even ignoring the "wrapper function" pattern--it could have been written to delete the property immediately after invoking put).

By synchronously removing the property accessor, won't we reduce coverage?

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Oct 30, 2019

By synchronously removing the property accessor, won't we reduce coverage?

Technically yes. But in the spirit of "tests should be verifying specific behavior in the spec", the array→key conversion should be happening synchronously during the put() call. We could imagine an implementation that's non-conforming and doing it asynchronously, which this test might accidentally provide coverage for, but that should be verified in other ways, e.g. mutating the array after the put() call.

I'm the original author of the test and honestly can't remember my mindset at the time. I do know that the corresponding C++ implementation change that went along with the test was only invoked synchronously during the put() call, so the test was overly broad.

@jugglinmike
Copy link
Contributor

jugglinmike commented Nov 1, 2019

It would be more helpful to document that the function wrapper is used to avoid interference with Chromium's infrastructure, specifically (and not WPT's testharness.js, for example). Even then, though, it doesn't seem ideal to work around the particulars of a specific implementation from WPT.

This has the potential to interfere with other tests, so maybe we can use this as motivation to alter Chromedriver itself. Unless I'm mistaken, the following change to the CacheWithUUID function might be all that's needed:

 /**
  * A cache which maps IDs <-> cached objects for the purpose of identifying
  * a script object remotely. Uses UUIDs for identification.
  * @constructor
  */
 function CacheWithUUID() {
-  this.cache_ = {};
+  this.cache_ = Object.create(null);
 }

@JohnChen0 does that seem like an improvement to you?

@JohnChen0
Copy link
Contributor

JohnChen0 commented Nov 1, 2019

I'm OK with using this.cache_ = Object.create(null).

@jugglinmike
Copy link
Contributor

jugglinmike commented Nov 2, 2019

Thanks, John! I'll see about getting you a patch next week.

@jugglinmike
Copy link
Contributor

jugglinmike commented Nov 7, 2019

Here's that patch: https://chromium-review.googlesource.com/c/chromium/src/+/1894707

With that landed, I believe this issue and gh-19713 can be closed without merging. Do you agree @inexorabletash?

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Nov 8, 2019

How does it look on the bots? I thought your other PR (that pulls out the one test case) might still be necessary. Not sure though.

This one can definitely close though - thanks!

@jugglinmike
Copy link
Contributor

jugglinmike commented Nov 8, 2019

How does it look on the bots?

The results are still inconsistent on wpt.fyi, but I imagine we'll have to wait on the ChromeDriver release process. I don't know when the change will make it to a ChromeDriver that we use.

I thought your other PR (that pulls out the one test case) might still be necessary. Not sure though.

That's gh-19713. I made a claim about that patch which is incorrect. I've elaborated there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.