Bug 1507952 - Part 2: Rewrite ReadableStream constructor to match the standard. r=arai
authorJason Orendorff <jorendorff@mozilla.com>
Thu, 22 Nov 2018 13:41:37 +0000
changeset 504204 5a0df7634eaf384506d4ed5213dff1235325ced1
parent 504203 dc904523f5a381ed77904a2b4db577160a2a4e03
child 504205 7687d5a601cb074ba03e97af24ad93d0176867ca
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersarai
bugs1507952
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1507952 - Part 2: Rewrite ReadableStream constructor to match the standard. r=arai We were very close to compliance, but all the step numbers have changed and some user-visible behavior around default arguments was a bit off. Also, update step numbers in ValidateAndNormalizeHighWaterMark, implement MakeSizeAlgorithmFromSizeFunction, and generally validate size/highWaterMark arguments earlier. Differential Revision: https://phabricator.services.mozilla.com/D12456
js/src/builtin/Stream.cpp
js/src/builtin/Stream.h
js/src/tests/non262/ReadableStream/constructor-default.js
--- a/js/src/builtin/Stream.cpp
+++ b/js/src/builtin/Stream.cpp
@@ -488,38 +488,43 @@ const Class cls::protoClass_ = { \
 
 /*** 3.2. Class ReadableStream **********************************************/
 
 static MOZ_MUST_USE ReadableStreamDefaultController*
 CreateReadableStreamDefaultController(JSContext* cx,
                                       Handle<ReadableStream*> stream,
                                       HandleValue underlyingSource,
                                       HandleValue size,
-                                      HandleValue highWaterMarkVal);
+                                      double highWaterMarkVal);
 
 /**
  * Streams spec, 3.2.3., steps 1-4, 8.
  */
 ReadableStream*
 ReadableStream::createDefaultStream(JSContext* cx, HandleValue underlyingSource,
-                                    HandleValue size, HandleValue highWaterMark,
+                                    HandleValue size, double highWaterMark,
                                     HandleObject proto /* = nullptr */)
 {
+    cx->check(underlyingSource, size, proto);
+    MOZ_ASSERT(size.isUndefined() || IsCallable(size));
+    MOZ_ASSERT(highWaterMark >= 0);
+
     // Steps 1-4.
     Rooted<ReadableStream*> stream(cx, create(cx));
     if (!stream) {
         return nullptr;
     }
 
     // Step 8.b: Set this.[[readableStreamController]] to
     //           ? Construct(ReadableStreamDefaultController,
     //                       « this, underlyingSource, size,
     //                         highWaterMark »).
     ReadableStreamDefaultController* controller =
-        CreateReadableStreamDefaultController(cx, stream, underlyingSource, size, highWaterMark);
+        CreateReadableStreamDefaultController(cx, stream, underlyingSource, size,
+                                              highWaterMark);
     if (!controller) {
         return nullptr;
     }
     stream->setController(controller);
     return stream;
 }
 
 static MOZ_MUST_USE ReadableByteStreamController*
@@ -542,103 +547,130 @@ ReadableStream::createExternalSourceStre
     }
 
     stream->setController(controller);
     controller->setEmbeddingFlags(flags);
 
     return stream;
 }
 
+static MOZ_MUST_USE bool
+MakeSizeAlgorithmFromSizeFunction(JSContext* cx, HandleValue size);
+
+static MOZ_MUST_USE bool
+ValidateAndNormalizeHighWaterMark(JSContext* cx,
+                                  HandleValue highWaterMarkVal,
+                                  double* highWaterMark);
+
 /**
- * Streams spec, 3.2.3.
+ * Streams spec, 3.2.3. new ReadableStream(underlyingSource = {}, strategy = {})
  */
 bool
 ReadableStream::constructor(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
 
-    RootedValue underlyingSource(cx, args.get(0));
-    RootedValue options(cx, args.get(1));
-
-    // Do argument handling first to keep the right order of error reporting.
-    if (underlyingSource.isUndefined()) {
-        RootedObject sourceObj(cx, NewBuiltinClassInstance<PlainObject>(cx));
-        if (!sourceObj) {
-            return false;
-        }
-        underlyingSource = ObjectValue(*sourceObj);
-    }
-    RootedValue size(cx);
-    RootedValue highWaterMark(cx);
-
-    if (!options.isUndefined()) {
-        if (!GetProperty(cx, options, cx->names().size, &size)) {
-            return false;
-        }
-
-        if (!GetProperty(cx, options, cx->names().highWaterMark, &highWaterMark)) {
-            return false;
-        }
-    }
-
     if (!ThrowIfNotConstructing(cx, args, "ReadableStream")) {
         return false;
     }
 
-    // Step 5: Let type be ? GetV(underlyingSource, "type").
-    RootedValue typeVal(cx);
-    if (!GetProperty(cx, underlyingSource, cx->names().type, &typeVal)) {
-        return false;
-    }
-
-    // Step 6: Let typeString be ? ToString(type).
-    RootedString type(cx, ToString<CanGC>(cx, typeVal));
-    if (!type) {
-        return false;
-    }
-
-    int32_t notByteStream;
-    if (!CompareStrings(cx, type, cx->names().bytes, &notByteStream)) {
+    // Implicit in the spec: argument default values.
+    RootedValue underlyingSource(cx, args.get(0));
+    if (underlyingSource.isUndefined()) {
+        JSObject* emptyObj = NewBuiltinClassInstance<PlainObject>(cx);
+        if (!emptyObj) {
+            return false;
+        }
+        underlyingSource = ObjectValue(*emptyObj);
+    }
+
+    RootedValue strategy(cx, args.get(1));
+    if (strategy.isUndefined()) {
+        JSObject* emptyObj = NewBuiltinClassInstance<PlainObject>(cx);
+        if (!emptyObj) {
+            return false;
+        }
+        strategy = ObjectValue(*emptyObj);
+    }
+
+    // Step 1: Perform ! InitializeReadableStream(this).
+    // This is moved down to step 7.d.
+
+    // Step 2: Let size be ? GetV(strategy, "size").
+    RootedValue size(cx);
+    if (!GetProperty(cx, strategy, cx->names().size, &size)) {
         return false;
     }
 
-    // Step 7.a & 8.a (reordered): If highWaterMark is undefined, let
-    //                             highWaterMark be 1 (or 0 for byte streams).
-    if (highWaterMark.isUndefined()) {
-        highWaterMark = Int32Value(notByteStream ? 1 : 0);
-    }
-
-    Rooted<ReadableStream*> stream(cx);
-
-    // Step 7: If typeString is "bytes",
-    if (!notByteStream) {
-        // Step 7.b: Set this.[[readableStreamController]] to
-        //           ? Construct(ReadableByteStreamController,
-        //                       « this, underlyingSource, highWaterMark »).
+    // Step 3: Let highWaterMark be ? GetV(strategy, "highWaterMark").
+    RootedValue highWaterMarkVal(cx);
+    if (!GetProperty(cx, strategy, cx->names().highWaterMark, &highWaterMarkVal)) {
+        return false;
+    }
+
+    // Step 4: Let type be ? GetV(underlyingSource, "type").
+    RootedValue type(cx);
+    if (!GetProperty(cx, underlyingSource, cx->names().type, &type)) {
+        return false;
+    }
+
+    // Step 5: Let typeString be ? ToString(type).
+    RootedString typeString(cx, ToString<CanGC>(cx, type));
+    if (!typeString) {
+        return false;
+    }
+
+    // Step 6: If typeString is "bytes",
+    int32_t cmp;
+    if (!CompareStrings(cx, typeString, cx->names().bytes, &cmp)) {
+        return false;
+    }
+    if (cmp == 0) {
+        // The rest of step 6 is unimplemented, since we don't support
+        // user-defined byte streams yet.
         JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                                   JSMSG_READABLESTREAM_BYTES_TYPE_NOT_IMPLEMENTED);
         return false;
-    } else if (typeVal.isUndefined()) {
-        // Step 8: Otherwise, if type is undefined,
-        // Step 8.b: Set this.[[readableStreamController]] to
-        //           ? Construct(ReadableStreamDefaultController,
-        //                       « this, underlyingSource, size, highWaterMark »).
-        stream = createDefaultStream(cx, underlyingSource, size, highWaterMark);
-    } else {
-        // Step 9: Otherwise, throw a RangeError exception.
-        JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
-                                  JSMSG_READABLESTREAM_UNDERLYINGSOURCE_TYPE_WRONG);
-        return false;
-    }
-    if (!stream) {
-        return false;
-    }
-
-    args.rval().setObject(*stream);
-    return true;
+    }
+
+    // Step 7: Otherwise, if type is undefined,
+    if (type.isUndefined()) {
+        // Step 7.a: Let sizeAlgorithm be ? MakeSizeAlgorithmFromSizeFunction(size).
+        if (!MakeSizeAlgorithmFromSizeFunction(cx, size)) {
+            return false;
+        }
+
+        // Step 7.b: If highWaterMark is undefined, let highWaterMark be 1.
+        double highWaterMark;
+        if (highWaterMarkVal.isUndefined()) {
+            highWaterMark = 1;
+        } else {
+            // Step 7.c: Set highWaterMark to ? ValidateAndNormalizeHighWaterMark(highWaterMark).
+            if (!ValidateAndNormalizeHighWaterMark(cx, highWaterMarkVal, &highWaterMark)) {
+                return false;
+            }
+        }
+
+        // Step 7.d: Perform
+        //           ? SetUpReadableStreamDefaultControllerFromUnderlyingSource(
+        //           this, underlyingSource, highWaterMark, sizeAlgorithm).
+        Rooted<ReadableStream*> stream(cx,
+            createDefaultStream(cx, underlyingSource, size, highWaterMark));
+        if (!stream) {
+            return false;
+        }
+
+        args.rval().setObject(*stream);
+        return true;
+    }
+
+    // Step 8: Otherwise, throw a RangeError exception.
+    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
+                              JSMSG_READABLESTREAM_UNDERLYINGSOURCE_TYPE_WRONG);
+    return false;
 }
 
 /**
  * Streams spec, 3.2.5.1. get locked
  */
 static bool
 ReadableStream_locked(JSContext* cx, unsigned argc, Value* vp)
 {
@@ -1236,36 +1268,33 @@ ReadableStreamTee(JSContext* cx,
     // [[pullAlgorithm]], [[cancelAlgorithm]], and so on. Instead, we decide
     // which one to perform based on class checks. For example, our
     // implementation of ReadableStreamControllerCallPullIfNeeded checks
     // whether the stream's underlyingSource is a TeeState object.
 
     // Step 16: Set branch1 to
     //          ! CreateReadableStream(startAlgorithm, pullAlgorithm,
     //                                 cancel1Algorithm).
-    RootedValue hwmValue(cx, NumberValue(1));
     RootedValue underlyingSource(cx, ObjectValue(*teeState));
     branch1Stream.set(ReadableStream::createDefaultStream(cx, underlyingSource,
-                                                          UndefinedHandleValue,
-                                                          hwmValue));
+                                                          UndefinedHandleValue, 1));
     if (!branch1Stream) {
         return false;
     }
 
     Rooted<ReadableStreamDefaultController*> branch1(cx);
     branch1 = &branch1Stream->controller()->as<ReadableStreamDefaultController>();
     branch1->setTeeBranch1();
     teeState->setBranch1(branch1);
 
     // Step 17: Set branch2 to
     //          ! CreateReadableStream(startAlgorithm, pullAlgorithm,
     //                                 cancel2Algorithm).
     branch2Stream.set(ReadableStream::createDefaultStream(cx, underlyingSource,
-                                                          UndefinedHandleValue,
-                                                          hwmValue));
+                                                          UndefinedHandleValue, 1));
     if (!branch2Stream) {
         return false;
     }
 
     Rooted<ReadableStreamDefaultController*> branch2(cx);
     branch2 = &branch2Stream->controller()->as<ReadableStreamDefaultController>();
     branch2->setTeeBranch2();
     teeState->setBranch2(branch2);
@@ -2257,44 +2286,35 @@ ControllerStartFailedHandler(JSContext* 
             return false;
         }
     }
 
     args.rval().setUndefined();
     return true;
 }
 
-static MOZ_MUST_USE bool
-ValidateAndNormalizeHighWaterMark(JSContext* cx,
-                                  HandleValue highWaterMarkVal,
-                                  double* highWaterMark);
-
-static MOZ_MUST_USE bool
-ValidateAndNormalizeQueuingStrategy(JSContext* cx,
-                                    HandleValue size,
-                                    HandleValue highWaterMarkVal,
-                                    double* highWaterMark);
-
 /**
  * Streams spec, 3.8.3
  *      new ReadableStreamDefaultController ( stream, underlyingSource,
  *                                            size, highWaterMark )
  * Steps 3 - 11.
  *
  * Note: All arguments must be same-compartment with cx. ReadableStream
  * controllers are always created in the same compartment as the stream.
  */
 static MOZ_MUST_USE ReadableStreamDefaultController*
 CreateReadableStreamDefaultController(JSContext* cx,
                                       Handle<ReadableStream*> stream,
                                       HandleValue underlyingSource,
                                       HandleValue size,
-                                      HandleValue highWaterMarkVal)
+                                      double highWaterMark)
 {
-    cx->check(stream, underlyingSource, size, highWaterMarkVal);
+    cx->check(stream, underlyingSource, size);
+    MOZ_ASSERT(highWaterMark >= 0);
+    MOZ_ASSERT(size.isUndefined() || IsCallable(size));
 
     Rooted<ReadableStreamDefaultController*> controller(cx,
         NewBuiltinClassInstance<ReadableStreamDefaultController>(cx));
     if (!controller) {
         return nullptr;
     }
 
     // Step 3: Set this.[[controlledReadableStream]] to stream.
@@ -2308,21 +2328,18 @@ CreateReadableStreamDefaultController(JS
         return nullptr;
     }
 
     // Step 6: Set this.[[started]], this.[[closeRequested]], this.[[pullAgain]],
     //         and this.[[pulling]] to false.
     controller->setFlags(0);
 
     // Step 7: Let normalizedStrategy be
-    //         ? ValidateAndNormalizeQueuingStrategy(size, highWaterMark).
-    double highWaterMark;
-    if (!ValidateAndNormalizeQueuingStrategy(cx, size, highWaterMarkVal, &highWaterMark)) {
-        return nullptr;
-    }
+    //         ? ValidateAndNormalizeQueuingStrategy(size, highWaterMark)
+    //         (implicit).
 
     // Step 8: Set this.[[strategySize]] to normalizedStrategy.[[size]] and
     //         this.[[strategyHWM]] to normalizedStrategy.[[highWaterMark]].
     controller->setStrategySize(size);
     controller->setStrategyHWM(highWaterMark);
 
     // Step 9: Let controller be this (implicit).
 
@@ -4066,57 +4083,69 @@ PromiseInvokeOrNoop(JSContext* cx, Handl
     if (!InvokeOrNoop(cx, O, P, arg, &returnValue)) {
         return PromiseRejectedWithPendingError(cx);
     }
 
     // Step 6: Otherwise, return a promise resolved with returnValue.[[Value]].
     return PromiseObject::unforgeableResolve(cx, returnValue);
 }
 
-// Streams spec, 6.3.7. ValidateAndNormalizeHighWaterMark ( highWaterMark )
+
+/**
+ * Streams spec, 6.3.7. ValidateAndNormalizeHighWaterMark ( highWaterMark )
+ */
 static MOZ_MUST_USE bool
-ValidateAndNormalizeHighWaterMark(JSContext* cx, HandleValue highWaterMarkVal, double* highWaterMark)
+ValidateAndNormalizeHighWaterMark(JSContext* cx, HandleValue highWaterMarkVal,
+                                  double* highWaterMark)
 {
     // Step 1: Set highWaterMark to ? ToNumber(highWaterMark).
     if (!ToNumber(cx, highWaterMarkVal, highWaterMark)) {
         return false;
     }
 
-    // Step 2: If highWaterMark is NaN, throw a TypeError exception.
-    // Step 3: If highWaterMark < 0, throw a RangeError exception.
+    // Step 2: If highWaterMark is NaN or highWaterMark < 0, throw a RangeError exception.
     if (mozilla::IsNaN(*highWaterMark) || *highWaterMark < 0) {
         JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_STREAM_INVALID_HIGHWATERMARK);
         return false;
     }
 
-    // Step 4: Return highWaterMark.
+    // Step 3: Return highWaterMark.
     return true;
 }
 
-// Streams spec, obsolete (previously 6.4.6)
-//      ValidateAndNormalizeQueuingStrategy ( size, highWaterMark )
+/**
+ * Streams spec, 6.3.8. MakeSizeAlgorithmFromSizeFunction ( size )
+ *
+ * The standard makes a big deal of turning JavaScript functions (grubby,
+ * touched by users, covered with germs) into algorithms (pristine,
+ * respectable, purposeful). We don't bother. Here we only check for errors and
+ * leave `size` unchanged. Then, in ReadableStreamDefaultControllerEnqueue,
+ * where this value is used, we have to check for undefined and behave as if we
+ * had "made" an "algorithm" as described below.
+ */
 static MOZ_MUST_USE bool
-ValidateAndNormalizeQueuingStrategy(JSContext* cx, HandleValue size,
-                                    HandleValue highWaterMarkVal, double* highWaterMark)
+MakeSizeAlgorithmFromSizeFunction(JSContext* cx, HandleValue size)
 {
-    // Step 1: If size is not undefined and ! IsCallable(size) is false, throw a
-    //         TypeError exception.
-    if (!size.isUndefined() && !IsCallable(size)) {
+    // Step 1: If size is undefined, return an algorithm that returns 1.
+    if (size.isUndefined()) {
+        // Deferred. Size algorithm users must check for undefined.
+        return true;
+    }
+
+    // Step 2: If ! IsCallable(size) is false, throw a TypeError exception.
+    if (!IsCallable(size)) {
         JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_NOT_FUNCTION,
                                   "ReadableStream argument options.size");
         return false;
     }
 
-    // Step 2: Let highWaterMark be
-    //         ? ValidateAndNormalizeHighWaterMark(highWaterMark).
-    if (!ValidateAndNormalizeHighWaterMark(cx, highWaterMarkVal, highWaterMark)) {
-        return false;
-    }
-
-    // Step 3: Return Record {[[size]]: size, [[highWaterMark]]: highWaterMark}.
+    // Step 3: Return an algorithm that performs the following steps, taking a
+    //         chunk argument:
+    //     a. Return ? Call(size, undefined, « chunk »).
+    // Deferred. Size algorithm users must know how to call the size function.
     return true;
 }
 
 
 /*** API entry points *******************************************************/
 
 JS_FRIEND_API JSObject*
 js::UnwrapReadableStream(JSObject* obj)
@@ -4172,28 +4201,28 @@ JS::NewReadableDefaultStreamObject(JSCon
                                    JS::HandleFunction size /* = nullptr */,
                                    double highWaterMark /* = 1 */,
                                    JS::HandleObject proto /* = nullptr */)
 {
     MOZ_ASSERT(!cx->zone()->isAtomsZone());
     AssertHeapIsIdle();
     CHECK_THREAD(cx);
     cx->check(underlyingSource, size, proto);
+    MOZ_ASSERT(highWaterMark >= 0);
 
     RootedObject source(cx, underlyingSource);
     if (!source) {
         source = NewBuiltinClassInstance<PlainObject>(cx);
         if (!source) {
             return nullptr;
         }
     }
     RootedValue sourceVal(cx, ObjectValue(*source));
     RootedValue sizeVal(cx, size ? ObjectValue(*size) : UndefinedValue());
-    RootedValue highWaterMarkVal(cx, NumberValue(highWaterMark));
-    return ReadableStream::createDefaultStream(cx, sourceVal, sizeVal, highWaterMarkVal, proto);
+    return ReadableStream::createDefaultStream(cx, sourceVal, sizeVal, highWaterMark, proto);
 }
 
 JS_PUBLIC_API JSObject*
 JS::NewReadableExternalSourceStreamObject(JSContext* cx,
                                           void* underlyingSource,
                                           uint8_t flags /* = 0 */,
                                           HandleObject proto /* = nullptr */)
 {
--- a/js/src/builtin/Stream.h
+++ b/js/src/builtin/Stream.h
@@ -94,17 +94,17 @@ class ReadableStream : public NativeObje
 
     JS::ReadableStreamMode mode() const;
     uint8_t embeddingFlags() const;
 
     bool locked() const;
 
   public:
     static ReadableStream* createDefaultStream(JSContext* cx, HandleValue underlyingSource,
-                                               HandleValue size, HandleValue highWaterMark,
+                                               HandleValue size, double highWaterMark,
                                                HandleObject proto = nullptr);
     static ReadableStream* createExternalSourceStream(JSContext* cx, void* underlyingSource,
                                                       uint8_t flags, HandleObject proto = nullptr);
 
   private:
     static MOZ_MUST_USE ReadableStream* create(JSContext* cx, HandleObject proto = nullptr);
 
   public:
new file mode 100644
--- /dev/null
+++ b/js/src/tests/non262/ReadableStream/constructor-default.js
@@ -0,0 +1,25 @@
+// The second argument to `new ReadableStream` defaults to `{}`, so it observes
+// properties hacked onto Object.prototype.
+
+let log = [];
+
+Object.defineProperty(Object.prototype, "size", {
+    configurable: true,
+    get() {
+        log.push("size");
+        log.push(this);
+        return undefined;
+    }
+});
+Object.prototype.highWaterMark = 1337;
+
+let s = new ReadableStream({
+    start(controller) {
+        log.push("start");
+        log.push(controller.desiredSize);
+    }
+});
+assertDeepEq(log, ["size", {}, "start", 1337]);
+
+if (typeof reportCompare == "function")
+    reportCompare(0, 0);