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
Stop calling Promise.prototype.then() from StreamPromiseThen() #18452
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already reviewed downstream.
When on_fulfilled was null, StreamPromiseThen() used v8::Promise::Catch(), which calls promise.then(), resulting in a call to Promise.prototype.then which may not be set to the original value. Use the two-argument form of v8::Promise::Then() instead, which doesn't have the problem. Also add tests to verify that ReadableStream tee() and pipeTo() do not call Promise.prototype.then(). Bug: 992482 Change-Id: I5658f90df864785bfe6c54ae1bce37d7a2af6e0c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1755627 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Adam Rice <ricea@chromium.org> Cr-Commit-Position: refs/heads/master@{#687554}
50ec1b1
to
09f6d39
Compare
This change fails on the reference implementation: Test output
Specifically: since the test patches const startResult = startAlgorithm();
Promise.resolve(startResult).then(
() => { We could fix this for Even then, there are still a ton of cases where To fix this properly, I think the reference implementation needs to always call the original |
We have a history of (well-commented) reference implementation hacks specifically to pass the tests, so I'd just get the original |
Okay, in that case we should replace all occurrences of: somePromise.then(fulfillmentHandler, rejectionHandler) with: promiseThen(somePromise, fulfillmentHandler, rejectionHandler) where const originalPromiseThen = Promise.prototype.then;
function promiseThen(promise, fulfillmentHandler, rejectionHandler) {
return originalPromiseThen.call(promise, fulfillmentHandler, rejectionHandler);
} And similarly for I'll start working on a PR. It's not going to be pretty, but I guess it's necessary. 😅 |
Well, I was thinking we don't have to use it uniformly. We could just do it in the targeted locations necessary to pass this test, and add a comment. That might be be a weird in-between state though, so I'm not sure... |
Yeah, I think that makes the reference implementation geared too much towards the tests. I propose we do it uniformly. |
I like the uniform approach, particularly as we can make the reference implementation read closer to the standard text in the process. It also means I can add more |
Before this change, the reference implementation did not use the initial value of the Promise constructor and its methods to implement phrases such as "upon fulfillment of p with value v", although this is required by the promise guide. This causes the reference implementation to fail on the new tests introduced in web-platform-tests/wpt#18452. This change adds helper functions to call the original versions of the Promise constructor and its methods. Assuming they haven't been monkey-patched before the reference implementation was loaded, these should be the initial values expected by the spec. It also replaces all direct calls to the Promise constructors and its methods with calls to the respective helper functions. This is a purely mechanical change, but it does make for quite a big diff. With this change, the reference implementation once again passes all tests.
When on_fulfilled was null, StreamPromiseThen() used
v8::Promise::Catch(), which calls promise.then(), resulting in a call to
Promise.prototype.then which may not be set to the original value.
Use the two-argument form of v8::Promise::Then() instead, which doesn't
have the problem.
Also add tests to verify that ReadableStream tee() and pipeTo() do not
call Promise.prototype.then().
Bug: 992482
Change-Id: I5658f90df864785bfe6c54ae1bce37d7a2af6e0c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1755627
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#687554}