Closed Bug 1272697 Opened 8 years ago Closed 7 years ago

Implement ReadableStream in the JavaScript Engine

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bkelly, Assigned: till)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 15 obsolete files)

12.51 KB, patch
lth
: review+
Details | Diff | Splinter Review
226.83 KB, patch
shu
: review+
Details | Diff | Splinter Review
127.51 KB, patch
Details | Diff | Splinter Review
60.00 KB, patch
Details | Diff | Splinter Review
65.74 KB, patch
shu
: review+
Details | Diff | Splinter Review
506.77 KB, patch
baku
: feedback+
Details | Diff | Splinter Review
11.57 KB, patch
baku
: review+
jonco
: review+
Details | Diff | Splinter Review
As a first step I'd like to land pure js ReadableStream objects as self-hosted code in the js engine.  Further bugs will implement DOM bindings and platform defined streams.
This is the work-in-progress patch from bug 1128959.  I will make further refinements as follow-on patches.
Depends on: 911216
Depends on: 1272748
The spec has actually changed quite a bit since the P1 patch was first written.  I'm going to update to the latest spec in P3.
This is probably a stupid place to ask this question, but what is the difference between `@` and `.` in <https://streams.spec.whatwg.org/>?

    22. Upon rejection of reader@[[closedPromise]] with reason r,
        a.  If teeState.[[closedOrErrored]] is true, return undefined.
NI myself to answer comment 4.
Flags: needinfo?(bkelly)
It's the difference between internal slots (@) and Record fields (.). `reader` is a JavaScript object with internal slots. `teeState` is a spec-internal Record with fields.

This has come up a few times and seems to confuse people. We should perhaps think of a better solution. I still think this is better than the ES spec's way of making the distinction, which is "the [[closedPromise]] internal slot of reader" vs. "teeState.[[closedOrErrored]]".
Flags: needinfo?(bkelly)
Thanks, Domenic.

How about using `.[[FieldName]]` for both (fields of records and internal slots of objects)? We could even rename "internal slots" to "internal fields".

The virtue of this suggestion is that I don't think you would ever have to spend any time explaining it to anyone.
(FWIW, the distinction between @ and . has been removed per https://github.com/tc39/ecma262/issues/574.)
Work-in-progress on updating to the latest spec.  This roughs in the javascript bits, but I still have to add the intrinsics and c++ side of the prototypes.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Depends on: 1336705
Preparatory patch needed for the streams spec, which requires copying ranges from one ArrayBuffer to another, without going through a typed array view. See step 10.d in https://streams.spec.whatwg.org/#readable-byte-stream-controller-fill-pull-into-descriptor-from-queue for an example.
Assignee: nobody → till
Attachment #8752253 - Attachment is obsolete: true
Attachment #8752323 - Attachment is obsolete: true
Attachment #8768164 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8833610 - Flags: review?(lhansen)
Comment on attachment 8833610 [details] [diff] [review]
Part 1: Change ArrayBufferCopyData self-hosting intrinsic to take a start offset for the destination

Review of attachment 8833610 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, and it's a nice generalization regardless.
Attachment #8833610 - Flags: review?(lhansen) → review+
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f57723b3c9
Part 1: Change ArrayBufferCopyData self-hosting intrinsic to take a start offset for the destination. r=lth
Keywords: leave-open
Attached patch ReadableStream implementation (obsolete) — Splinter Review
Here's the first part, the pure-JS implementation of ReadableStream. This is passing all WPT tests (which can be run in the shell after applying the patch in bug 1333800, or in the browser without that), but lacks JSAPI functions for creating and operating on streams. I have a WIP patch for that locally but it's not ready for review.

I'm also aware of a few obvious optimization opportunities that we should definitely apply before shipping, but would like to do those as follow-ups.
Attachment #8836928 - Flags: review?(shu)
Comment on attachment 8836928 [details] [diff] [review]
ReadableStream implementation

Review of attachment 8836928 [details] [diff] [review]:
-----------------------------------------------------------------

Posting what comments I have now, since I'll need to finish review from my home computer.

::: js/src/builtin/Stream.cpp
@@ +78,5 @@
> +    JS_SELF_HOSTED_GET("locked", "ReadableStream_locked", 0),
> +    JS_PS_END
> +};
> +
> +CLASS_SPEC(ReadableStream, 0,

The nargs is 0 here because both parameters have defaults, right? That is, this nargs is actually "value of .length"?

@@ +91,5 @@
> +
> +    if (!ThrowIfNotConstructing(cx, args, "ReadableStreamDefaultReader"))
> +        return false;
> +
> +    Rooted<ReadableStreamDefaultReader*> reader(cx, NewBuiltinClassInstance<ReadableStreamDefaultReader>(cx));

Nit: line too long

@@ +133,5 @@
> +
> +    if (!ThrowIfNotConstructing(cx, args, "ReadableStreamBYOBReader"))
> +        return false;
> +
> +    Rooted<ReadableStreamBYOBReader*> reader(cx, NewBuiltinClassInstance<ReadableStreamBYOBReader>(cx));

Nit: line too long

@@ +175,5 @@
> +
> +    if (!ThrowIfNotConstructing(cx, args, "ReadableStreamDefaultController"))
> +        return false;
> +
> +    Rooted<ReadableStreamDefaultController*> controller(cx, NewBuiltinClassInstance<ReadableStreamDefaultController>(cx));

Nit: line too long

@@ +220,5 @@
> +
> +    if (!ThrowIfNotConstructing(cx, args, "ReadableByteStreamController"))
> +        return false;
> +
> +    Rooted<ReadableByteStreamController*> controller(cx, NewBuiltinClassInstance<ReadableByteStreamController>(cx));

Nit: line too long

@@ +265,5 @@
> +
> +    if (!ThrowIfNotConstructing(cx, args, "ReadableStreamBYOBRequest"))
> +        return false;
> +
> +    Rooted<ReadableStreamBYOBRequest*> request(cx, NewBuiltinClassInstance<ReadableStreamBYOBRequest>(cx));

Nit: line too long

@@ +274,5 @@
> +    initArgs[0].set(args.get(0));
> +    initArgs[1].set(args.get(1));
> +    initArgs[2].set(args.get(2));
> +    RootedValue thisv(cx, ObjectValue(*request));
> +    if (!CallSelfHostedFunction(cx, cx->names().ReadableStreamBYOBRequest, thisv, initArgs, initArgs.rval()))

Nit: line too long

@@ +298,5 @@
> +           ClassSpec::DontDefineConstructor);
> +
> +
> +static bool
> +ByteLengthQueuingStrategyConstructor(JSContext* cx, unsigned argc, Value* vp)

AFAICT this and CountQueuingStrategy are both completely generic and plain JS objects. It's unfortunate we need classes for these. Would these go away completely if we could self-host constructors?

::: js/src/builtin/Stream.h
@@ +10,5 @@
> +#include "vm/NativeObject.h"
> +
> +namespace js {
> +
> +class AutoSetNewObjectMetadata;

Stale forward declaration?

@@ +23,5 @@
> +    static const ClassSpec protoClassSpec_; \
> +    static const Class protoClass_; \
> +};
> +
> +NATIVE_CLASS_WITH_SLOTS(ReadableStream, 4)

Do the slot numbers need to be in the header? Can RESERVED_SLOTS be left to be defined in the .cpp file like the other static fields? Doesn't seem like this header should need to change when changing slot count.

@@ +36,5 @@
> +NATIVE_CLASS_WITH_SLOTS(WritableStreamDefaultController, 8)
> +
> +NATIVE_CLASS_WITH_SLOTS(ByteLengthQueuingStrategy, 0)
> +NATIVE_CLASS_WITH_SLOTS(CountQueuingStrategy, 0)
> +

#undef NATIVE_CLASS_WITH_SLOTS here.

::: js/src/builtin/Stream.js
@@ +4,5 @@
> +
> +// Streams spec, 3.2.3.
> +function ReadableStream(underlyingSource = {}, {size, highWaterMark} = {}) {
> +  if (!IsObject(this) || !IsReadableStream(this))
> +    ThrowTypeError(JSMSG_INCOMPATIBLE_PROTO, "ReadableStream", "ctor", typeof this);

Could these be changed to asserts? AFAICT these checks are to prevent accidental internal misuse, since the real constructor is in C++.

@@ +6,5 @@
> +function ReadableStream(underlyingSource = {}, {size, highWaterMark} = {}) {
> +  if (!IsObject(this) || !IsReadableStream(this))
> +    ThrowTypeError(JSMSG_INCOMPATIBLE_PROTO, "ReadableStream", "ctor", typeof this);
> +
> +  // Step 1: Set this@[[state]] to "readable".

All the confusing @/. got changed to . in the version I'm reading [1]. Change these to . as well to avoid confusion.

[1] https://streams.spec.whatwg.org/#rs-constructor

@@ +35,5 @@
> +                                                               highWaterMark));
> +  }
> +
> +  // Step 8: Otherwise, if type is undefined,
> +  else if (type === undefined) {

Nit: this else style here and below is kinda weird. I'm fine with // Step 8 appearing as the first line in the else body.

@@ +104,5 @@
> +// Streams spec, 3.2.4.4. pipeThrough({ writable, readable }, options)
> +function ReadableStream_pipeThrough({ writable, readable }, options) {
> +  // Step 1: Perform ? Invoke(this, "pipeTo", « writable, options »).
> +  callContentFunction("pipeTo", this, writable, options);
> +

I don't know our Promises. impl or the Promises spec as well as I should. The current step 2 of pipeThrough says:

2. If Type(promise) is Object and promise has a [[PromiseIsHandled]] internal slot, set promise.[[PromiseIsHandled]] to true.

Is that something that needs to be set?

@@ +110,5 @@
> +  return readable;
> +}
> +
> +// Streams spec, 3.2.4.5. pipeTo(dest, { preventClose, preventAbort, preventCancel } = {})
> +// TODO: Unimplemented since spec is not complete yet.

Spec seems complete now, but needs WritableStreams to be implemented.

We should throw NYI here instead of noop.

@@ +126,5 @@
> +
> +  // Step 3: Return ! CreateArrayFromList(branches).
> +  assert(branches.length == 2, "ReadableStreamTee() must return two branches.");
> +
> +  // TODO: ReadableStreamTee returns an Array.

ReadableStreamTee already returns an Array.

@@ +382,5 @@
> +    teeState.closedOrErrored = true;
> +  });
> +
> +  // Step 22: Return « branch1, branch2 ».
> +  // Changed to return an array that ReadableStream_tee can just return.

Is this comment leftover from an older spec?

@@ +395,5 @@
> +  // Step 1: Assert: ! IsReadableStreamBYOBReader(stream@[[reader]]) is true.
> +  let reader = UnsafeGetObjectFromReservedSlot(stream, READABLESTREAM_SLOT_READER);
> +  assert(IsReadableStreamBYOBReader(reader),
> +         "ReadableStreamAddReadIntoRequest() must operate on a ReadableStreamBYOBReader");
> +

Current spec contains:

2. Assert: stream.[[state]] is "readable" or "closed".

@@ +421,5 @@
> +  // Step 1: Assert: ! IsReadableStreamDefaultReader(stream@[[reader]]) is true.
> +  let reader = UnsafeGetObjectFromReservedSlot(stream, READABLESTREAM_SLOT_READER);
> +  assert(IsReadableStreamDefaultReader(reader),
> +         "ReadableStreamAddReadRequest() must operate on a ReadableStreamDefaultReader");
> +

Current spec contains:

2. Assert: stream.[[state]] is "readable".

@@ +432,5 @@
> +
> +  // Step 4: Append readRequest as the last element of stream@[[reader]]@[[readIntoRequests]].
> +  let readIntoRequests = UnsafeGetObjectFromReservedSlot(reader,
> +                                                         READABLESTREAMREADER_SLOT_REQUESTS);
> +  ArrayStaticPush(readIntoRequests, readIntoRequest);

The spec says append to [[reader]].[[readRequests]], not [[readIntoRequests]]. Please fix the comment and naming.

The actual code is okay because the slot number is the same between the two reader types.

::: js/src/jsapi.cpp
@@ +4932,2 @@
>  
> +    MOZ_ASSERT_IF(onResolvedObj, IsCallable(onResolvedObj));

I don't understand why these got changed to allow null.

::: js/src/vm/GlobalObject.h
@@ +654,5 @@
>      }
>  
> +    static JSFunction*
> +    getOrCreateReadableStreamDefaultReaderConstructor(JSContext* cx,
> +                                                      Handle<GlobalObject*> global) {

This method isn't used anywhere.

@@ +664,5 @@
> +    }
> +
> +    static JSFunction*
> +    getOrCreateReadableStreamDefaultControllerConstructor(JSContext* cx,
> +                                                          Handle<GlobalObject*> global) {

This isn't used anywhere either.

::: js/src/vm/SelfHosting.cpp
@@ +2302,5 @@
> +    void* contents = JS_StealArrayBufferContents(cx, buffer);
> +    if (!contents)
> +        return false;
> +
> +    RootedObject transferredBuffer(cx, JS_NewArrayBufferWithContents(cx, size, contents));

|realm| isn't used. You should enter it before creating the new, transferred buffer. This, as is, is always transferring intra to whatever cx's global is.
Comment on attachment 8836928 [details] [diff] [review]
ReadableStream implementation

Review of attachment 8836928 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look at all the .ini's in WPT. Just gonna rubber-stamp those changes.

Looks great! Straight-forward implementation of the spec. The spec's been updated in the meantime with some new behavior. The two main ones that need updating before r+:

 1. [[totalQueueSize]]
 2. Handling of "errored" and "closed" states in the GetDesiredSize functions.

::: js/src/builtin/Stream.js
@@ +442,5 @@
> +// Streams spec, 3.4.3. ReadableStreamCancel ( stream, reason )
> +function ReadableStreamCancel(stream, reason) {
> +  // Step 1: Assert: stream is not undefined.
> +  assert(IsReadableStream(stream),
> +         "ReadableStreamCancel must operate on a ReadableStream");

This step is no longer there in the current spec fwiw.

@@ +549,5 @@
> +
> +  // Step 5: Let reader be stream@[[reader]].
> +  let reader = UnsafeGetReservedSlot(stream, READABLESTREAM_SLOT_READER);
> +
> +  // Step 6: If reader is undefined, return undefined.

Spec change to just "return"

@@ +566,5 @@
> +      RejectPromise(readRequest.promise, e);
> +    }
> +
> +    // Step b: Set reader@[[readRequests]] to a new empty List.
> +    UnsafeSetReservedSlot(reader, READABLESTREAMREADER_SLOT_REQUESTS, new List());

For unobservable lists, here and elsewhere, could we use plain arrays? We use the static array methods on them.

@@ +648,5 @@
> +  assert(IsReadableStream(stream),
> +         "ReadableStreamGetNumReadIntoRequests must operate on a ReadableStream");
> +
> +  let reader =
> +    UnsafeGetObjectFromReservedSlot(stream, READABLESTREAM_SLOT_READER);

Nit: line can fit.

@@ +802,5 @@
> +    ThrowTypeError(JSMSG_INCOMPATIBLE_PROTO, "ReadableStreamDefaultReader",
> +                   "releaseLock", typeof this);
> +  }
> +
> +  // Step 2: If this@[[ownerReadableStream]] is undefined, return undefined.

Spec change to just "return"

@@ +810,5 @@
> +    return undefined;
> +
> +  // Step 3: If this@[[readRequests]] is not empty, throw a TypeError exception.
> +  let readRequests = UnsafeGetObjectFromReservedSlot(this, READABLESTREAMREADER_SLOT_REQUESTS);
> +  if (readRequests.length > 0)

Prefer !== 0

@@ +1202,5 @@
> +      // Step i: If stream@[[state]] is "readable", perform
> +      //         ! ReadableStreamDefaultControllerError(controller, r).
> +      let state = UnsafeGetInt32FromReservedSlot(stream, READABLESTREAM_SLOT_STATE);
> +      if (state & READABLESTREAM_STATE_READABLE)
> +        ReadableStreamDefaultControllerError(controller, r);

In the current spec this got refactored to ReadableStreamDefaultControllerErrorIfNeeded.

@@ +1539,5 @@
> +        // Step 1: If stream.[[state]] is "readable", perform
> +        //         ! ReadableStreamDefaultControllerError(controller, chunkSize.[[Value]]).
> +        let state = UnsafeGetInt32FromReservedSlot(stream, READABLESTREAM_SLOT_STATE);
> +        if (state & READABLESTREAM_STATE_READABLE)
> +          ReadableStreamDefaultControllerError(controller, e);

In the current spec this got refactored to ReadableStreamDefaultControllerErrorIfNeeded.

@@ +1548,5 @@
> +      }
> +    }
> +
> +    // Step c: Let enqueueResult be
> +    //         ! EnqueueValueWithSize(controller.[[queue]], chunk, chunkSize).

Comment fix: the spec passes controller directly to EnqueueValueWithSize.

@@ +1564,5 @@
> +      // Step i: If stream.[[state]] is "readable", perform
> +      //         ! ReadableStreamDefaultControllerError(controller, enqueueResult.[[Value]]).
> +      let state = UnsafeGetInt32FromReservedSlot(stream, READABLESTREAM_SLOT_STATE);
> +      if (state & READABLESTREAM_STATE_READABLE)
> +        ReadableStreamDefaultControllerError(controller, e);

In the current spec this got refactored to ReadableStreamDefaultControllerErrorIfNeeded.

@@ +1596,5 @@
> +         "stream should be in the readable state");
> +
> +  // Step 3: Set controller.[[queue]] to a new empty List.
> +  let queue = new List();
> +  queue.size = 0;

Refactored to ResetQueue.

@@ +1603,5 @@
> +  // Step 4: Perform ! ReadableStreamError(stream, e).
> +  ReadableStreamError(stream, e);
> +}
> +
> +// Streams spec, 3.9.7. ReadableStreamDefaultControllerGetDesiredSize ( controller )

This got bumped to 3.9.8 due to ReadableStreamDefaultControllerErrorIfNeeded.

@@ +1612,5 @@
> +  // Step 1: Let queueSize be ! GetTotalQueueSize(controller.[[queue]]).
> +  let queue =
> +    UnsafeGetObjectFromReservedSlot(controller, READABLESTREAMDEFAULTCONTROLLER_SLOT_QUEUE);
> +  let queueSize = GetTotalQueueSize(queue);
> +

There are error cases now in the spec in steps 2-4, and [[queueTotalSize]] is now tracked as an internal slot even in the default controller.

@@ +1659,5 @@
> +  // Step 7: Set this.[[queue]] to a new empty List.
> +  UnsafeSetReservedSlot(this, READABLEBYTESTREAMCONTROLLER_SLOT_QUEUE, new List());
> +
> +  // Step 8: Set this.[[totalQueuedBytes]] to 0.
> +  UnsafeSetReservedSlot(this, READABLEBYTESTREAMCONTROLLER_SLOT_TOTAL_QUEUED_BYTES, 0);

7-8 got refactored to ResetQueue

@@ +1677,5 @@
> +
> +  // Step 12: If autoAllocateChunkSize is not undefined,
> +  if (autoAllocateChunkSize !== undefined) {
> +    // Step a: Set autoAllocateChunkSize to ? ToInteger(autoAllocateChunkSize).
> +    autoAllocateChunkSize = ToInteger(autoAllocateChunkSize);

There is no longer a ToInteger step, just:

If ! IsInteger(autoAllocateChunkSize) is false, or if autoAllocateChunkSize ≤ 0, throw a RangeError exception.

@@ +1745,5 @@
> +  let byobRequest =
> +    UnsafeGetReservedSlot(this, READABLEBYTESTREAMCONTROLLER_SLOT_BYOB_REQUEST);
> +  let pendingPullIntos =
> +    UnsafeGetObjectFromReservedSlot(this, READABLEBYTESTREAMCONTROLLER_SLOT_PENDING_PULL_INTOS);
> +  if (byobRequest === undefined && pendingPullIntos.length > 0) {

Nit: prefer !== 0

@@ +1799,5 @@
> +
> +  // Step 3: If this.[[controlledReadableStream]].[[state]] is not "readable",
> +  //         throw a TypeError exception.
> +  let stream =
> +    UnsafeGetObjectFromReservedSlot(this, READABLEBYTESTREAMCONTROLLER_SLOT_CONTROLLED_READABLE_STREAM);

Line too long?

@@ +1873,5 @@
> +
> +  // Step 1: If this.[[pendingPullIntos]] is not empty,
> +  let pendingPullIntos =
> +    UnsafeGetObjectFromReservedSlot(this, READABLEBYTESTREAMCONTROLLER_SLOT_PENDING_PULL_INTOS);
> +  if (pendingPullIntos.length > 0) {

Prefer !== 0

@@ +1884,5 @@
> +  // Step 2: Set this.[[queue]] to a new empty List.
> +  UnsafeSetReservedSlot(this, READABLEBYTESTREAMCONTROLLER_SLOT_QUEUE, new List());
> +
> +  // Step 3: Set this.[[totalQueuedBytes]] to 0.
> +  UnsafeSetReservedSlot(this, READABLEBYTESTREAMCONTROLLER_SLOT_TOTAL_QUEUED_BYTES, 0);

2-3 got refactored to ResetQueue

@@ +1905,5 @@
> +  assert(ReadableStreamHasDefaultReader(stream), "Stream should have a default reader");
> +
> +  // Step 3: If this.[[totalQueuedBytes]] > 0,
> +  let totalQueuedBytes =
> +      UnsafeGetInt32FromReservedSlot(this, READABLEBYTESTREAMCONTROLLER_SLOT_TOTAL_QUEUED_BYTES);

[[totalQueueBytes]] renamed to [[totalQueueSize]]

@@ +1924,5 @@
> +
> +    // Step 3.e: Perform ! ReadableByteStreamControllerHandleQueueDrain(this).
> +    ReadableByteStreamControllerHandleQueueDrain(this);
> +
> +    // Step 3.f: Let view be ! Construct(<a idl>%Uint8Array%</a>, « entry.[[buffer]],

Nit: Remove markup here and below.

@@ +1966,5 @@
> +      bytesFilled: 0,
> +      elementSize: 1,
> +      ctor: GetBuiltinConstructor('Uint8Array'),
> +      readerType: "default"
> +    };

Refactor using Record.

@@ +2082,5 @@
> +  let shouldPull = ReadableByteStreamControllerShouldCallPull(controller);
> +
> +  // Step 2: If shouldPull is false, return undefined.
> +  if (!shouldPull)
> +    return undefined;

Spec changed to just "return"

@@ +2092,5 @@
> +    // Step a: Set controller.[[pullAgain]] to true.
> +    UnsafeSetReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_PULL_AGAIN, true);
> +
> +    // Step b: Return undefined.
> +    return undefined;

Spec changed to just "return"

@@ +2096,5 @@
> +    return undefined;
> +  }
> +
> +  // Step 4: Set controller.[[pullAgain]] to false.
> +  UnsafeSetReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_PULL_AGAIN, false);

This step is now an assert instead of a set.

@@ +2137,5 @@
> +      ReadableByteStreamControllerError(controller, e);
> +  });
> +
> +  // Step 9: Return undefined.
> +  return undefined;

This step is removed.

@@ +2172,5 @@
> +  assert(UnsafeGetInt32FromReservedSlot(stream, READABLESTREAM_SLOT_STATE) &
> +         READABLESTREAM_STATE_READABLE,
> +         "controller's stream should be in the readable state");
> +
> +  // Step 4: If controller.[[totalQueuedBytes]] > 0,

[[queueTotalSize]]

@@ +2186,5 @@
> +
> +  // Step 5: If controller.[[pendingPullIntos]] is not empty,
> +  let pendingPullIntos =
> +    UnsafeGetObjectFromReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_PENDING_PULL_INTOS);
> +  if (pendingPullIntos.length > 0) {

Prefer !== 0

@@ +2282,5 @@
> +         "must operate on ReadableByteStreamController");
> +
> +  // Step 1: Let stream be controller.[[controlledReadableStream]].
> +  let stream =
> +    UnsafeGetObjectFromReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_CONTROLLED_READABLE_STREAM);

Nit: line too long

@@ +2303,5 @@
> +
> +  // Step 6: Let byteLength be chunk.[[ByteLength]].
> +  let byteLength = ArrayBufferByteLength(buffer);
> +
> +  // Step 7: Let transferredBuffer be ! SameRealmTransfer(buffer).

Comment fix: Transfer(buffer, the current Realm Record).

@@ +2323,5 @@
> +
> +    // Step b: Otherwise,
> +    else {
> +      // Step i: Assert: controller.[[queue]] is empty.
> +      assert(UnsafeGetObjectFromReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_QUEUE).length === 0,

Nit: line too long

@@ +2339,5 @@
> +
> +  // Step 9: Otherwise,
> +  else {
> +    // Step a: If ! ReadableStreamHasBYOBReader(stream) is true,
> +    if (ReadableStreamHasBYOBReader(stream)) {

Weird nesting here. |else if (Readable...) {| would be nicer.

@@ +2387,5 @@
> +    __proto__: null,
> +    buffer,
> +    byteOffset,
> +    byteLength
> +  };

Refactor using Record

@@ +2390,5 @@
> +    byteLength
> +  };
> +  ArrayStaticPush(queue, record);
> +
> +  // Step 2: Add byteLength to controller.[[totalQueuedBytes]].

[[queueTotalSize]]

@@ +2416,5 @@
> +  // Step 3: Perform ! ReadableByteStreamControllerClearPendingPullIntos(controller).
> +  ReadableByteStreamControllerClearPendingPullIntos(controller);
> +
> +  // Step 4: Let controller.[[queue]] be a new empty List.
> +  UnsafeSetReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_QUEUE, new List());

Refactored into ResetQueue

@@ +2458,5 @@
> +  let currentAlignedBytes = pullIntoDescriptor.bytesFilled -
> +                            (pullIntoDescriptor.bytesFilled % elementSize);
> +
> +  // Step 3: Let maxBytesToCopy be min(controller.[[totalQueuedBytes]],
> +  //         pullIntoDescriptor.[[byteLength]] − pullIntoDescriptor.[[bytesFilled]]).

[[queueTotalSize]]

@@ +2462,5 @@
> +  //         pullIntoDescriptor.[[byteLength]] − pullIntoDescriptor.[[bytesFilled]]).
> +  let totalQueuedBytes =
> +    UnsafeGetInt32FromReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_TOTAL_QUEUED_BYTES);
> +  let maxBytesToCopy = std_Math_min(totalQueuedBytes,
> +    pullIntoDescriptor.byteLength - pullIntoDescriptor.bytesFilled);

Nit: line up one past (

@@ +2536,5 @@
> +      headOfQueue.byteLength -= bytesToCopy;
> +    }
> +
> +    // Step g: Set controller.[[totalQueuedBytes]] to
> +    //         controller.[[totalQueuedBytes]] − bytesToCopy.

[[totalQueueSize]]

@@ +2578,5 @@
> +function ReadableByteStreamControllerGetDesiredSize(controller) {
> +  assert(IsReadableByteStreamController(controller),
> +         "must operate on ReadableByteStreamController");
> +
> +  // Step 1: Return controller.[[strategyHWM]] − controller.[[totalQueuedBytes]].

Like the default controller one, there error and closed cases are now handled here.

@@ +2600,5 @@
> +         READABLESTREAM_STATE_READABLE,
> +         "controller's stream must be in the readable state");
> +
> +  // Step 2: If controller.[[totalQueuedBytes]] is 0 and
> +  //         controller.[[closeRequested]] is true,

[[queueTotalSize]]

@@ +2653,5 @@
> +  // Step 2: Repeat the following steps while controller.[[pendingPullIntos]]
> +  //         is not empty,
> +  let pendingPullIntos =
> +    UnsafeGetObjectFromReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_PENDING_PULL_INTOS);
> +  while (pendingPullIntos.length > 0) {

Prefer !== 0

@@ +2654,5 @@
> +  //         is not empty,
> +  let pendingPullIntos =
> +    UnsafeGetObjectFromReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_PENDING_PULL_INTOS);
> +  while (pendingPullIntos.length > 0) {
> +    // Step a: If controller.[[totalQueuedBytes]] is 0, return.

[[totalQueueSize]]

@@ +2732,5 @@
> +    bytesFilled: 0,
> +    elementSize,
> +    ctor,
> +    readerType: "byob"
> +  };

Refactor using Record

@@ +2737,5 @@
> +
> +  // Step 6: If controller.[[pendingPullIntos]] is not empty,
> +  let pendingPullIntos =
> +    UnsafeGetObjectFromReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_PENDING_PULL_INTOS);
> +  if (pendingPullIntos.length > 0) {

Prefer !== 0

@@ +2762,5 @@
> +    //         ! CreateIterResultObject(emptyView, true).
> +    return CreatePromiseResolvedWith({value: emptyView, done: true});
> +  }
> +
> +  // Step 8: If controller.[[totalQueuedBytes]] > 0,

[[queueTotalSize]]

@@ +2801,5 @@
> +  }
> +
> +  // Step 9: Set pullIntoDescriptor.[[buffer]] to
> +  //         ! SameRealmTransfer(pullIntoDescriptor.[[buffer]]).
> +  pullIntoDescriptor.buffer = SameRealmTransfer(pullIntoDescriptor.buffer);

Wait, what's the point of this step? To neuter all views created previously on the underlying buffer?

@@ +2912,5 @@
> +
> +    // Step c: Perform ! ReadableByteStreamControllerEnqueueChunkToQueue(controller,
> +    //                                                                   remainder, 0,
> +    //                                                                   remainder.[[ByteLength]]).
> +    // Note: `remainderSize` is equivalent to remainder.[[ByteLength]].

Huh. Wonder why spec uses remainder.[[ByteLength]].

@@ +2948,5 @@
> +  let firstDescriptor = pendingPullIntos[0];
> +
> +  // Step 2: Let stream be controller.[[controlledReadableStream]].
> +  let stream =
> +    UnsafeGetObjectFromReservedSlot(controller, READABLEBYTESTREAMCONTROLLER_SLOT_CONTROLLED_READABLE_STREAM);

Nit: line too long

@@ +3097,5 @@
> +  // Step 1: Return 1.
> +  return 1;
> +}
> +
> +// Streams spec, 6.3.1. DequeueValue ( queue )

All the queue-with-sizes operations in the spec got changed to take containers which contain [[queue]] and [[queueTotalSize]] internal slots. These should change accordingly.

@@ +3135,5 @@
> +  assert(!Number_isNaN(queue.size), "Invalid queue size");
> +  ArrayStaticPush(queue, {value, size});
> +}
> +
> +// Streams spec, 6.3.3. GetTotalQueueSize ( queue )

This is now gone and are tracked by the containers themselves.

@@ +3233,5 @@
> +    return CreatePromiseRejectedWith(e);
> +  }
> +
> +  // Step 9: Otherwise, return a new promise resolved with returnValue.[[Value]].
> +  return CreatePromiseResolvedWith(returnValue);

This operation is now refactored to use InvokeOrNoop.

@@ +3265,5 @@
> +  // Step 2: Let highWaterMark be ? ValidateAndNormalizeHighWaterMark(highWaterMark).
> +  highWaterMark = ValidateAndNormalizeHighWaterMark(highWaterMark);
> +
> +  // Step 3: Return Record {[[size]]: size, [[highWaterMark]]: highWaterMark}.
> +  return {size, highWaterMark};

Do you wanna use Record here? What is the actual danger of having a non-null prototype for these internal Records in actual implementation?

::: js/src/builtin/StreamDefines.h
@@ +31,5 @@
> +#define READABLESTREAMDEFAULTCONTROLLER_SLOT_QUEUE                      4
> +#define READABLESTREAMDEFAULTCONTROLLER_SLOT_STARTED                    5
> +#define READABLESTREAMDEFAULTCONTROLLER_SLOT_STRATEGY_HWM               6
> +#define READABLESTREAMDEFAULTCONTROLLER_SLOT_STRATEGY_SIZE              7
> +#define READABLESTREAMDEFAULTCONTROLLER_SLOT_UNDERLYING_SOURCE          8

Needs [[queueTotalSize]].

@@ +45,5 @@
> +#define READABLEBYTESTREAMCONTROLLER_SLOT_QUEUE                      7
> +#define READABLEBYTESTREAMCONTROLLER_SLOT_STARTED                    8
> +#define READABLEBYTESTREAMCONTROLLER_SLOT_STRATEGY_HWM               9
> +#define READABLEBYTESTREAMCONTROLLER_SLOT_TOTAL_QUEUED_BYTES        10
> +#define READABLEBYTESTREAMCONTROLLER_SLOT_UNDERLYING_BYTE_SOURCE    11

Pretty sure [[totalQueuedBytes]] got renamed to [[queueTotalSize]].
Attachment #8836928 - Flags: review?(shu)
(In reply to Shu-yu Guo [:shu] from comment #16)

First, some comments on things I *didn't* change. I'll attach a couple of patches next.

> @@ +566,5 @@
> > +      RejectPromise(readRequest.promise, e);
> > +    }
> > +
> > +    // Step b: Set reader@[[readRequests]] to a new empty List.
> > +    UnsafeSetReservedSlot(reader, READABLESTREAMREADER_SLOT_REQUESTS, new List());
> 
> For unobservable lists, here and elsewhere, could we use plain arrays? We
> use the static array methods on them.

No, because push and shift use [[set]] operations, which would trigger accessors on Array.prototype - that's why List has a null __proto__. I could change the queue to an array and introduce macros for adding, peeking, and removing items, or I could change to using a linked list. Without any clever internal optimizations for queue usage of arrays like JSC has[1], linked lists are probably better.

[1] https://mail.mozilla.org/pipermail/es-discuss/2017-February/047766.html



> 
> @@ +1966,5 @@
> > +      bytesFilled: 0,
> > +      elementSize: 1,
> > +      ctor: GetBuiltinConstructor('Uint8Array'),
> > +      readerType: "default"
> > +    };
> 
> Refactor using Record.

I find using records somewhat annoying, tbh. It's much nicer to just be able to create object inline. The __proto__: null is basically a precaution against accidentally setting fields that you shouldn't have, and doesn't buy us all that much.

One thing I could do is introduce a RECORD macro that seals the given object in debug builds. Not sure it's worth it, though.

> 
> @@ +2801,5 @@
> > +  }
> > +
> > +  // Step 9: Set pullIntoDescriptor.[[buffer]] to
> > +  //         ! SameRealmTransfer(pullIntoDescriptor.[[buffer]]).
> > +  pullIntoDescriptor.buffer = SameRealmTransfer(pullIntoDescriptor.buffer);
> 
> Wait, what's the point of this step? To neuter all views created previously
> on the underlying buffer?

Exactly. The idea is that the implementation is free to fill the buffer in whichever way and whenever it wants to, including in a parallel non-JS thread, without any of that being observable.

> 
> @@ +3265,5 @@
> > +  // Step 2: Let highWaterMark be ? ValidateAndNormalizeHighWaterMark(highWaterMark).
> > +  highWaterMark = ValidateAndNormalizeHighWaterMark(highWaterMark);
> > +
> > +  // Step 3: Return Record {[[size]]: size, [[highWaterMark]]: highWaterMark}.
> > +  return {size, highWaterMark};
> 
> Do you wanna use Record here? What is the actual danger of having a non-null
> prototype for these internal Records in actual implementation?

See the above comment on Records. I can add __proto__: null here, remove it everywhere else, introduce the RECORD macro thing, or, if I must, change all the things to Record.
I tried splitting things up into patches that consist of either just nits or just more substantial changes. I succeeded only partially, but this patch at least really does just address nits or removes unused code.
Attachment #8844042 - Flags: review?(shu)
This unfortunately contains quite a bit of silly renaming which I didn't properly separate from the more substantial changes. Sorry about that.
Attachment #8844043 - Flags: review?(shu)
This is an actual follow-up: I combined all boolean flags on the ReadableStream controllers into a single slot and introduced macros for getting/setting them. These objects are unfortunately quite large as it is, so there's no need to have them be larger than necessary. (Though it only actually makes a difference for default controllers, as byte stream controllers fall into the OBJECT_12 bucket no matter what.)
Attachment #8844044 - Flags: review?(shu)
Comment on attachment 8844042 [details] [diff] [review]
Part 2 follow-up: nits (will be merge into part 2)

Review of attachment 8844042 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Stream.cpp
@@ +68,5 @@
>  static const JSFunctionSpec ReadableStream_methods[] = {
>      JS_SELF_HOSTED_FN("cancel", "ReadableStream_cancel", 1, 0),
>      JS_SELF_HOSTED_FN("getReader", "ReadableStream_getReader", 1, 0),
> +     JS_SELF_HOSTED_FN("pipeThrough", "ReadableStream_pipeThrough", 2, 0),
> +     JS_SELF_HOSTED_FN("pipeTo", "ReadableStream_pipeTo", 2, 0),

Indentation got messed up
Attachment #8844042 - Flags: review?(shu) → review+
(In reply to Till Schneidereit [till] from comment #17)
> (In reply to Shu-yu Guo [:shu] from comment #16)
> 
> First, some comments on things I *didn't* change. I'll attach a couple of
> patches next.
> 
> > @@ +566,5 @@
> > > +      RejectPromise(readRequest.promise, e);
> > > +    }
> > > +
> > > +    // Step b: Set reader@[[readRequests]] to a new empty List.
> > > +    UnsafeSetReservedSlot(reader, READABLESTREAMREADER_SLOT_REQUESTS, new List());
> > 
> > For unobservable lists, here and elsewhere, could we use plain arrays? We
> > use the static array methods on them.
> 
> No, because push and shift use [[set]] operations, which would trigger
> accessors on Array.prototype - that's why List has a null __proto__. I could
> change the queue to an array and introduce macros for adding, peeking, and
> removing items, or I could change to using a linked list. Without any clever
> internal optimizations for queue usage of arrays like JSC has[1], linked
> lists are probably better.
> 
> [1] https://mail.mozilla.org/pipermail/es-discuss/2017-February/047766.html
> 
> 

I... sigh, yeah, okay.

> 
> > 
> > @@ +1966,5 @@
> > > +      bytesFilled: 0,
> > > +      elementSize: 1,
> > > +      ctor: GetBuiltinConstructor('Uint8Array'),
> > > +      readerType: "default"
> > > +    };
> > 
> > Refactor using Record.
> 
> I find using records somewhat annoying, tbh. It's much nicer to just be able
> to create object inline. The __proto__: null is basically a precaution
> against accidentally setting fields that you shouldn't have, and doesn't buy
> us all that much.
> 
> One thing I could do is introduce a RECORD macro that seals the given object
> in debug builds. Not sure it's worth it, though.
> 

I'm fond of __proto__: null in object literals -- I had originally asked for Record usage for consistency with List. Let's just do __proto__: null.
Comment on attachment 8844043 [details] [diff] [review]
Part 2 follow-up: more substantial stuff (will be merge into part 2)

Review of attachment 8844043 [details] [diff] [review]:
-----------------------------------------------------------------

Glad the constant names got a little shorter with the rename.

::: js/src/builtin/Stream.js
@@ +1509,5 @@
>      if (strategySize !== undefined) {
>        // Step i: Set chunkSize to Call(stream.[[strategySize]], undefined, chunk).
>        try {
>          chunkSize = callContentFunction(strategySize, undefined, chunk);
> +      } catch(e) {

nit: catch (e)

@@ +1521,1 @@
>          throw(e);

nit: throw e

@@ +3140,2 @@
>  
> +// Streams spec, 6.3.3. ResetQueue ( container ) nothrow

ResetQueue is 6.3.4
Attachment #8844043 - Flags: review?(shu) → review+
Comment on attachment 8844044 [details] [diff] [review]
Part 2 follow-up: stream controller flags optimizations (will be merge into part 2)

Review of attachment 8844044 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: js/src/builtin/Stream.js
@@ +1631,5 @@
>    ResetQueue(this);
>  
>    // Step 8: Set this.[[started]] and this.[[closeRequested]] to false.
> +  UNSET_READABLESTREAMCONTROLLER_FLAGS(this, READABLESTREAMCONTROLLER_FLAG_STARTED |
> +                                             READABLESTREAMCONTROLLER_FLAG_CLOSE_REQUESTED);

You have |UnsafeSetReservedSlot(this, READABLESTREAMCONTROLLER_SLOT_FLAGS, 0);| in step 5 already. This step shouldn't be needed.

@@ +2730,5 @@
>      }
>  
>      // Step b: If controller.[[closeRequested]] is true,
> +    if (READABLESTREAMCONTROLLER_FLAGS(controller) &
> +        READABLESTREAMCONTROLLER_FLAG_CLOSE_REQUESTED) {

Could this fit on one line?
Attachment #8844044 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #24)
> Comment on attachment 8844044 [details] [diff] [review]
> Part 2 follow-up: stream controller flags optimizations (will be merge into
> part 2)
> 
> Review of attachment 8844044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.
> 
> ::: js/src/builtin/Stream.js
> @@ +1631,5 @@
> >    ResetQueue(this);
> >  
> >    // Step 8: Set this.[[started]] and this.[[closeRequested]] to false.
> > +  UNSET_READABLESTREAMCONTROLLER_FLAGS(this, READABLESTREAMCONTROLLER_FLAG_STARTED |
> > +                                             READABLESTREAMCONTROLLER_FLAG_CLOSE_REQUESTED);
> 
> You have |UnsafeSetReservedSlot(this, READABLESTREAMCONTROLLER_SLOT_FLAGS,
> 0);| in step 5 already. This step shouldn't be needed.

The spec says to do this, but you're right, it shouldn't be needed. I changed this to an assert instead so we don't get caught off-guard if it ever accidentally does become required.
> 
> @@ +2730,5 @@
> >      }
> >  
> >      // Step b: If controller.[[closeRequested]] is true,
> > +    if (READABLESTREAMCONTROLLER_FLAGS(controller) &
> > +        READABLESTREAMCONTROLLER_FLAG_CLOSE_REQUESTED) {
> 
> Could this fit on one line?

No, it'd be 102 chars.
Roll-up patch for posterity.
Attachment #8836928 - Attachment is obsolete: true
Attachment #8844042 - Attachment is obsolete: true
Attachment #8844043 - Attachment is obsolete: true
Attachment #8844044 - Attachment is obsolete: true
Attachment #8845428 - Flags: review+
The Records really aren't needed here, so let's not allocate them.
Attachment #8845452 - Flags: review?(shu)
Attachment #8845452 - Flags: review?(shu) → review+
Depends on: 1357958
This is a complete re-implementation of ReadableStream compared to the last patch - in C++ instead of JS.

Two reasons for doing this:
1. It became more and more clear that we'd never get decent performance with the JS version, because pretty much every single function needed to do one or more VM calls without any real possibility of inlining them.
2. Interacting with streams via JSAPI became extremely unwieldy, in particular once I began adding support for ReadableStreams with embedding-provided sources, where the JS engine needs to invoke embedding-provided callbacks to request more data and all that. I'll attach a patch with just that next.

I tried very hard to make the implementation easy to follow. Where possible I extracted common patterns into small helper functions, collected at the beginning of the file. All Records have been turned into NativeObject subclasses with the required amount of fixed slots. Where applicable I hid the slot accesses behind methods on the class. That's not always possible - in particular for readers and controllers, because for those different classes partly share the same slot layouts. (I guess I could've introduced a common parent class?)

Apart from the helper functions, all code is in spec-order, and any omitted functions are marked as such, with explanations.
Attachment #8845428 - Attachment is obsolete: true
Attachment #8845452 - Attachment is obsolete: true
Attachment #8876376 - Flags: review?(shu)
This adds a ton of JSAPI functions for creating and querying the state of ReadableStreams, and support for creating ReadableStream instances whose source is supplied by the embedding.

Asking for feedback instead of review because so far this only underwent somewhat lightweight testing. Baku graciously offered to help out with writing tests, which I'm sure will uncover issues with the implementation.

I'll attach a roll-up patch for testing next.
Attachment #8876377 - Flags: feedback?(shu)
Attachment #8876377 - Flags: feedback?(amarchesini)
Attached patch Rollup patch for testing (obsolete) — Splinter Review
This applies to current inbound. All tests for handling of external, embedding-provided sources for ReadableStream can be found in js/src/jsapi-tests/testReadableStream.cpp
Attachment #8876379 - Flags: feedback?(amarchesini)
Updated patch with a few bugfixes applied.
Attachment #8876376 - Attachment is obsolete: true
Attachment #8876376 - Flags: review?(shu)
Attachment #8877793 - Flags: review?(shu)
Attached patch Rollup patch for testing, v2 (obsolete) — Splinter Review
New version, containing some bugfixes and the following JSAPI changes:
- Added ReadableStreamFinalizeCallback, invoked upon finalization of a ReadableStream (or, more precisely, a ReadableByteStreamController, but that shouldn't matter for the embedding) with an external underlying source. The source is passed as the sole argument.
- Changed WriteIntoReadRequestBufferCallback to take a size_t* bytesWritten instead of a bool* done.
- Moved from all callbacks being passed into JS::NewExternalSourceStreamObject to setting them once per runtime, using JS::SetReadableStreamExternalSourceCallbacks.
- Added a JSContext* parameter to all callbacks except the finalizer one.
Attachment #8876379 - Attachment is obsolete: true
Attachment #8876379 - Flags: feedback?(amarchesini)
Attachment #8877946 - Flags: feedback?(amarchesini)
Attached patch Function needed (obsolete) — Splinter Review
I need these functions:

+extern JS_PUBLIC_API(bool)
+JS::HasReadableStreamExternalSourceCallbacks(JSContext* cx)

+JS_PUBLIC_API(bool)
+JS::ReadableStreamIsDisturbedOrLocked(JSContext* cx, JS::HandleObject streamObj)
Attachment #8878084 - Flags: feedback?(till)
Comment on attachment 8878084 [details] [diff] [review]
Function needed

Review of attachment 8878084 [details] [diff] [review]:
-----------------------------------------------------------------

I folded this into a larger patch, except I introduced a ReadableStreamIsDisturbed that has to be called in addition to the already existing ReadableStreamIsLocked, instead of adding ReadableStreamIsDisturbedOrLocked.
Attachment #8878084 - Flags: feedback?(till) → feedback+
Comment on attachment 8877793 [details] [diff] [review]
Part 2: Implement ReadableStream and associated classes in the JS engine, C++ edition

Review of attachment 8877793 [details] [diff] [review]:
-----------------------------------------------------------------

Posting partial review because I'm switching machines.

::: js/src/builtin/Promise.cpp
@@ +2104,5 @@
>      RootedObject promiseCtor(cx, JS::GetPromiseConstructor(cx));
>      if (!promiseCtor)
>          return nullptr;
>      RootedValue cVal(cx, ObjectValue(*promiseCtor));
> +    JSObject* promise = CommonStaticResolveRejectImpl(cx, cVal, value, RejectMode);

Style nit: Here and below, I prefer

if (!promise)
    return nullptr;
return &promise->as<PromiseObject>();

::: js/src/builtin/Stream.cpp
@@ +73,5 @@
> +    ControllerFlag_PullAgain      = 0x04,
> +    ControllerFlag_CloseRequested = 0x08,
> +    ControllerFlag_TeeBranch      = 0x10,
> +    ControllerFlag_TeeBranch1     = 0x20,
> +    ControllerFlag_TeeBranch2     = 0x40

I'd prefer 1 << 0, 1 << 1, etc.

@@ +108,5 @@
> +
> +static inline uint32_t
> +ControllerFlags(NativeObject* controller)
> +{
> +    return controller->getFixedSlot(ControllerSlot_Flags).toInt32();

MOZ_ASSERT(IsReadableStreamController(controller)); here and in the flags functions below.

@@ +140,5 @@
> +    stream->setFixedSlot(StreamSlot_State, Int32Value(state));
> +}
> +
> +inline bool
> +js::ReadableStream::readable() {

Is the js:: needed? Also { on its own line in this and the getters below.

@@ +161,5 @@
> +}
> +
> +inline static MOZ_MUST_USE ReadableStream*
> +StreamFromController(NativeObject* controller)
> +{

MOZ_ASSERT(IsReadableStreamController(controller));

::: js/src/builtin/Stream.h
@@ +12,5 @@
> +namespace js {
> +
> +class AutoSetNewObjectMetadata;
> +
> +class ReadableStream : public NativeObject {

Style nit: { on own line here and below

@@ +22,5 @@
> +
> +    inline bool readable();
> +    inline bool closed();
> +    inline bool errored();
> +    inline bool disturbed();

These should be const.

@@ +25,5 @@
> +    inline bool errored();
> +    inline bool disturbed();
> +
> +    enum State {
> +         Readable  = 0x1,

I'd prefer 1 << 0, 1 << 1, etc
Attachment #8877793 - Flags: review?(shu)
Comment on attachment 8877793 [details] [diff] [review]
Part 2: Implement ReadableStream and associated classes in the JS engine, C++ edition

Review of attachment 8877793 [details] [diff] [review]:
-----------------------------------------------------------------

General questions:

- Is the only reason that pendingPullIntos is separate from readIntoRequests to handle autoAllocateChunkSize?
- Is a byobRequest always associated with the first pullInto descriptor?

::: js/src/builtin/Stream.cpp
@@ +471,5 @@
> +    {
> +        Flag_ClosedOrErrored = 0x1,
> +        Flag_Canceled1 =       0x2,
> +        Flag_Canceled2 =       0x4,
> +        Flag_CloneForBranch2 = 0x8,

Would prefer 1 << 0, etc.

@@ +548,5 @@
> +        setFixedSlot(Slot_Branch2, ObjectValue(*controller));
> +    }
> +
> +    static TeeState* create(JSContext* cx, Handle<ReadableStream*> stream)
> +   {

Style nit: { indented wrong

@@ +690,5 @@
> +        RootedObject sourceObj(cx, NewBuiltinClassInstance<PlainObject>(cx));
> +        if (!sourceObj)
> +            return false;
> +        underlyingSource = ObjectValue(*sourceObj);
> +    }

I don't understand why underlyingSource becomes a plain object if it is undefined. Step 5 calls GetV, which calls ToObject, which should throw if the underlying source is undefined.

@@ +707,5 @@
> +        return false;
> +
> +    // Step 5: Let type be ? GetV(underlyingSource, "type").
> +    RootedValue typeVal(cx);
> +    if (!underlyingSource.isUndefined()) {

This is always true, because of the above comment. This check shouldn't be needed.

@@ +717,5 @@
> +    RootedString type(cx, ToString<CanGC>(cx, typeVal));
> +    if (!type)
> +        return false;
> +
> +    int32_t strDiff;

More clear as |notByteStream|.

@@ +782,5 @@
> +    //         with a TypeError exception.
> +    if (!Is<ReadableStream>(args.thisv())) {
> +        ReportValueError3(cx, JSMSG_INCOMPATIBLE_PROTO, JSDVG_SEARCH_STACK, args.thisv(),
> +                              nullptr, "cancel", "");
> +        return ReturnPromiseRejectedWithPendingError(cx, args);

I'm not sure how much I like brand checks being async... I guess it's for the sake of uniformity of dealing with errors.

@@ +834,5 @@
> +        if (!mode)
> +            return false;
> +
> +        // Step 4: If mode is "byob", return ? AcquireReadableStreamBYOBReader(this).
> +        int32_t strDiff;

Clearer as notByob

@@ +1003,5 @@
> +        if (!teeState->canceled1()) {
> +            // Step 1: Perform ! ReadableStreamDefaultControllerClose(branch1).
> +            Rooted<ReadableStreamDefaultController*> branch1(cx, teeState->branch1());
> +            if (!ReadableStreamDefaultControllerClose(cx, branch1))
> +                return false;

The spec says ReadableStreamDefaultControllerClose cannot throw. Is it possible to do MOZ_ALWAYS_TRUE?

There are other ! calls throughout the spec. I'll just comment on this one.

@@ +1149,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    Rooted<TeeState*> teeState(cx, TargetFromHandler<TeeState>(args.callee()));
> +    HandleValue reason = args.get(0);
> +
> +    // Step a: If teeState.[[closedOrErrored]] is true, return.

These steps no longer match with spec text.

@@ +1211,5 @@
> +    // Step 12: Perform ! CreateDataProperty(underlyingSource1, "pull", pull).
> +    // Step 13: Perform ! CreateDataProperty(underlyingSource1, "cancel", cancel1).
> +
> +    // Step 14: Let branch1Stream be ! Construct(ReadableStream, underlyingSource1).
> +    RootedValue highWaterMark(cx, NumberValue(1));

As a future refactor hazard I'd prefer highWaterMark be left undefined and have the default value logic in the constructor take care of it.

@@ +1302,5 @@
> +  Rooted<PromiseObject*> promise(cx, PromiseObject::createSkippingExecutor(cx));
> +  if (!promise)
> +      return nullptr;
> +
> +  // Step 4: Let readIntoRequest be Record {[[promise]]: promise}.

Let readRequest

@@ +1344,5 @@
> +
> +    // Step 2: If stream.[[state]] is "closed", return a new promise resolved
> +    //         with undefined.
> +    if (stream->closed())
> +        return PromiseObject::unforgeableResolve(cx, reason);

Should this be unforgeableResolve(cx, UndefinedValue())?

@@ +1526,5 @@
> +    NativeObject* reader = &readerVal.toObject().as<NativeObject>();
> +    MOZ_ASSERT(reader->is<ReadableStreamDefaultReader>() ||
> +               reader->is<ReadableStreamBYOBReader>());
> +    Value readRequests = reader->getFixedSlot(ReaderSlot_Requests);
> +    return readRequests.toObject().as<NativeObject>().getDenseInitializedLength();

So do you ever use the list's .length property?

@@ +1763,5 @@
> +
> +    Rooted<ReadableStreamBYOBReader*> reader(cx);
> +    reader = NewBuiltinClassInstance<ReadableStreamBYOBReader>(cx);
> +    if (!reader)
> +        return nullptr;

This should go below the locked check.

@@ +1883,5 @@
> +    // Step 3: If Type(view) is not Object, return a promise rejected with a
> +    //         TypeError exception.
> +    // Step 4: If view does not have a [[ViewedArrayBuffer]] internal slot,
> +    //         return a promise rejected with a TypeError exception.
> +    if (!viewVal.isObject() || !viewVal.toObject().is<TypedArrayObject>()) {

This should be is<ArrayBufferViewObject>() or JS_IsArrayBufferViewObject(&viewVal.toObject())

@@ +2010,5 @@
> +        //         undefined.
> +        promise = PromiseObject::unforgeableResolve(cx, UndefinedHandleValue);
> +        if (!promise)
> +            return false;
> +        reader->setFixedSlot(ReaderSlot_ClosedPromise, ObjectValue(*promise));

The null check and the setFixedSlot are unnecessary.

@@ +2036,5 @@
> +{
> +    // Step 1: Assert: reader.[[ownerReadableStream]] is not undefined.
> +    Rooted<ReadableStream*> stream(cx, StreamFromReader(reader));
> +
> +    // Step 2: Assert: reader.[[ownerReadableStream]].[[reader]] is not undefined.

Step 2 now says Assert: reader.[[ownerReadableStream]].[[reader]] is reader.

@@ +2043,5 @@
> +    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_READABLESTREAMREADER_RELEASED);
> +    RootedValue exn(cx);
> +    // Not much we can do about uncatchable exception, just bail.
> +    if (!GetAndClearException(cx, &exn))
> +        return false;

I was initially confused by what's going on here. Could use a comment about how it's making a new exception to reject promises with, as whether the owner stream is "readable", a promise will be rejected with the exception.

@@ +2193,5 @@
> +        controller = &controllerObj->as<ReadableStreamDefaultController>();
> +        return ReadableStreamDefaultControllerErrorIfNeeded(cx, controller, args.get(0));
> +    }
> +
> +    // Step iv: If stream.[[state]] is "readable", perform

Do you mean step i of 3.10.3?

@@ +2195,5 @@
> +    }
> +
> +    // Step iv: If stream.[[state]] is "readable", perform
> +    //          ! ReadableByteStreamControllerError(controller, r).
> +    if (StreamFromController(controllerObj)->errored())

readable()?

@@ +2345,5 @@
> +        return true;
> +    }
> +
> +    // Step 2: Return ! ReadableStreamDefaultControllerGetDesiredSize(this).
> +    args.rval().setNumber(ReadableStreamControllerGetDesiredSize(controller, args));

Did you mean GetDesiredSizeUnchecked here? You're doing the errored/closed checks twice. Stuff that slipped through refactoring, maybe.

@@ +2458,5 @@
> +{
> +    Rooted<ReadableStreamDefaultController*> controller(cx);
> +    controller = &args.thisv().toObject().as<ReadableStreamDefaultController>();
> +
> +    // Step 2: If this.[[closeRequested]] is true, throw a TypeError exception.

This step doesn't exist anymore.

@@ +2475,5 @@
> +                                  JSMSG_READABLESTREAMCONTROLLER_NOT_READABLE, "close");
> +        return false;
> +    }
> +
> +    // Step 4: Return ! ReadableStreamDefaultControllerEnqueue(this, chunk).

Copypasta?

@@ +2542,5 @@
> +        return nullptr;
> +
> +    // Step 2 of 3.8.5.1, step 3 of 3.10.5.1:
> +    // Return ! PromiseInvokeOrNoop(this.[[underlying(Byte)Source]],
> +//                                  "cancel", « reason »)

// indented wrong

@@ +2551,5 @@
> +        Rooted<TeeState*> teeState(cx, &underlyingSource.toObject().as<TeeState>());
> +        Rooted<ReadableStreamDefaultController*> defaultController(cx);
> +        defaultController = &controller->as<ReadableStreamDefaultController>();
> +        return ReadableStreamTee_Cancel(cx, teeState, defaultController, reason);
> +    } else {

Style nit: else after return

@@ +2599,5 @@
> +      RootedObject iterResultObj(cx, CreateIterResultObject(cx, chunk, false));
> +      if (!iterResultObj)
> +          return nullptr;
> +      RootedValue iterResult(cx, ObjectValue(*iterResultObj));
> +      return PromiseObject::unforgeableResolve(cx, iterResult);

This whole block is indented wrong.

@@ +2649,5 @@
> +    RootedNativeObject controller(cx, TargetFromHandler<NativeObject>(args.callee()));
> +    HandleValue e = args.get(0);
> +
> +    // Step a: If controller.[[controlledReadableStream]].[[state]] is "readable",
> +    //         perform ! ReadableByteStreamControllerError(controller, e).

The spec factoring here is weird. The byte controller inlines the ErrorIfNeeded logic, and the default controller does not. But the end result is that the Error abstract method is only called if [[state]] is "readable".

@@ +2937,5 @@
> +// Streams spec 3.12.13. ReadableByteStreamControllerGetDesiredSize ( controller )
> +static MOZ_MUST_USE double
> +ReadableStreamControllerGetDesiredSize(NativeObject* controller, const CallArgs& args)
> +{
> +

Nit: extra newline

@@ +3157,5 @@
> +{
> +    // Step 2: Return ! ReadableByteStreamControllerGetDesiredSize(this).
> +    // The implementation for default controllers is identical, so just
> +    // forward to that.
> +    return ReadableStreamDefaultController_desiredSize_impl(cx, args);

Could have ReadableByteStreamController_desiredSize do CallNonGenericMethod directly on ReadableStreamDefaultController_desiredSize_impl.

@@ +3391,5 @@
> +        if (!bufferObj) {
> +            // Not much we can do about uncatchable exception, just bail.
> +            if (!GetAndClearException(cx, &val))
> +                return nullptr;
> +            return PromiseObject::unforgeableReject(cx, val);

Refactor using PromiseRejectedWithPendingException.

@@ +3411,5 @@
> +        if (!pullIntoDescriptor) {
> +            // Not much we can do about uncatchable exception, just bail.
> +            if (!GetAndClearException(cx, &val))
> +                return nullptr;
> +            return PromiseObject::unforgeableReject(cx, val);

Refactor using PromiseRejectedWithPendingException.

@@ +3514,5 @@
> +    return true;
> +}
> +
> +// Streams spec, 3.11.4.1 get view
> +// (Implemented in Stream.js)

Why is this one getter self hosted?

@@ +3557,5 @@
> +{
> +    // Step 1: If ! IsReadableStreamBYOBRequest(this) is false, throw a TypeError
> +    //         exception.
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    return CallNonGenericMethod<Is<ReadableStreamBYOBRequest>, 

Nit: trailing space

@@ +3612,5 @@
> +{
> +    // Step 1: If ! IsReadableStreamBYOBRequest(this) is false, throw a TypeError
> +    //         exception.
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    return CallNonGenericMethod<Is<ReadableStreamBYOBRequest>, 

Nit: trailing space

@@ +3692,5 @@
> +            // Step i: Let e be a new TypeError exception. {
> +            JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
> +                                      JSMSG_READABLEBYTESTREAMCONTROLLER_CLOSE_PENDING_PULL);
> +            RootedValue e(cx);
> +            // Not much we can do about uncatchable exception, just bail.

This comment doesn't make sense.

@@ +3782,5 @@
> +    //                            pullIntoDescriptor.[[buffer]],
> +    //                            pullIntoDescriptor.[[byteOffset]],
> +    //                            bytesFilled / elementSize).
> +    RootedObject ctor(cx, pullIntoDescriptor->ctor());
> +    if (!ctor) {

Why the choice to make ctor nullable and handle the default case here? To make getting the builtin constructor as lazy as possible?

@@ +3942,5 @@
> +
> +    // Step 2: Add byteLength to controller.[[queueTotalSize]].
> +    uint32_t queueTotalSize = controller->getFixedSlot(QueueContainerSlot_TotalSize).toInt32();
> +    controller->setFixedSlot(QueueContainerSlot_TotalSize,
> +                             Int32Value(queueTotalSize + byteLength));

What if this overflows int32?

@@ +3994,5 @@
> +    uint32_t byteLength = pullIntoDescriptor->byteLength();
> +    uint32_t queueTotalSize = controller->getFixedSlot(QueueContainerSlot_TotalSize).toInt32();
> +    uint32_t maxBytesToCopy = queueTotalSize < byteLength - bytesFilled
> +                              ? queueTotalSize
> +                              : byteLength - bytesFilled;

Should actually use Min here.

@@ +4034,5 @@
> +        //                                headOfQueue.[[byteLength]]).
> +        uint32_t byteLength = headOfQueue->byteLength();
> +        uint32_t bytesToCopy = totalBytesToCopyRemaining < byteLength
> +                               ? totalBytesToCopyRemaining
> +                               : byteLength;

Use Min.

@@ +4056,5 @@
> +        if (byteLength == bytesToCopy) {
> +            // Step i: Remove the first element of queue, shifting all other elements
> +            //         downward (so that the second becomes the first, and so on).
> +            headOfQueue = ShiftFromList<ByteStreamChunk>(cx, queue);
> +            if (!headOfQueue)

When would this fail?

@@ +4080,5 @@
> +        //                                                                          bytesToCopy,
> +        //                                                                          pullIntoDescriptor).
> +        ReadableByteStreamControllerFillHeadPullIntoDescriptor(controller, bytesToCopy,
> +                                                               pullIntoDescriptor);
> +        bytesFilled += bytesToCopy;

Since you're optimizing re-getting bytesFilled from the pullIntoDescriptor, an assertion that the two are in sync would be good.

@@ +4202,5 @@
> +                                                                      pullIntoDescriptor))
> +            {
> +                return false;
> +            }
> +        }

Spec question: if the available data is < the descriptor's [[elementSize]], causing ready == false, how do you get the rest of the data out? Assuming there is no more data -- that is, all the underlying source has left is still < the descriptor's [[elementSize]].

@@ +4248,5 @@
> +    //                                           [[ctor]]: ctor,
> +    //                                           [[readerType]]: "byob"}.
> +    bool dummy;
> +    RootedArrayBufferObject buffer(cx, &JS_GetArrayBufferViewBuffer(cx, view, &dummy)
> +                                   ->as<ArrayBufferObject>());

Style nit: line up -> with &JS_GetArrayBufferViewBuffer

@@ +4306,5 @@
> +    }
> +
> +    // Step 8: If controller.[[queueTotalSize]] > 0,
> +    uint32_t queueTotalSize = controller->getFixedSlot(QueueContainerSlot_TotalSize).toInt32();
> +    if (queueTotalSize > 0) {

Spec question: This is the case when the pullIntoDescriptor can be fulfilled immediately. Why do we not need to TransferArrayBuffer in this case? Why do we need to TransferArrayBuffer at all?

@@ +4312,5 @@
> +        //                                                                          pullIntoDescriptor)
> +        //         is true,
> +        bool ready;
> +        if (!ReadableByteStreamControllerFillPullIntoDescriptorFromQueue(cx, controller,
> +                                                                        pullIntoDescriptor, &ready))

Style nit: pullIntoDescriptor 1 space off

@@ +4533,5 @@
> +        }
> +    }
> +
> +    // Step 7: Set pullIntoDescriptor.[[buffer]] to
> +    //         ! TransferArrayBuffer(pullIntoDescriptor.[[buffer]]).

So... the buffer is transferred every time some bytes are filled in a pullIntoDescriptor? But wait -- calling byobRequest.respond always shifts off the first descriptor and fulfills a readInto request. Is this transferring twice? Didn't pullIntoDescriptor.buffer already get transferred when it was enqueued?

@@ +4611,5 @@
> +
> +    // Step 3: If firstDescriptor.[[byteOffset]] + firstDescriptor.[[bytesFilled]]
> +    //         is not view.[[ByteOffset]], throw a RangeError exception.
> +    uint32_t byteOffset = uint32_t(JS_GetArrayBufferViewByteOffset(view));
> +    if (firstDescriptor->byteOffset() + firstDescriptor->bytesFilled() != byteOffset) {

How does the programmer create a new view with the correct offsets? Copying them from byobRequest.view?

::: js/src/builtin/Stream.js
@@ +27,5 @@
> +// Streams spec 6.2.3.1. size ()
> +function CountQueuingStrategy_size() {
> +  // Step 1: Return 1.
> +  return 1;
> +}

Why are these self-hosted?
Attachment #8877793 - Flags: review+
This adds a ton of JSAPI functions and callbacks for working with ReadableStream instances. They generally fall into three categories:

1. General object querying and manipulation functions - i.e. functions for creating streams, querying their state, etc.
2. Functions for interacting with JS-created streams used by native code. Required for example by the Fetch Request object, which allows passing in a ReadableStream to be used as the request body.
3. Functions for creating and interacting with streams where the embedding provides the content. Needed for Fetch Response, which allows returning a ReadableStream as the response body.

I added lots of documentation, so I hope things are reasonably clear.
Attachment #8876377 - Attachment is obsolete: true
Attachment #8878084 - Attachment is obsolete: true
Attachment #8876377 - Flags: feedback?(shu)
Attachment #8876377 - Flags: feedback?(amarchesini)
Attachment #8886594 - Flags: review?(shu)
Attached patch Rollup patch for testing, v3 (obsolete) — Splinter Review
New rollup patch, incorporating review comments for part 2 and the new version of part 3.
Attachment #8877946 - Attachment is obsolete: true
Attachment #8877946 - Flags: feedback?(amarchesini)
Attachment #8886603 - Flags: feedback?(amarchesini)
(In reply to Shu-yu Guo (backlogged) from comment #15 and #16)

The attached patch addressed most of the feedback. See below for things I didn't outright change as requested.

> General questions:

> - Is the only reason that pendingPullIntos is separate from readIntoRequests to handle autoAllocateChunkSize?

That's my understanding, yes. Not sure how to structure this better.

> - Is a byobRequest always associated with the first pullInto descriptor?

Yes, that's correct.


> @@ +690,5 @@
> > +        RootedObject sourceObj(cx, NewBuiltinClassInstance<PlainObject>(cx));
> > +        if (!sourceObj)
> > +            return false;
> > +        underlyingSource = ObjectValue(*sourceObj);
> > +    }

> I don't understand why underlyingSource becomes a plain object if it is undefined. Step 5 calls GetV, which calls ToObject, which should throw if the underlying source is undefined.

The constructor's signature has the first argument as `underlyingSource = {}`, so if it's undefined the GetV has to be done on a plain object. In the face of possible accessors on Object.prototype I don't see how I could do anything different here.


> @@ +782,5 @@
> > +    //         with a TypeError exception.
> > +    if (!Is<ReadableStream>(args.thisv())) {
> > +        ReportValueError3(cx, JSMSG_INCOMPATIBLE_PROTO, JSDVG_SEARCH_STACK, args.thisv(),
> > +                              nullptr, "cancel", "");
> > +        return ReturnPromiseRejectedWithPendingError(cx, args);

> I'm not sure how much I like brand checks being async... I guess it's for the sake of uniformity of dealing with errors.

I concur, they're somewhat weird. Not weird enough for me to reopen a discussion about them, though. (In particular given that there are 3 shipping implementations.)


> @@ +1003,5 @@
> > +        if (!teeState->canceled1()) {
> > +            // Step 1: Perform ! ReadableStreamDefaultControllerClose(branch1).
> > +            Rooted<ReadableStreamDefaultController*> branch1(cx, teeState->branch1());
> > +            if (!ReadableStreamDefaultControllerClose(cx, branch1))
> > +                return false;

> The spec says ReadableStreamDefaultControllerClose cannot throw. Is it possible to do MOZ_ALWAYS_TRUE?
> There are other ! calls throughout the spec. I'll just comment on this one.

The spec ignores OOM errors, but we don't have that luxury.

> @@ +2649,5 @@
> > +    RootedNativeObject controller(cx, TargetFromHandler<NativeObject>(args.callee()));
> > +    HandleValue e = args.get(0);
> > +
> > +    // Step a: If controller.[[controlledReadableStream]].[[state]] is "readable",
> > +    //         perform ! ReadableByteStreamControllerError(controller, e).

> The spec factoring here is weird. The byte controller inlines the ErrorIfNeeded logic, and the default controller does not. But the end result is that the Error abstract method is only called if [[state]] is "readable".

I agree. Will file a spec issue.

> @@ +3692,5 @@
> > +            // Step i: Let e be a new TypeError exception. {
> > +            JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
> > +                                      JSMSG_READABLEBYTESTREAMCONTROLLER_CLOSE_PENDING_PULL);
> > +            RootedValue e(cx);
> > +            // Not much we can do about uncatchable exception, just bail.

> This comment doesn't make sense.

I think it does: throwing an exception can cause an OOM, in which case there really isn't much we can do here. I removed a few of these by using PromiseRejectedWithPendingError and RejectWithPendingError, but that doesn't work for all of them.

> @@ +3782,5 @@
> > +    //                            pullIntoDescriptor.[[buffer]],
> > +    //                            pullIntoDescriptor.[[byteOffset]],
> > +    //                            bytesFilled / elementSize).
> > +    RootedObject ctor(cx, pullIntoDescriptor->ctor());
> > +    if (!ctor) {

> Why the choice to make ctor nullable and handle the default case here? To make getting the builtin constructor as lazy as possible?

Exactly. I guess it doesn't matter much, but I also don't think there's much of a need to change it.

> @@ +4202,5 @@
> > +                                                                      pullIntoDescriptor))
> > +            {
> > +                return false;
> > +            }
> > +        }

> Spec question: if the available data is < the descriptor's [[elementSize]], causing ready == false, how do you get the rest of the data out? Assuming there is no more data -- that is, all the underlying source has left is still < the descriptor's [[elementSize]].

That causes the pending read-into request to be rejected with an error. See the discussion starting here for background: https://github.com/whatwg/streams/issues/353#issuecomment-103275526

> @@ +4306,5 @@
> > +    }
> > +
> > +    // Step 8: If controller.[[queueTotalSize]] > 0,
> > +    uint32_t queueTotalSize = controller->getFixedSlot(QueueContainerSlot_TotalSize).toInt32();
> > +    if (queueTotalSize > 0) {

> Spec question: This is the case when the pullIntoDescriptor can be fulfilled immediately. Why do we not need to TransferArrayBuffer in this case? Why do we need to TransferArrayBuffer at all?

That's a good question. It does look like we should need to TransferArrayBuffer here, too. Investigating.

> @@ +4533,5 @@
> > +        }
> > +    }
> > +
> > +    // Step 7: Set pullIntoDescriptor.[[buffer]] to
> > +    //         ! TransferArrayBuffer(pullIntoDescriptor.[[buffer]]).

> So... the buffer is transferred every time some bytes are filled in a pullIntoDescriptor? But wait -- calling byobRequest.respond always shifts off the first descriptor and fulfills a readInto request. Is this transferring twice? Didn't pullIntoDescriptor.buffer already get transferred when it was enqueued?

I agree that this looks superfluous. Investigating. It's not relevant to correctness in any case, at least.

> @@ +4611,5 @@
> > +
> > +    // Step 3: If firstDescriptor.[[byteOffset]] + firstDescriptor.[[bytesFilled]]
> > +    //         is not view.[[ByteOffset]], throw a RangeError exception.
> > +    uint32_t byteOffset = uint32_t(JS_GetArrayBufferViewByteOffset(view));
> > +    if (firstDescriptor->byteOffset() + firstDescriptor->bytesFilled() != byteOffset) {

> How does the programmer create a new view with the correct offsets? Copying them from byobRequest.view?

I guess? There is pretty much no documentation or discussion to be found on this.
Comment on attachment 8886594 [details] [diff] [review]
Part 3: Add JSAPI functions for working with ReadableStream

Review of attachment 8886594 [details] [diff] [review]:
-----------------------------------------------------------------

Man, having a C++ API sure would help here...

Awesome job documenting every one of those C functions.

While most of the stuff here is sensible, the API for external underlying source readable streams aren't quite matched up to the JS API, like making some controller methods available on the stream itself. They seem not very well segregated from the standardized methods, and could lead to confusion, or worse, as the spec evolves, behavior deviation. Things I'd like to see:

1. A block comment about how this new kind of external source readable stream behaves. In what ways does it deviate from spec for non-external sources?
2. Are the convenience methods absolutely necessary? If so, could we separate them better?

::: js/src/builtin/Stream.cpp
@@ +1007,5 @@
> +    // explicitly via JSAPI, which is indicated by a controller flag.
> +    // IsReadableStreamLocked is called from the controller's constructor, at
> +    // which point we can't yet call ControllerFromStream(stream), but the
> +    // source also can't be locked yet.
> +    if (!stream->getFixedSlot(StreamSlot_Controller).isUndefined() &&

Could factor out checking if the controller is undefined to a bool local.

@@ +1018,5 @@
>  
> +bool
> +ReadableStream::locked() const
> +{
> +    return IsReadableStreamLocked(this);

Why isn't the implementation of IsReadableStreamLocked inlined into this method?

@@ -1535,5 @@
>      //         stream.[[reader]].[[readRequests]].
>      Value readerVal = stream->getFixedSlot(StreamSlot_Reader);
> -    NativeObject* reader = &readerVal.toObject().as<NativeObject>();
> -    MOZ_ASSERT(reader->is<ReadableStreamDefaultReader>() ||
> -               reader->is<ReadableStreamBYOBReader>());

Seems like this assertion is still useful when readerVal is not undefined.

@@ +2392,4 @@
>      if (!controller)
>          return false;
>  
> +    stream->setFixedSlot(StreamSlot_Controller, ObjectValue(*controller));

Wait, why is this here?

@@ +2635,4 @@
>          Rooted<ReadableStreamDefaultController*> defaultController(cx);
>          defaultController = &controller->as<ReadableStreamDefaultController>();
>          return ReadableStreamTee_Cancel(cx, teeState, defaultController, reason);
> +    } else if (ControllerFlags(controller) & ControllerFlag_ExternalSource) {

Nit: else after return

@@ +3206,5 @@
> +        return nullptr;
> +
> +    // Step 14: Let controller be this (implicit).
> +    // Step 15: Let startResult be
> +    //          ? InvokeOrNoop(underlyingSource, "start", « this »).

How are external underlying sources started then?

@@ +3509,5 @@
> +            {
> +                JS::AutoCheckCannotGC noGC(cx);
> +                bool dummy;
> +                void* buffer = JS_GetArrayBufferViewData(view, &dummy, noGC);
> +                auto cb = cx->runtime()->readableStreamWriteIntoReadRequestCallback;

Check if (cb)?

@@ +4235,5 @@
> +            JS::AutoCheckCannotGC noGC(cx);
> +            bool dummy;
> +            uint8_t* buffer = JS_GetArrayBufferData(targetBuffer, &dummy, noGC);
> +            buffer += bytesFilled;
> +            auto cb = cx->runtime()->readableStreamWriteIntoReadRequestCallback;

Check if (cb)?

@@ +5317,5 @@
> +{
> +    Rooted<ReadableByteStreamController*> controller(cx);
> +    controller = &ControllerFromStream(stream)->as<ReadableByteStreamController>();
> +
> +    // Step 2: If this.[[closeRequested]] is true, throw a TypeError exception.

What spec section do these steps refer to?

@@ +5391,5 @@
> +//        if (!ReadableByteStreamControllerEnqueueChunkToQueue(cx, controller, transferredBuffer,
> +//                                                             byteOffset, byteLength))
> +//        {
> +//            return false;
> +//        }

Why commented out?

@@ +5410,5 @@
> +//        if (!ReadableByteStreamControllerEnqueueChunkToQueue(cx, controller, transferredBuffer,
> +//                                                            byteOffset, byteLength))
> +//        {
> +//            return false;
> +//        }

Why commented out?

::: js/src/builtin/Stream.h
@@ +37,5 @@
> +    void desiredSize(bool* hasSize, double* size) const;
> +
> +    JS::ReadableStreamMode mode() const;
> +
> +    static bool close(JSContext* cx, Handle<ReadableStream*> stream);

Missed MOZ_MUST_USE?

::: js/src/jsapi.cpp
@@ +5263,5 @@
> +                                             JS::WriteIntoReadRequestBufferCallback writeIntoReadRequestCallback,
> +                                             JS::CancelReadableStreamCallback cancelCallback,
> +                                             JS::ReadableStreamClosedCallback closedCallback,
> +                                             JS::ReadableStreamErroredCallback erroredCallback,
> +                                             JS::ReadableStreamFinalizeCallback finalizeCallback)

Nit: indent is off

@@ +5309,5 @@
> +    JSRuntime* rt = cx->runtime();
> +    MOZ_ASSERT(rt->readableStreamDataRequestCallback);
> +    MOZ_ASSERT(rt->readableStreamWriteIntoReadRequestCallback);
> +    MOZ_ASSERT(rt->readableStreamCancelCallback);
> +    MOZ_ASSERT(rt->readableStreamFinalizeCallback);

Missing asserts for closed and errored.

::: js/src/jsapi.h
@@ +4830,5 @@
> + * underlying source being finalized. Only the underlying source is passed
> + * as an argument, while the ReadableStream itself is not to prevent the
> + * embedding from operating on a JSObject that might not be in a valid state
> + * anymore.
> + */

ReadableStreams aren't background finalizable, right?

@@ +4855,5 @@
> +
> +/**
> + * Returns a new instance of the ReadableStream builtin class in the current
> + * compartment, configured as a default stream.
> + * If a `proto` is passed, that gets set as the instance's [[Prototype]]

Do you have a compelling example use case for allowing custom protos? What's the use of creating a stream that doesn't expose at least the basic set of methods on the default proto?

@@ +4907,5 @@
> +extern JS_PUBLIC_API(uint8_t)
> +ReadableStreamGetEmbeddingFlags(const JSObject* stream);
> +
> +/**
> + * Returns the given `stream`s embedding-provided underlying source.

Nit: `stream`'s? That looks super weird though. Maybe "Returns the embedding-provided underlying source of the given |stream|. Do similar edits to other occurrences of `stream` please. There's inconsistent use of ` and |.

@@ +4919,5 @@
> + * called.
> + *
> + * Throws an exception if the stream is locked, i.e. if a reader has been
> + * acquired for the stream, or if ReadableStreamGetExternalUnderlyingSource
> + * has been used previously without releasing the external source again.

Ah ha, this kind of optimization is why an externally sourced readable stream needs to be lockable without having a reader?

@@ +5066,5 @@
> +                  JS::MutableHandleObject branch1Stream, JS::MutableHandleObject branch2Stream);
> +
> +/**
> + * Retrieves the desired combined size of additional chunks to fill the given
> + * ReadableStream's queue, stores it in |value| and sets |hasValue| to true.

This reads |value| will always get something and |hasValue| will always be set to true.

@@ +5071,5 @@
> + *
> + * If the stream is errored, the call will succeed but no value will be stored
> + * in |value| and |hasValue| will be set to false.
> + *
> + * Throws a TypeError and returns false if the operation fails.

The return type here is void.

::: js/src/vm/Runtime.cpp
@@ +110,5 @@
>      promiseTasksToDestroy(mutexid::PromiseTaskPtrVector),
> +    readableStreamDataRequestCallback(nullptr),
> +    readableStreamWriteIntoReadRequestCallback(nullptr),
> +    readableStreamCancelCallback(nullptr),
> +    readableStreamFinalizeCallback(nullptr),

Missing nulling out closed and errored here.
Attachment #8886594 - Flags: review?(shu)
I'm a little confused by the compartment/wrapper handling in these patches.  Let's take a simple example: JS::ReadableStreamGetReader.  The documentation says:

>+ * Asserts that |stream| is a ReadableStream instance or a wrapper for one
>+ * that can be unwrapped safely.

but the implementation does:

>+    Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

without worrying about it maybe being a wrapper.  Which one is wrong?

Also, in ReadableStreamGetReader(), what happens when cx and streamObj are not same-compartment?  Is that allowed?  It's not asserted against... but then we end up creating the reader in the cx compartment and sticking these objects in each other's slots, which I'm pretty sure _does_ assert.

As I read the stream spec, nothing requires that ReadableStreamGetReader() happen in the "stream global", fwiw.
Flags: needinfo?(till)
Oh, and all that stuff should have tests, clearly.
(In reply to Boris Zbarsky [:bz] from comment #41)
> I'm a little confused by the compartment/wrapper handling in these patches. 
> Let's take a simple example: JS::ReadableStreamGetReader.  The documentation
> says:
> 
> >+ * Asserts that |stream| is a ReadableStream instance or a wrapper for one
> >+ * that can be unwrapped safely.
> 
> but the implementation does:
> 
> >+    Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());
> 
> without worrying about it maybe being a wrapper.  Which one is wrong?

The documentation. I decided at some point to for now require the embedding to ensure that the stream/reader is unwrapped, but forgot to update the docs.
> 
> Also, in ReadableStreamGetReader(), what happens when cx and streamObj are
> not same-compartment?  Is that allowed?  It's not asserted against... but
> then we end up creating the reader in the cx compartment and sticking these
> objects in each other's slots, which I'm pretty sure _does_ assert.

You're right, I need to add asserts at the very least.
> 
> As I read the stream spec, nothing requires that ReadableStreamGetReader()
> happen in the "stream global", fwiw.

Is there any upside to not doing it in the stream's global? I'd *very much* prefer not having to deal with wrapped streams/readers in pretty much every single part of the implementation.

> Oh, and all that stuff should have tests, clearly.

True. I have some amount of tests for the jsapi bits (in jsapi-tests/testReadableStream.cpp), but they're not enough, nor do they cover any cross-global stuff.
Flags: needinfo?(till)
> Is there any upside to not doing it in the stream's global? 

I don't know, but the specs seem to require it in various cases.  If that's at all observable, then we need to either implement it or change the specs.

> nor do they cover any cross-global stuff

I meant specifically for the cross-global stuff we need tests.  The spec is pretty cavalier about this stuff, but we can't be in implementation....
Comment on attachment 8886594 [details] [diff] [review] [diff] [review]
Part 3: Add JSAPI functions for working with ReadableStream

Review of attachment 8886594 [details] [diff] [review] [diff] [review]:
-----------------------------------------------------------------

Please interpret this review request as meaning the entire part 3, not just the interdiff.

> Man, having a C++ API sure would help here...

Yes, it sure would :(

> Awesome job documenting every one of those C functions.

> While most of the stuff here is sensible, the API for external underlying source readable streams aren't quite matched up to the JS API, like making some controller methods available on the stream itself. They seem not very well segregated from the standardized methods, and could lead to confusion, or worse, as the spec evolves, behavior deviation. Things I'd like to see:

1. A block comment about how this new kind of external source readable stream behaves. In what ways does it deviate from spec for non-external sources?
2. Are the convenience methods absolutely necessary? If so, could we separate them better?

I moved all streams-related functions into their own header, js/public/Stream.h, and added a long block comment at the top that gives an overview of the API and some reasoning for why various parts are there at all and have the shape they have.

@@ +3206,5 @@
> +        return nullptr;
> +
> +    // Step 14: Let controller be this (implicit).
> +    // Step 15: Let startResult be
> +    //          ? InvokeOrNoop(underlyingSource, "start", « this »).

> How are external underlying sources started then?

They aren't started in the same way as normal JS sources are. I guess I could introduce a callback for that and invoke it during stream construction. So far, that hasn't been required, and I don't really foresee any need for it. Part of the reason is that in JS, the controller isn't otherwise exposed (except as an argument to the source's pull method), so this gives the source a chance to store a reference to the controller for push-based operation. That's not required for jsapi usage.

::: js/src/jsapi.h
@@ +4830,5 @@
> + * underlying source being finalized. Only the underlying source is passed
> + * as an argument, while the ReadableStream itself is not to prevent the
> + * embedding from operating on a JSObject that might not be in a valid state
> + * anymore.
> + */

> ReadableStreams aren't background finalizable, right?

They are - I added a note about that to the comment.

@@ +4855,5 @@
> +
> +/**
> + * Returns a new instance of the ReadableStream builtin class in the current
> + * compartment, configured as a default stream.
> + * If a `proto` is passed, that gets set as the instance's [[Prototype]]

> Do you have a compelling example use case for allowing custom protos? What's the use of creating a stream that doesn't expose at least the basic set of methods on the default proto?

The embedding might want to expose something that is a subclass of ReadableStream, e.g. to add additional functions to the object's interface. It certainly wouldn't want to expose fewer methods, I agree, but that's not what this is for. Note that other builtins provide for passing custom protos this way, e.g. Promise.

I don't know of any current use cases in Gecko, but I'm pretty sure that I will have use cases for this in our runtime thing.

@@ +4919,5 @@
> + * called.
> + *
> + * Throws an exception if the stream is locked, i.e. if a reader has been
> + * acquired for the stream, or if ReadableStreamGetExternalUnderlyingSource
> + * has been used previously without releasing the external source again.

> Ah ha, this kind of optimization is why an externally sourced readable stream needs to be lockable without having a reader?

Exactly.
Attachment #8889421 - Flags: review?(shubhamjindal18)
Comment on attachment 8889421 [details] [diff] [review]
Changes to address review comments for part 3

Actually asking shu for review. My apologies for the bugmail spam, shubhamjindal18.

(And not actually formally asking shu for review because "is not currently accepting 'review' requests")
Flags: needinfo?(shu)
Attachment #8889421 - Flags: review?(shubhamjindal18)
New version incorporating shu's feedback on the jsapi bits. Note that the jsapi functions now live in their own header: js/public/Stream.h
Attachment #8886603 - Attachment is obsolete: true
Attachment #8886603 - Flags: feedback?(amarchesini)
Attachment #8889447 - Flags: feedback?(amarchesini)
Attachment #8889421 - Flags: review?(shu)
Comment on attachment 8889421 [details] [diff] [review]
Changes to address review comments for part 3

Review of attachment 8889421 [details] [diff] [review]:
-----------------------------------------------------------------

The code overall looks fine to me.

This review comes down to a question of taste about how usable the C API is. That's hard for me to really judge, given that I don't even have experience programming with the JS streams API. Due to that, my main guidance for the review is usage-style parity with the JS API such that if a programmer were familiar with the JS Streams API, using the C API should not involve jumping through hoops.

Given the new comments, I'm okay with the current API. I suppose the best thing to do here is to get more experience using it (as I'm sure you're doing), and improve it as we go.

::: js/public/Stream.h
@@ +9,5 @@
> + *
> + * Much of the API here mirrors the JS API of ReadableStream and associated
> + * classes, e.g. ReadableStreamDefaultReader, ReadableStreamBYOBReader,
> + * ReadableStreamDefaultController, ReadableByteStreamController, and
> + * ReadableStreamBYOBRequest.

I like the separate file much better.

@@ +369,5 @@
> + * outparams. Returns false if the operation fails, e.g. because the stream is
> + * locked.
> + *
> + * Asserts that |stream| is an unwrapped ReadableStream instance or a wrapper for one
> + * that can be unwrapped safely.

Looks like you made all APIs require unwrapped stream objects. Leftover comment?

@@ +384,5 @@
> + * If the stream is errored, the call will succeed but no value will be stored
> + * in |value| and |hasValue| will be set to false.
> + *
> + * Note: In JS, this is a getter on the stream's associated controller, but
> + * we expose it with the stream itself as a target for simplicity.

Could tighten up this language to say that it is semantically equivalent to getting the underlying controller of the stream, then calling the desiredSize getter. Ditto for the other helpers.
Attachment #8889421 - Flags: review?(shu) → review+
This just adds the dom.streams.enabled pref and pipes it through to the JS engine.

Baku, note that this adds the pref to a different location in all.js compared to your patch adding the same pref, so you need to remove it from your patch.
Flags: needinfo?(shu)
Attachment #8890836 - Flags: review?(amarchesini)
Comment on attachment 8890836 [details] [diff] [review]
Add runtime pref to enable streams

Review of attachment 8890836 [details] [diff] [review]:
-----------------------------------------------------------------

For the DOM part. I cannot review the js part.
Attachment #8890836 - Flags: review?(amarchesini) → review+
Attachment #8890836 - Flags: review?(jcoppeard)
Comment on attachment 8890836 [details] [diff] [review]
Add runtime pref to enable streams

Review of attachment 8890836 [details] [diff] [review]:
-----------------------------------------------------------------

The JS parts look fine to me.
Attachment #8890836 - Flags: review?(jcoppeard) → review+
Comment on attachment 8889447 [details] [diff] [review]
Rollup patch for testing, v4

Review of attachment 8889447 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsnum.h
@@ +82,5 @@
>  Int32ToAtom(JSContext* cx, int32_t si);
>  
> +// ES6 15.7.3.12
> +extern bool
> +IsInteger(Value val);

Drive-by comment:
This should be |const Value&| per http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/js/public/Value.h#289-291.
(In reply to André Bargull from comment #52)
> Drive-by comment:
> This should be |const Value&| per
> http://searchfox.org/mozilla-central/rev/
> ad093e98f42338effe2e2513e26c3a311dd96422/js/public/Value.h#289-291.

Thank you, try also made me aware of that by now :)
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d87b1dc4ce2
Part 2: Add runtime pref to enable streams. r=baku,jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/862ba8683d19
Part 3: Implement ReadableStream and associated classes in the JS engine. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/2693a863dabd
Part 4: Add JSAPI functions for working with ReadableStream. r=shu
Renaming because this only adds ReadableStream and directly associated classes, not WritableStream.
Summary: Implement pure js streams in the JavaScript Engine → Implement ReadableStream in the JavaScript Engine
Backed out for failing mochitest test_interfaces.html for 'ByteLengthQueuingStrategy' and wpts, e.g. general.dedicatedworker.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf3949a7ae8577007f7692cdb9beb3bc85fae8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85c954dfda4a99af6de33828b129ba962fc752f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d6b96a16105bed25679a46d2ea163a538e37998

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2693a863dabdd783e32e7b8ac6164e25c30fed4c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel

Failure log mochitest: https://treeherder.mozilla.org/logviewer.html#?job_id=119028013&repo=mozilla-inbound
> dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface ByteLengthQueuingStrategy to all webpages as a property on the window (XBL: false)? Do not make a change to this file without a review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals)

Failure log example wpt: https://treeherder.mozilla.org/logviewer.html#?job_id=119027938&repo=mozilla-inbound
17:14:22     INFO - TEST-START | /streams/readable-byte-streams/general.dedicatedworker.html
17:14:22     INFO - Setting pref dom.streams.enabled (true)
17:14:23     INFO - 
17:14:23     INFO - TEST-UNEXPECTED-FAIL | /streams/readable-byte-streams/general.dedicatedworker.html | getReader({mode: "byob"}) throws on non-bytes streams - assert_throws: function "() => new ReadableStream().getReader({ mode: 'byob' })" threw object "ReferenceError: ReadableStream is not defined" ("ReferenceError") expected object "TypeError" ("TypeError")
17:14:23     INFO - @http://web-platform.test:8000/streams/readable-byte-streams/general.js:12:3
17:14:23     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1412:20
17:14:23     INFO - test@http://web-platform.test:8000/resources/testharness.js:501:9
17:14:23     INFO - @http://web-platform.test:8000/streams/readable-byte-streams/general.js:11:1
Flags: needinfo?(till)
I believe these test things are fixed in baku's patchset.  You might need to push it all together.
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64bbc26920aa
Part 2: Add runtime pref to enable streams. r=jonco,baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1427496354
Part 3: Implement ReadableStream and associated classes in the JS engine. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/364538819208
Part 4: Add JSAPI functions for working with ReadableStream. r=shu,f=baku
(In reply to Ben Kelly [:bkelly] from comment #57)
> I believe these test things are fixed in baku's patchset.  You might need to
> push it all together.

The failures were caused by streams being disabled by default and the runtime pref update not being propagated to the JS engine in a timely manner. That only works for "javascript.options.foo" properties. So this new landing has the pref renamed to "javascript.options.streams". (I also fixed a couple of unrelated silly issues I overlooked in my earlier try run.)
Flags: needinfo?(till)
Depends on: 1385890
Depends on: 1386203
Till, should this still be open?
Flags: needinfo?(till)
(In reply to Ben Kelly [:bkelly] from comment #61)
> Till, should this still be open?

No, it shouldn't. I'll attach a patch for the cross-global stuff to bug 1385890 in a bit, but it makes sense to close this one.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(till)
Resolution: --- → FIXED
Blocks: 1385890
No longer depends on: 1385890
Keywords: leave-open
Target Milestone: --- → mozilla56
Version: 48 Branch → Trunk
Attachment #8889447 - Flags: feedback?(amarchesini) → feedback+
Depends on: 1441927
You need to log in before you can comment on or make changes to this bug.