Add "mark promise as handled" change to pipeThrough()

Add "mark promise as handled" change to pipeThrough()

Update the implementation of ReadableStream pipeThrough() with standard change
https://github.com/whatwg/streams/commit/94140088224c1a0e219c630378d1fa7985d657a9

This makes it mark the Promise returned by pipeTo() as handled to avoid
uncatchable rejections when pipeTo fails.

BUG=668951
Review-Url: https://codereview.chromium.org/2796633002
Cr-Commit-Position: refs/heads/master@{#464718}
Committed: https://chromium.googlesource.com/chromium/src/+/1e9fa3b2e7dca2ba228b9720570512676ed23798

[Adam Rice, 2017-04-03 14:55:52]

ricea@chromium.org changed reviewers:
+ domenic@chromium.org

[Adam Rice, 2017-04-05 06:45:41]

Patchset #2 (id:20001) has been deleted

[domenic, 2017-04-05 08:22:30]

https://codereview.chromium.org/2796633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/streams/ReadableStream.js (right):

https://codereview.chromium.org/2796633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/streams/ReadableStream.js:185: if (promise
instanceof Promise) {
This will not work; the next line will cause a crash for e.g. `{ __proto__:
Promise.prototype }` or `{ [Symbol.hasInstance() { return true; } }`. (I think
those are the right incantations to fool it, but I might be off a bit.)

We should add tests to WPT (either by editing external/wpt directly and letting
the bot export them, or on GitHub) for these cases.

The real fix is https://codereview.chromium.org/2796243002

[Adam Rice, 2017-04-05 08:31:01]

https://codereview.chromium.org/2796633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/streams/ReadableStream.js (right):

https://codereview.chromium.org/2796633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/streams/ReadableStream.js:185: if (promise
instanceof Promise) {
On 2017/04/05 08:22:30, domenic wrote:
> This will not work; the next line will cause a crash for e.g. `{ __proto__:
> Promise.prototype }` or `{ [Symbol.hasInstance() { return true; } }`. (I think
> those are the right incantations to fool it, but I might be off a bit.)
> 
> We should add tests to WPT (either by editing external/wpt directly and
letting
> the bot export them, or on GitHub) for these cases.

I was thinking along the same lines, but I realised if we added a test for this
the reference implementation would fail it :-(

Maybe I should add a Chromium-specific test for this?

> The real fix is https://codereview.chromium.org/2796243002

I will use this once it is rolled.

[domenic, 2017-04-05 08:41:18]

On 2017/04/05 at 08:31:01, ricea wrote:
>
https://codereview.chromium.org/2796633002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/streams/ReadableStream.js (right):
> 
>
https://codereview.chromium.org/2796633002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/streams/ReadableStream.js:185: if (promise
instanceof Promise) {
> On 2017/04/05 08:22:30, domenic wrote:
> > This will not work; the next line will cause a crash for e.g. `{ __proto__:
> > Promise.prototype }` or `{ [Symbol.hasInstance() { return true; } }`. (I
think
> > those are the right incantations to fool it, but I might be off a bit.)
> > 
> > We should add tests to WPT (either by editing external/wpt directly and
letting
> > the bot export them, or on GitHub) for these cases.
> 
> I was thinking along the same lines, but I realised if we added a test for
this the reference implementation would fail it :-(
> 
> Maybe I should add a Chromium-specific test for this?

Ah, no, I just realized, there is something the reference implementation can
do---and indeed, something we can do too, given that this isn't
performance-critical code:

```js
function isPromise(o) {
  try {
    promiseThen(o);
  } catch (e) {
    return false;
  }

  return true;
}
```

[Adam Rice, 2017-04-14 08:00:59]

ricea@chromium.org changed reviewers:
+ tyoshino@chromium.org

[Adam Rice, 2017-04-14 08:01:00]

+tyoshino PTAL

[tyoshino (SeeGerritForStatus), 2017-04-14 10:25:42]

lgtm

[Adam Rice, 2017-04-14 10:47:23]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2017-04-14 10:47:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 12:47:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492166843027450,
"parent_rev": "d68c1beaad7929dddc6e6e1278dd03fedcd348eb", "commit_rev":
"1e9fa3b2e7dca2ba228b9720570512676ed23798"}

[commit-bot: I haz the power, 2017-04-14 12:48:41]

Description was changed from

==========
Add "mark promise as handled" change to pipeThrough()

Update the implementation of ReadableStream pipeThrough() with standard change
https://github.com/whatwg/streams/commit/94140088224c1a0e219c630378d1fa7985d6...

This makes it mark the Promise returned by pipeTo() as handled to avoid
uncatchable rejections when pipeTo fails.

BUG=668951
==========

to

==========
Add "mark promise as handled" change to pipeThrough()

Update the implementation of ReadableStream pipeThrough() with standard change
https://github.com/whatwg/streams/commit/94140088224c1a0e219c630378d1fa7985d6...

This makes it mark the Promise returned by pipeTo() as handled to avoid
uncatchable rejections when pipeTo fails.

BUG=668951

Review-Url: https://codereview.chromium.org/2796633002
Cr-Commit-Position: refs/heads/master@{#464718}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e9fa3b2e7dca2ba228b97205705...
==========

[commit-bot: I haz the power, 2017-04-14 12:48:42]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1e9fa3b2e7dca2ba228b97205705...