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

Stop calling Promise.prototype.then() from StreamPromiseThen() #18452

Merged
merged 1 commit into from Aug 16, 2019

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Aug 15, 2019

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}

Copy link
Collaborator

wpt-pr-bot left a comment

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}
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1755627 branch from 50ec1b1 to 09f6d39 Aug 16, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 7046bf4 into master Aug 16, 2019
14 checks passed
14 checks passed
update-pr-preview
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
Azure Pipelines Build #20190816.33 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
staging.wpt.fyi - chrome[experimental] Chrome results
Details
staging.wpt.fyi - firefox[experimental] Firefox results
Details
staging.wpt.fyi - safari[experimental] Safari results
Details
wpt.fyi - chrome[experimental] Chrome results
Details
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1755627 branch Aug 16, 2019
@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Aug 18, 2019

This change fails on the reference implementation:

Test output
  readable-streams/patched-global.any.html

  × tee() should not call Promise.prototype.then()

    patched then() called
    Error: patched then() called
        at Promise.then (http://127.0.0.1:53050/streams/readable-streams/patched-global.any.js:114:11)
        at SetUpReadableStreamDefaultController (eval at setup (C:\Users\Mattias\Documents\GitHub\whatwg-streams\reference-implementation\run-web-platform-tests.js:43:14), <anonymous>:1643:32)
        at SetUpReadableStreamDefaultControllerFromUnderlyingSource (eval at setup (C:\Users\Mattias\Documents\GitHub\whatwg-streams\reference-implementation\run-web-platform-tests.js:43:14), <anonymous>:1671:3)
        at new ReadableStream (eval at setup (C:\Users\Mattias\Documents\GitHub\whatwg-streams\reference-implementation\run-web-platform-tests.js:43:14), <anonymous>:367:7)
        at Test.t (http://127.0.0.1:53050/streams/readable-streams/patched-global.any.js:119:30)
        at Test.step (http://127.0.0.1:53050/resources/testharness.js:1561:25)
        at test (http://127.0.0.1:53050/resources/testharness.js:544:30)
        at http://127.0.0.1:53050/streams/readable-streams/patched-global.any.js:111:1
        at Script.runInContext (vm.js:130:20)
        at Object.runInContext (vm.js:308:6)
  × pipeTo() should not call Promise.prototype.then()

    patched then() called
    Error: patched then() called
        at Promise.then (http://127.0.0.1:53050/streams/readable-streams/patched-global.any.js:127:11)
        at SetUpReadableStreamDefaultController (eval at setup (C:\Users\Mattias\Documents\GitHub\whatwg-streams\reference-implementation\run-web-platform-tests.js:43:14), <anonymous>:1643:32)
        at SetUpReadableStreamDefaultControllerFromUnderlyingSource (eval at setup (C:\Users\Mattias\Documents\GitHub\whatwg-streams\reference-implementation\run-web-platform-tests.js:43:14), <anonymous>:1671:3)
        at new ReadableStream (eval at setup (C:\Users\Mattias\Documents\GitHub\whatwg-streams\reference-implementation\run-web-platform-tests.js:43:14), <anonymous>:367:7)
        at Test.t (http://127.0.0.1:53050/streams/readable-streams/patched-global.any.js:133:14)
        at Test.step (http://127.0.0.1:53050/resources/testharness.js:1561:25)
        at test (http://127.0.0.1:53050/resources/testharness.js:544:30)
        at http://127.0.0.1:53050/streams/readable-streams/patched-global.any.js:124:1
        at Script.runInContext (vm.js:130:20)
        at Object.runInContext (vm.js:308:6)

Specifically: since the test patches .then before calling new ReadableStream(), the following line inside SetUpReadableStreamDefaultController hits the patched .then method:

  const startResult = startAlgorithm();
  Promise.resolve(startResult).then(
    () => {

We could fix this for pipeTo() by only patching .then after constructing the readable stream. However, this won't work for tee(), since that method must construct two ReadableStream instances and thus call into SetUpReadableStreamDefaultController.

Even then, there are still a ton of cases where .then is called inside pipeTo (1, 2, 3) and tee (1, 2). The only reason these tests passed previously is because either the stream was already closed (in the case of pipeTo) or because no chunks were ever enqueued (in the case of tee).

To fix this properly, I think the reference implementation needs to always call the original .then and .catch methods everywhere. I think we'd also need to worry about patching the global Promise constructor, since that's also used internally. That means replacing a ton of constructor calls and method calls with calls to helper functions instead. I'm a bit worried this change would make the reference implementation less readable though, so I'm not sure if this is the best approach. 😕

@ricea @domenic Thoughts?

@domenic
Copy link
Member

domenic commented Aug 19, 2019

We have a history of (well-commented) reference implementation hacks specifically to pass the tests, so I'd just get the original then and use it to pass this particular test.

@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Aug 19, 2019

Okay, in that case we should replace all occurrences of:

somePromise.then(fulfillmentHandler, rejectionHandler)

with:

promiseThen(somePromise, fulfillmentHandler, rejectionHandler)

where promiseThen is a helper function that looks something like:

const originalPromiseThen = Promise.prototype.then;
function promiseThen(promise, fulfillmentHandler, rejectionHandler) {
  return originalPromiseThen.call(promise, fulfillmentHandler, rejectionHandler);
}

And similarly for .catch(), new Promise(), Promise.resolve() and Promise.reject(). Basically, every phrase from the promise guide needs a corresponding helper function.

I'll start working on a PR. It's not going to be pretty, but I guess it's necessary. 😅

@domenic
Copy link
Member

domenic commented Aug 19, 2019

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...

@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Aug 19, 2019

Yeah, I think that makes the reference implementation geared too much towards the tests. I propose we do it uniformly.

@ricea
Copy link
Contributor

ricea commented Aug 20, 2019

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 Promise.prototype-mangling tests in future, though I don't think I'll ever be able to upstream https://cs.chromium.org/chromium/src/third_party/blink/web_tests/http/tests/streams/chromium/touching-global-object.html 😄

domenic added a commit to whatwg/streams that referenced this pull request Aug 23, 2019
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.
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.