Assigning to v8 sheriff for triage. This seems similar to  issue 743042  although the v8 roll isn't in the regression range. 

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

-> current sheriff

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

CF managed to create a case where "writer[_closedPromise]" is undefined at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/streams/WritableStream.js?type=cs&l=411
which is not expected by promises implementation. 

Cool! I was worried this might happen, but I couldn't find any holes in our implementation. Hooray for Clusterfuzz! I mean it, this is really great.

Do you want a quick patch or can we take a couple of days to fix it properly?

Given that we are going to branch in two days I'd prefer to have a proper fix quickly :)

What I have so far is that we get a "Maximum call stack size exceeded" exception. My hypothesis is this is caused by the self-assignment inside the Array setter.

What happens next is still not totally clear, but my guess is that the stack overflow actually happens inside the call to getWriter(). This leaves the object in a broken state.

This seems like something we need to be able to handle, but I don't know how to do so in a general way. It appears that everything abstract operation which is marked "nothrow" in the standard (https://streams.spec.whatwg.org/) which calls another function can actually throw?

Here's a somewhat reduced and readable version wrapped in a layout test for easy running.

A key point I missed before is that

    try { this[0] = 0.1; } catch (e) {  }

effectively functions as a loop to get the stack as full as possible before the first call to ws.getWriter(). This is important because every call to getWriter() after the first is a no-op.

Deleted: closedPromise-undefined.html 442 bytes

closedPromise-undefined.html 442 bytes View Download

I replaced the confusing business with the Array prototype with a simple recursive function. The core of the repro is now this:

  const ws = new WritableStream();
  const abortPromise = ws.abort();
  function goDeeper() {
    try {
      goDeeper();
      ws.getWriter();
    } catch (e) {}
  }
  goDeeper();

ws.getWriter() gets called once-per-level as the stack unwinds, which is a near-perfect method for triggering this kind of bug.

Deleted: closedPromise-undefined.html 514 bytes

closedPromise-undefined.html 514 bytes View Download

I've been thinking about two approaches to fix this:

1. Make sure ReadableStream and WritableStream don't violate security even if their internal state is inconsistent.
2. Make sure the internal state of ReadableStream and WritableStream is never inconsistent.

2. is obviously better but looks extremely difficult. I've been investigating whether there is some way to verify that we have sufficient available stack at each of the public entry points, but it appears not. Another approach is to always ensure we are in an internally consistent state before performing any operation that might lead to a stack overflow. This restructuring is itself difficult, but a bigger problem is how do we know which operations might overflow the stack? Can assignment overflow the stack? How about dereferencing a field?

So for the time being I am focussing on 1. I am assuming we only need to protect operations that call into V8 intrinsics. It should be sufficient to wrap them.

I'm concerned about the long-term viability of approach 1. For example, if we set the body of a Request to a ReadableStream and pass that to fetch(), then the implementation needs to be able to cope with being passed a corrupted ReadableStream with invalid behaviour.

Domenic, what's the severity if we leak InternalPackedArray? We could switch to an ordinary Array if necessary.

Leaking InternalPackedArray would be very bad. The reason is that V8 uses InternalPackedArrays to implement some JS built-ins. They do not use ordinary Arrays because these can be monkey-patched, e.g. Array.prototype.push could be overwritten by user code. By using InternalPackedArrays, we side step this issue. But if it's leaked, it can also get monkey-patched.

Iirc v8 extras work like this: whatever feature is being implemented is implemented partly in JS and partly in C++. The JS part is included as extras library in V8 so that it can be part of the snapshot. The C++ part lives in Blink. The binding (literally the "binding" argument in the extras script) object already exists at snapshotting time, but at Blink has to install C++ bindings at runtime so that they become available to the JS part of the implementation.

In d8, no binding is being installed at runtime. The JS part of the implementation exists, but would fail whenever it attempts to call to the C++ part.

So whatever your fix is, if it's only in the C++ part, that would not silence this feature.

I actually don't think it makes sense to run clusterfuzz on a d8 that includes extras JS from blink. It would test incomplete implementations that lack the binding to C++.

yangguo@: although I agree it's weird that clusterfuzz is running here, and in general compiling d8 with Blink's V8 extras doesn't make sense, in this case it caught a legit bug. Partially because streams have very little C++ `binding` parts. That was distilled into something you can run in a web browser in comment #16. So I'm not sure about running clusterfuzz in this way going forward, but at least for now it's discovered a bug...

So yeah, that test case in comment #16 is scary. This whole bug is scary. How do/did V8 built-ins deal with this?

My first thought was to try to restore invariants with try { ... } finally { ... }. I guess that is a variant on (2)?

How bad is crashing on stack overflow? Crashing seems relatively safe. Why are people talking about leaking InternalPackedArray? Was that a red herring?

Maybe one approach that also addresses the long-term viability concern is to have an invariant check function. Then we can run that in various points (Certainly when accepting a stream in fetch, but maybe also in various public API calls? Before calling underlying source functions?) and bail out if any invariants are violated.

yangguo@: I think what I will do is temporarily switch from InternalPackedArray to Array. It's actually only used inside SimpleQueue, which I can harden by using internal symbols. SimpleQueue has a forEach() method which is slightly tricky; the rest should be trivial to review for exception safety. Once we have confidence that SimpleQueue is exception-safe, we can switch back to using InternalPackedArray.

domenic@: I looked at V8 built-ins, and I couldn't find any systematic solution to this problem. Many of them behave like pure functions with no (Javascript-exposed) internal state and so are innately safe. Others proxy to runtime C++ functions which do the actual state management and have stack checks on entry. Others, I just don't know.

I abandoned the try {} catch(e) {} approach because I expect the overhead to be very high. I also thought about what it would take to rollback to a known-good state inside the catch block, and that requires us to know the full space of internally consistent states, or to implement some kind of transaction log we can unwind. If we understand the full state space we can restructure the code to be exception safe. The restructuring would reduce our correspondence with the standard text, but would almost certainly be more performant than scattering try {} blocks everywhere. A transaction log sounds like massive overkill, and also costly.

try { } finally { } strikes me as even harder than try { } catch (){}, since inside the finally block we'd need logic like

if (!IsStateInternallyConsistent()) MakeStateInternallyConsistent();

It's worth mentioning that the concept of "state" includes things like "are we waiting for the underlying sink write() method?" which we can only determine by looking at our internal state, which might be corrupted.

We also need to know where to put the try {} blocks. Obviously they need to go around any of our own functions that changes state, but where else? I know from inspection that resolvePromise() can throw stack exceeded, but what else? Can it vary between different versions of v8? Different optimisation levels? Different architectures?

As you say, crashing because someone called us with a full stack doesn't seem like a big problem. Either the user code was broken anyway or it was an intentional attack. However, the crash only happens when the WritableStream makes use of the corrupted state. A careful attacker can prevent WritableStream doing something with that state until they are ready for it. By carefully controlling the stack size and calling pattern they can trigger exceptions in parts of the code of their choosing, causing precise mutations of the internal state. By repeatedly performing small mutations they can achieve nearly arbitrary states, with the goal of getting us to leak a private object.

I doubt they could get to the InternalPackedArray directly but once they have one private object they can leverage that to get more.

Re #20:
- Clusterfuzz should actually be running content_shell.
- Crashing on stack overflow would be very unexpected. Iirc ECMA spec has no notion of stack overflow, and we try very hard to turn all cases of stack overflow into an exception. In particular we fix all the cases that Clusterfuzz finds.
- V8 team is moving away from implementing built-ins in JS, but rather in C++ or our internal CodeStubAssembler. I'm not sure what should be done with extras. Maybe Jochen can give a bigger picture here?

Re #21:
- It's not obvious to me how an InternalPackedArray might escape just looking at the code. However, it is a bad idea to just replace it with a normal Array. I explained this in #19. You can easily set up some getter for array elements on the Array.prototype and wreck your built-in implementation if it depends on Arrays. In V8, we usually use an InternalPackedArray internally, and transfer its content to an Array before passing it to the user: https://cs.chromium.org/search/?q=%25MoveArrayContents&sq=package:chromium&type=cs
- Again, crashing on full stack should be avoided. Maybe we should expose v8::internal::StackLimitCheck to the API?

#22: If we could use v8::internal::StackLimitCheck from Javascript at public entry points with a sufficiently large gap value to ensure no nested functions throw then that should solve the problem. I noticed that the gap set for compilation is 40KB. I guess that compilation can be triggered by us calling one of our own functions, and so we need to leave enough space for compilation in addition to whatever we need to run our own code?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2431d137d1016a82d007556eb15189f0500507b1

commit 2431d137d1016a82d007556eb15189f0500507b1
Author: Adam Rice <ricea@chromium.org>
Date: Fri Jul 21 06:57:22 2017

Wrap Promise manipulation methods in Stream API

Maximum stack space exceeded exceptions within the WritableStream and
ReadableStream implementations can lead to slots being undefined that
are expected to contain Promises. This in turn leads to CHECK
failures. Protect all calls to V8 Promise intrinsics with checks that
the expected Promise is present, and throw an exception if it isn't.

Also add a test to verify that a CHECK failure does not occur.

Bug:  743082 
Change-Id: Iaf89aa2a694600d129021e7e902c6bc02401c8b4
Reviewed-on: https://chromium-review.googlesource.com/579628
Commit-Queue: Takeshi Yoshino <tyoshino@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488605}
[add] https://crrev.com/2431d137d1016a82d007556eb15189f0500507b1/third_party/WebKit/LayoutTests/http/tests/streams/chromium/deep-recursion-getwriter.html
[modify] https://crrev.com/2431d137d1016a82d007556eb15189f0500507b1/third_party/WebKit/Source/core/streams/ReadableStream.js
[modify] https://crrev.com/2431d137d1016a82d007556eb15189f0500507b1/third_party/WebKit/Source/core/streams/WritableStream.js

For reference, here is the sequence of operations that happen in the repro in #16:

--
abort() schedules [Microtask 1]

goDeeper() try {
  goDeeper() try {
    ...
              goDeeper() try {
                goDeeper() -> throws stack exceeded
              } catch {}
            getWriter() -> throws calling IsWritableStream
          getWriter() -> throws calling WritableStreamDefaultWriter constructor
        getWriter() -> throws calling IsWritableStream (inside constructor)
      getWriter() -> throws calling Promise_reject
                     stream[_writer] has been set
                     but writer[_closedPromise] has not
    getWriter() -> throws "Illegal Constructor" ([_writer] already set)
  ...
  getWriter() -> throws "Illegal Constructor"
} catch {}

[Microtask 1]: finds [_writer] set, tries to reject [_closedPromise]
               [_closedPromise] is undefined, so CHECK fails.
--


Some of this is speculative, particularly the exact locations that the stack exceeded exceptions are thrown inside getWriter(), which are based on reading the source and guessing. I haven't found a way to pinpoint where the exceptions are thrown in practice.

ClusterFuzz has detected this issue as fixed in range 488601:488619.

Detailed report: https://clusterfuzz.com/testcase?key=6086398158569472

Fuzzer: v8_builtins_generator
Job Type: linux_msan_d8
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  args[0]->IsJSPromise() in runtime-promise.cc
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_msan_d8&range=463944:463968
Fixed: https://clusterfuzz.com/revisions?job=linux_msan_d8&range=488601:488619

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6086398158569472


See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 6086398158569472 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3bdaf8ef960c95c2f7836bebe0be476d0d1e2fa

commit a3bdaf8ef960c95c2f7836bebe0be476d0d1e2fa
Author: Adam Rice <ricea@chromium.org>
Date: Mon Jul 24 08:49:54 2017

Streams API: Harden SimpleQueue

Make the Javascript SimpleQueue class used internally by ReadableStream
and WritableStream more resilient against stack overflow exceptions and
incorrect usage. This is part of a defense-in-depth strategy against
stack overflow bugs.

Internal fields are now stored as private symbols so they cannot be
accessed by the caller. Mutations to object state have been reordered so
that they happen after any operation that could throw a stack overflow
exception. shift() and peek() now throw a RangeError if called on an
empty queue.

In the process of the change, the semantics of the |cursor| field has
been changed so that it never points off-the-end when the queue is
non-empty. This has simplified the implementation of shift() and peek().

This change also adds Chromium-specific layout tests which create
sufficiently large queues to exercise the code in SimpleQueue that
creates and switches nodes.

Bug:  743082 
Change-Id: I2ae538bc2b3b00df2c81214a49e36aa5e6bcdf33
Reviewed-on: https://chromium-review.googlesource.com/581248
Commit-Queue: Adam Rice <ricea@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488932}
[add] https://crrev.com/a3bdaf8ef960c95c2f7836bebe0be476d0d1e2fa/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue.html
[modify] https://crrev.com/a3bdaf8ef960c95c2f7836bebe0be476d0d1e2fa/third_party/WebKit/Source/core/streams/SimpleQueue.js

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/adfbc34fcac6289a34bfd719501119a587a27cb8

commit adfbc34fcac6289a34bfd719501119a587a27cb8
Author: Robert Flack <flackr@chromium.org>
Date: Mon Jul 24 14:03:21 2017

Revert "Streams API: Harden SimpleQueue"

This reverts commit a3bdaf8ef960c95c2f7836bebe0be476d0d1e2fa.

Reason for revert: simple-queue.html test is failing on the MSAN bot. First failure is here: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20MSAN/builds/2173

Original change's description:
> Streams API: Harden SimpleQueue
> 
> Make the Javascript SimpleQueue class used internally by ReadableStream
> and WritableStream more resilient against stack overflow exceptions and
> incorrect usage. This is part of a defense-in-depth strategy against
> stack overflow bugs.
> 
> Internal fields are now stored as private symbols so they cannot be
> accessed by the caller. Mutations to object state have been reordered so
> that they happen after any operation that could throw a stack overflow
> exception. shift() and peek() now throw a RangeError if called on an
> empty queue.
> 
> In the process of the change, the semantics of the |cursor| field has
> been changed so that it never points off-the-end when the queue is
> non-empty. This has simplified the implementation of shift() and peek().
> 
> This change also adds Chromium-specific layout tests which create
> sufficiently large queues to exercise the code in SimpleQueue that
> creates and switches nodes.
> 
> Bug:  743082 
> Change-Id: I2ae538bc2b3b00df2c81214a49e36aa5e6bcdf33
> Reviewed-on: https://chromium-review.googlesource.com/581248
> Commit-Queue: Adam Rice <ricea@chromium.org>
> Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#488932}

TBR=ricea@chromium.org,tyoshino@chromium.org

Change-Id: Ia5e4b6fd2aa9bf5b652881cc27ba50561a1b84c9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  743082 
Reviewed-on: https://chromium-review.googlesource.com/583007
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488962}
[delete] https://crrev.com/933261416a9cf80b6ac4e6857d4b0ca601aa9f33/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue.html
[modify] https://crrev.com/adfbc34fcac6289a34bfd719501119a587a27cb8/third_party/WebKit/Source/core/streams/SimpleQueue.js

Open, due to revert.

Benedikt and I discussed, and it seems a lot safer to not expose InternalPackedArray anymore, but instead expose an Array with null prototype.

The only use site [0] of InternalPackedArray only uses push, which can be replaced by setting an element at the end. Benedikt will work on optimizing this use case to make sure it does not regress performance.

[0] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/streams/SimpleQueue.js

#32: That sounds excellent. As you say, we don't actually need most of the functionality that InternalPackedArray provides.

My only concern is there may be non-Chrome users of V8 Extras that use more.

I'm not aware of any users of extras outside of Chrome. I think we are fine here.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9f278a0500975315f87c79e1f2dca81a2f79a05

commit a9f278a0500975315f87c79e1f2dca81a2f79a05
Author: Adam Rice <ricea@chromium.org>
Date: Tue Jul 25 15:03:57 2017

Streams API: Harden SimpleQueue

Make the Javascript SimpleQueue class used internally by ReadableStream
and WritableStream more resilient against stack overflow exceptions and
incorrect usage. This is part of a defense-in-depth strategy against
stack overflow bugs.

Internal fields are now stored as private symbols so they cannot be
accessed by the caller. Mutations to object state have been reordered so
that they happen after any operation that could throw a stack overflow
exception. shift() and peek() now throw a RangeError if called on an
empty queue.

In the process of the change, the semantics of the |cursor| field has
been changed so that it never points off-the-end when the queue is
non-empty. This has simplified the implementation of shift() and peek().

push() now aggressively creates new back nodes immediately well it fills
the old one. This is necessary to permit |cursor| to be set to 0 when
the last element is shifted off the old back node.

There are Chromium-specific layout tests which create sufficiently large
queues to exercise the code in SimpleQueue that creates and switches
nodes. There are also tests for the cases of inserting then consuming
one complete node, iterating over one complete node, and inserting and
consuming a new element after consuming one complete node.

Tested with asserts enabled, and also with QUEUE_MAX_ARRAY_SIZE values
of 1 and 2 on the web-platform-tests.

The simple-queue.html layout test is marked as slow because otherwise
the number of iterations it needs to do would timeout on the msan
bots. deep-recursion-getwriter.html has also been marked as slow because
of the large number of iterations it requires.

This is a combination of the reverted change
https://chromium-review.googlesource.com/c/581248 and the followup fix
https://chromium-review.googlesource.com/c/581956/.

Bug:  743082 
Change-Id: I97a5e9000e6f7b6cdec0aaf73adf8d427bd18203
Reviewed-on: https://chromium-review.googlesource.com/584649
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489299}
[modify] https://crrev.com/a9f278a0500975315f87c79e1f2dca81a2f79a05/third_party/WebKit/LayoutTests/SlowTests
[add] https://crrev.com/a9f278a0500975315f87c79e1f2dca81a2f79a05/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue.html
[modify] https://crrev.com/a9f278a0500975315f87c79e1f2dca81a2f79a05/third_party/WebKit/Source/core/streams/SimpleQueue.js

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

To clarify, the two CLs I want to merge are https://chromium-review.googlesource.com/579628 from #24, and  https://chromium-review.googlesource.com/584649 from #36. The second one is a respin of the reverted CL from #29, which was timing out on the msan bots due to a large test. In the new version of the CL I reduced the size of the test and marked it slow to avoid the timeout problem.

+awhalley@ for M61 merge review.

govind@ - good for 61.  ricea@ - many thanks!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa3b03a5cf0faf937d0f0f53333526686ae031d5

commit fa3b03a5cf0faf937d0f0f53333526686ae031d5
Author: Adam Rice <ricea@chromium.org>
Date: Fri Jul 28 08:49:21 2017

Reland "Streams API: Harden SimpleQueue"

This is a reland of a9f278a0500975315f87c79e1f2dca81a2f79a05
Original change's description:
> Streams API: Harden SimpleQueue
> 
> Make the Javascript SimpleQueue class used internally by ReadableStream
> and WritableStream more resilient against stack overflow exceptions and
> incorrect usage. This is part of a defense-in-depth strategy against
> stack overflow bugs.
> 
> Internal fields are now stored as private symbols so they cannot be
> accessed by the caller. Mutations to object state have been reordered so
> that they happen after any operation that could throw a stack overflow
> exception. shift() and peek() now throw a RangeError if called on an
> empty queue.
> 
> In the process of the change, the semantics of the |cursor| field has
> been changed so that it never points off-the-end when the queue is
> non-empty. This has simplified the implementation of shift() and peek().
> 
> push() now aggressively creates new back nodes immediately well it fills
> the old one. This is necessary to permit |cursor| to be set to 0 when
> the last element is shifted off the old back node.
> 
> There are Chromium-specific layout tests which create sufficiently large
> queues to exercise the code in SimpleQueue that creates and switches
> nodes. There are also tests for the cases of inserting then consuming
> one complete node, iterating over one complete node, and inserting and
> consuming a new element after consuming one complete node.
> 
> Tested with asserts enabled, and also with QUEUE_MAX_ARRAY_SIZE values
> of 1 and 2 on the web-platform-tests.
> 
> The simple-queue.html layout test is marked as slow because otherwise
> the number of iterations it needs to do would timeout on the msan
> bots. deep-recursion-getwriter.html has also been marked as slow because
> of the large number of iterations it requires.
> 
> This is a combination of the reverted change
> https://chromium-review.googlesource.com/c/581248 and the followup fix
> https://chromium-review.googlesource.com/c/581956/.
> 
> Bug:  743082 
> Change-Id: I97a5e9000e6f7b6cdec0aaf73adf8d427bd18203
> Reviewed-on: https://chromium-review.googlesource.com/584649
> Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
> Commit-Queue: Adam Rice <ricea@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#489299}

Bug:  743082 
Change-Id: I2072bcaed827588fdffc46df0112a36da6d62681
Reviewed-on: https://chromium-review.googlesource.com/590912
Commit-Queue: Adam Rice <ricea@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490325}
[modify] https://crrev.com/fa3b03a5cf0faf937d0f0f53333526686ae031d5/third_party/WebKit/LayoutTests/SlowTests
[add] https://crrev.com/fa3b03a5cf0faf937d0f0f53333526686ae031d5/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue.html
[modify] https://crrev.com/fa3b03a5cf0faf937d0f0f53333526686ae031d5/third_party/WebKit/Source/core/streams/SimpleQueue.js

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Approving merge to M61 branch 3163 based on comment #41. Please merge ASAP. Thank you.

Pls merge you change to M61 branch 3163 before 3:00 PM PT on Monday so we can take it in for next week last M61 Dev release. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6725ae7b0461c8bcdd85cc7da7173fde95cad82b

commit 6725ae7b0461c8bcdd85cc7da7173fde95cad82b
Author: Adam Rice <ricea@chromium.org>
Date: Mon Jul 31 06:13:57 2017

Wrap Promise manipulation methods in Stream API

Maximum stack space exceeded exceptions within the WritableStream and
ReadableStream implementations can lead to slots being undefined that
are expected to contain Promises. This in turn leads to CHECK
failures. Protect all calls to V8 Promise intrinsics with checks that
the expected Promise is present, and throw an exception if it isn't.

Also add a test to verify that a CHECK failure does not occur.

TBR=ricea@chromium.org

(cherry picked from commit 2431d137d1016a82d007556eb15189f0500507b1)

Bug:  743082 
Change-Id: Iaf89aa2a694600d129021e7e902c6bc02401c8b4
Reviewed-on: https://chromium-review.googlesource.com/579628
Commit-Queue: Takeshi Yoshino <tyoshino@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#488605}
Reviewed-on: https://chromium-review.googlesource.com/593386
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#141}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[add] https://crrev.com/6725ae7b0461c8bcdd85cc7da7173fde95cad82b/third_party/WebKit/LayoutTests/http/tests/streams/chromium/deep-recursion-getwriter.html
[modify] https://crrev.com/6725ae7b0461c8bcdd85cc7da7173fde95cad82b/third_party/WebKit/Source/core/streams/ReadableStream.js
[modify] https://crrev.com/6725ae7b0461c8bcdd85cc7da7173fde95cad82b/third_party/WebKit/Source/core/streams/WritableStream.js

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/042f321c9ff266eef2ec21f7562c909497e583d7

commit 042f321c9ff266eef2ec21f7562c909497e583d7
Author: Adam Rice <ricea@chromium.org>
Date: Mon Jul 31 06:16:12 2017

Reland "Streams API: Harden SimpleQueue"

This is a reland of a9f278a0500975315f87c79e1f2dca81a2f79a05
Original change's description:
> Streams API: Harden SimpleQueue
>
> Make the Javascript SimpleQueue class used internally by ReadableStream
> and WritableStream more resilient against stack overflow exceptions and
> incorrect usage. This is part of a defense-in-depth strategy against
> stack overflow bugs.
>
> Internal fields are now stored as private symbols so they cannot be
> accessed by the caller. Mutations to object state have been reordered so
> that they happen after any operation that could throw a stack overflow
> exception. shift() and peek() now throw a RangeError if called on an
> empty queue.
>
> In the process of the change, the semantics of the |cursor| field has
> been changed so that it never points off-the-end when the queue is
> non-empty. This has simplified the implementation of shift() and peek().
>
> push() now aggressively creates new back nodes immediately well it fills
> the old one. This is necessary to permit |cursor| to be set to 0 when
> the last element is shifted off the old back node.
>
> There are Chromium-specific layout tests which create sufficiently large
> queues to exercise the code in SimpleQueue that creates and switches
> nodes. There are also tests for the cases of inserting then consuming
> one complete node, iterating over one complete node, and inserting and
> consuming a new element after consuming one complete node.
>
> Tested with asserts enabled, and also with QUEUE_MAX_ARRAY_SIZE values
> of 1 and 2 on the web-platform-tests.
>
> The simple-queue.html layout test is marked as slow because otherwise
> the number of iterations it needs to do would timeout on the msan
> bots. deep-recursion-getwriter.html has also been marked as slow because
> of the large number of iterations it requires.
>
> This is a combination of the reverted change
> https://chromium-review.googlesource.com/c/581248 and the followup fix
> https://chromium-review.googlesource.com/c/581956/.
>
> Bug:  743082 
> Change-Id: I97a5e9000e6f7b6cdec0aaf73adf8d427bd18203
> Reviewed-on: https://chromium-review.googlesource.com/584649
> Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
> Commit-Queue: Adam Rice <ricea@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#489299}

TBR=ricea@chromium.org

(cherry picked from commit fa3b03a5cf0faf937d0f0f53333526686ae031d5)

Bug:  743082 
Change-Id: I2072bcaed827588fdffc46df0112a36da6d62681
Reviewed-on: https://chromium-review.googlesource.com/590912
Commit-Queue: Adam Rice <ricea@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#490325}
Reviewed-on: https://chromium-review.googlesource.com/593407
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#142}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/042f321c9ff266eef2ec21f7562c909497e583d7/third_party/WebKit/LayoutTests/SlowTests
[add] https://crrev.com/042f321c9ff266eef2ec21f7562c909497e583d7/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue.html
[modify] https://crrev.com/042f321c9ff266eef2ec21f7562c909497e583d7/third_party/WebKit/Source/core/streams/SimpleQueue.js

Merge complete.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9122293ecfc27753c649781333b4b23f1fa613a4

commit 9122293ecfc27753c649781333b4b23f1fa613a4
Author: Adam Rice <ricea@chromium.org>
Date: Tue Aug 01 06:12:11 2017

Streams API: Fix for SimpleQueue with full node

SimpleQueue would erroneously set its "front" slot to undefined when shifting
out the last element of a completely full back node.

Modify push() to aggressively create a new back node immediately when it
fills the back node. This permits shift() to consume the final element
without breaking the invariant that the cursor position remains less
than QUEUE_MAX_ARRAY_SIZE.

Also add new tests for the cases of inserting then consuming one
complete node, iterating over one complete node, and inserting and
consuming a new element after consuming one complete node.

This change was originally landed as part of
https://chromium-review.googlesource.com/c/584649/ which was then
reverted due to some missing slow test expectations.

Due to an oversight, the reland in
https://chromium-review.googlesource.com/c/593407/ was missing
this change.

This change also splits simple-queue.html up into 5 parts to stop it
timing out on the MSAN bots.

Bug:  743082 
Change-Id: I791f39002911bd6c727807fa37cf2ae19dad878f
Reviewed-on: https://chromium-review.googlesource.com/593533
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490880}
[modify] https://crrev.com/9122293ecfc27753c649781333b4b23f1fa613a4/third_party/WebKit/LayoutTests/SlowTests
[modify] https://crrev.com/9122293ecfc27753c649781333b4b23f1fa613a4/third_party/WebKit/LayoutTests/http/tests/streams/chromium/README.txt
[add] https://crrev.com/9122293ecfc27753c649781333b4b23f1fa613a4/third_party/WebKit/LayoutTests/http/tests/streams/chromium/resources/simple-queue-common.js
[add] https://crrev.com/9122293ecfc27753c649781333b4b23f1fa613a4/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-empty-node-push.html
[add] https://crrev.com/9122293ecfc27753c649781333b4b23f1fa613a4/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-full-node-reject.html
[add] https://crrev.com/9122293ecfc27753c649781333b4b23f1fa613a4/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-full-node-shift.html
[add] https://crrev.com/9122293ecfc27753c649781333b4b23f1fa613a4/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-many-foreach.html
[add] https://crrev.com/9122293ecfc27753c649781333b4b23f1fa613a4/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-push-shift-peek.html
[delete] https://crrev.com/2398196195d7a4ca3e89ea4a173a53f3b0e89593/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue.html
[modify] https://crrev.com/9122293ecfc27753c649781333b4b23f1fa613a4/third_party/WebKit/Source/core/streams/SimpleQueue.js

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/834e1a93e2c3488fec158472e910909e3c9e414b

commit 834e1a93e2c3488fec158472e910909e3c9e414b
Author: Adam Rice <ricea@chromium.org>
Date: Fri Aug 04 01:43:12 2017

Streams API: Fix for SimpleQueue with full node

SimpleQueue would erroneously set its "front" slot to undefined when shifting
out the last element of a completely full back node.

Modify push() to aggressively create a new back node immediately when it
fills the back node. This permits shift() to consume the final element
without breaking the invariant that the cursor position remains less
than QUEUE_MAX_ARRAY_SIZE.

Also add new tests for the cases of inserting then consuming one
complete node, iterating over one complete node, and inserting and
consuming a new element after consuming one complete node.

This change was originally landed as part of
https://chromium-review.googlesource.com/c/584649/ which was then
reverted due to some missing slow test expectations.

Due to an oversight, the reland in
https://chromium-review.googlesource.com/c/593407/ was missing
this change.

This change also splits simple-queue.html up into 5 parts to stop it
timing out on the MSAN bots.

TBR=ricea@chromium.org

(cherry picked from commit 9122293ecfc27753c649781333b4b23f1fa613a4)

Bug:  743082 
Change-Id: I791f39002911bd6c727807fa37cf2ae19dad878f
Reviewed-on: https://chromium-review.googlesource.com/593533
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#490880}
Reviewed-on: https://chromium-review.googlesource.com/601134
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#303}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/834e1a93e2c3488fec158472e910909e3c9e414b/third_party/WebKit/LayoutTests/SlowTests
[modify] https://crrev.com/834e1a93e2c3488fec158472e910909e3c9e414b/third_party/WebKit/LayoutTests/http/tests/streams/chromium/README.txt
[add] https://crrev.com/834e1a93e2c3488fec158472e910909e3c9e414b/third_party/WebKit/LayoutTests/http/tests/streams/chromium/resources/simple-queue-common.js
[add] https://crrev.com/834e1a93e2c3488fec158472e910909e3c9e414b/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-empty-node-push.html
[add] https://crrev.com/834e1a93e2c3488fec158472e910909e3c9e414b/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-full-node-reject.html
[add] https://crrev.com/834e1a93e2c3488fec158472e910909e3c9e414b/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-full-node-shift.html
[add] https://crrev.com/834e1a93e2c3488fec158472e910909e3c9e414b/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-many-foreach.html
[add] https://crrev.com/834e1a93e2c3488fec158472e910909e3c9e414b/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue-push-shift-peek.html
[delete] https://crrev.com/4b0b385ad5eba8d657b9a20061b5c2c239e1afd3/third_party/WebKit/LayoutTests/http/tests/streams/chromium/simple-queue.html
[modify] https://crrev.com/834e1a93e2c3488fec158472e910909e3c9e414b/third_party/WebKit/Source/core/streams/SimpleQueue.js

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot