Fix cancelPromise in ReadableStreamTee being resolved twice (again) by MattiasBuelens · Pull Request #1118 · whatwg/streams

MattiasBuelens · 2021-03-27T00:04:24Z

I liked #1112 so much that I wanted to fix it twice! 😛

The following test case throws an AssertionError:

const rs = new ReadableStream({});

const [branch1, branch2] = rs.tee();

const cancel1 = branch1.cancel();
await flushAsyncEvents();
const cancel2 = branch2.cancel();

await Promise.all([cancel1, cancel2]);

This time, the error comes from this line:

promise_test: Unhandled rejection with value: object "AssertionError: false == true"
Error
    at AssertionError.get_stack (http://127.0.0.1:51551/resources/testharness.js:3553:21)
    at new AssertionError (http://127.0.0.1:51551/resources/testharness.js:3546:27)
    at assert (http://127.0.0.1:51551/resources/testharness.js:3539:19)
    at Test.<anonymous> (http://127.0.0.1:51551/resources/testharness.js:618:29)
    at Test.step (http://127.0.0.1:51551/resources/testharness.js:1977:25)
    at http://127.0.0.1:51551/resources/testharness.js:2002:35

At least two implementers are interested (and none opposed):
- Chome (@ricea)
- Firefox (@evilpie)
Tests are written and can be reviewed and commented upon at:
- Streams: add test for canceling both tee'd branches in sequence with delay web-platform-tests/wpt#28266
Implementation bugs are filed:

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

MattiasBuelens · 2021-03-27T00:10:15Z

I think this should be the last one. We now have checks in all places where we resolve cancelPromise, and each of those is covered by at least one test. 😅

For whatwg/streams#1118.

domenic

LGTM, and very nice find. It seems like the assertions are pretty worthwhile after all if they lead to discovering real browser bugs. Can you file Firefox and Safari bugs, and roll WPT again?

ricea · 2021-03-29T19:06:44Z

lgtm. You can say Chrome is interested.

evilpie · 2021-03-30T12:59:48Z

This is on my radar. I am going to fix it after the test landed in the mozilla repository.

MattiasBuelens · 2021-03-30T22:05:28Z

WPT updated. Should be ready to merge.

Can I assume that @ricea and @evilpie will open the Chromium and Firefox issues respectively? (When you do, comment with the issue link here so we can keep track.) Then I'll take care of opening the Safari issue. 🙂

MattiasBuelens · 2021-03-30T22:29:26Z

Hmm, make deploy failed on CI. Not sure why.

Gonna try to rebase on main and see if that helps. 😛

ricea · 2021-03-30T23:19:56Z

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1194266.

evilpie · 2021-04-01T20:08:10Z

https://bugzilla.mozilla.org/show_bug.cgi?id=1702636

…'d branches in sequence with delay, a=testonly Automatic update from web-platform-tests Streams: more tee() cancelation tests For whatwg/streams#1118. -- wpt-commits: 1bdb43faa7434d36645ab5c64e754b5caefbc9d2 wpt-pr: 28266

This change addresses the issue of resolving an already resolved |cancelPromise| in ReadableStreamTee, by checking if both branches have already been canceled. If that is the case, we will have already resolved |cancelPromise| and don't need to resolve it again. The corresponding specification changes are: - whatwg/streams#1112 - whatwg/streams#1118 Bug: 1194266 Change-Id: Iccd6917c1ed98ea5677ba71e050a5f07a5b2b6ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3565500 Reviewed-by: Adam Rice <[email protected]> Commit-Queue: Nidhi Jaju <[email protected]> Cr-Commit-Position: refs/heads/main@{#987847}

…'d branches in sequence with delay, a=testonly Automatic update from web-platform-tests Streams: more tee() cancelation tests For whatwg/streams#1118. -- wpt-commits: 1bdb43faa7434d36645ab5c64e754b5caefbc9d2 wpt-pr: 28266

MattiasBuelens mentioned this pull request Mar 27, 2021

Streams: add test for canceling both tee'd branches in sequence with delay web-platform-tests/wpt#28266

Merged

MattiasBuelens force-pushed the fix-tee-cancel-double-resolve-episode-two branch from ef6234b to e1c422b Compare March 27, 2021 00:32

domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 29, 2021

Streams: more tee() cancelation tests

1bdb43f

For whatwg/streams#1118.

domenic approved these changes Mar 29, 2021

View reviewed changes

MattiasBuelens force-pushed the fix-tee-cancel-double-resolve-episode-two branch from e1c422b to 2bfb5f3 Compare March 30, 2021 22:01

MattiasBuelens force-pushed the fix-tee-cancel-double-resolve-episode-two branch from 2bfb5f3 to bfa2036 Compare March 30, 2021 22:29

MattiasBuelens mentioned this pull request Mar 30, 2021

Fix dfn refs for arguments of "TransformStream/set up" #1119

Merged

MattiasBuelens added 2 commits March 31, 2021 01:18

Fix cancelPromise in ReadableStreamTee being resolved twice (again)

b996bd5

Roll WPT

9a5bcc7

MattiasBuelens force-pushed the fix-tee-cancel-double-resolve-episode-two branch from bfa2036 to 9a5bcc7 Compare March 30, 2021 23:18

domenic merged commit 6762cdb into whatwg:main Mar 31, 2021

MattiasBuelens deleted the fix-tee-cancel-double-resolve-episode-two branch March 31, 2021 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix cancelPromise in ReadableStreamTee being resolved twice (again)#1118

Fix cancelPromise in ReadableStreamTee being resolved twice (again)#1118
domenic merged 2 commits into
whatwg:mainfrom
MattiasBuelens:fix-tee-cancel-double-resolve-episode-two

MattiasBuelens commented Mar 27, 2021 •

edited

Loading

Uh oh!

MattiasBuelens commented Mar 27, 2021

Uh oh!

domenic left a comment

Uh oh!

ricea commented Mar 29, 2021

Uh oh!

evilpie commented Mar 30, 2021

Uh oh!

MattiasBuelens commented Mar 30, 2021

Uh oh!

MattiasBuelens commented Mar 30, 2021

Uh oh!

ricea commented Mar 30, 2021

Uh oh!

evilpie commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

MattiasBuelens commented Mar 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattiasBuelens commented Mar 27, 2021

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

ricea commented Mar 29, 2021

Uh oh!

evilpie commented Mar 30, 2021

Uh oh!

MattiasBuelens commented Mar 30, 2021

Uh oh!

MattiasBuelens commented Mar 30, 2021

Uh oh!

ricea commented Mar 30, 2021

Uh oh!

evilpie commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

MattiasBuelens commented Mar 27, 2021 •

edited

Loading