Bug 1512008 - Fix assertion failure with ReadableStream and Promise[Symbol.species]. r=arai
authorJason Orendorff <jorendorff@mozilla.com>
Sun, 09 Dec 2018 22:24:22 +0000
changeset 449680 c0c4ff2b0485101a3f587d0a799841c0039c2b18
parent 449679 3d9e1dd87d4539957dbcfa5ff93ef5c2b3036e8f
child 449681 1b641c848a60987464e438095f426baa66702086
push idunknown
push userunknown
push dateunknown
reviewersarai
bugs1512008
milestone65.0a1
Bug 1512008 - Fix assertion failure with ReadableStream and Promise[Symbol.species]. r=arai Differential Revision: https://phabricator.services.mozilla.com/D13945
js/src/builtin/Promise.cpp
js/src/jit-test/tests/stream/bug-1512008.js
js/src/jsapi.cpp
js/src/jsapi.h
js/src/vm/Compartment-inl.h
--- a/js/src/builtin/Promise.cpp
+++ b/js/src/builtin/Promise.cpp
@@ -3339,18 +3339,27 @@ static bool PromiseThenNewPromiseCapabil
   return true;
 }
 
 // ES2016, 25.4.5.3., steps 3-5.
 MOZ_MUST_USE bool js::OriginalPromiseThen(
     JSContext* cx, HandleObject promiseObj, HandleValue onFulfilled,
     HandleValue onRejected, MutableHandleObject dependent,
     CreateDependentPromise createDependent) {
+  RootedValue promiseVal(cx, ObjectValue(*promiseObj));
   Rooted<PromiseObject*> promise(
-      cx, &CheckedUnwrap(promiseObj)->as<PromiseObject>());
+      cx,
+      UnwrapAndTypeCheckValue<PromiseObject>(cx, promiseVal, [cx, promiseObj] {
+        JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr,
+                                   JSMSG_INCOMPATIBLE_PROTO, "Promise", "then",
+                                   promiseObj->getClass()->name);
+      }));
+  if (!promise) {
+    return false;
+  }
 
   // Steps 3-4.
   Rooted<PromiseCapability> resultCapability(cx);
   if (!PromiseThenNewPromiseCapability(cx, promiseObj, createDependent,
                                        &resultCapability)) {
     return false;
   }
 
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/stream/bug-1512008.js
@@ -0,0 +1,6 @@
+Object.defineProperty(Promise, Symbol.species, {
+  value: function(g) {
+    g(function() {}, function() {})
+  }
+});
+new ReadableStream().tee();
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -4029,36 +4029,28 @@ JS_PUBLIC_API bool JS::ResolvePromise(JS
   return ResolveOrRejectPromise(cx, promiseObj, resolutionValue, false);
 }
 
 JS_PUBLIC_API bool JS::RejectPromise(JSContext* cx, JS::HandleObject promiseObj,
                                      JS::HandleValue rejectionValue) {
   return ResolveOrRejectPromise(cx, promiseObj, rejectionValue, true);
 }
 
-static bool CallOriginalPromiseThenImpl(
-    JSContext* cx, JS::HandleObject promiseObj, JS::HandleObject onResolvedObj_,
-    JS::HandleObject onRejectedObj_, JS::MutableHandleObject resultObj,
+static MOZ_MUST_USE bool CallOriginalPromiseThenImpl(
+    JSContext* cx, JS::HandleObject promiseObj, JS::HandleObject onFulfilledObj,
+    JS::HandleObject onRejectedObj, JS::MutableHandleObject resultObj,
     CreateDependentPromise createDependent) {
   AssertHeapIsIdle();
   CHECK_THREAD(cx);
-  cx->check(promiseObj, onResolvedObj_, onRejectedObj_);
-
-  MOZ_ASSERT_IF(onResolvedObj_, IsCallable(onResolvedObj_));
-  MOZ_ASSERT_IF(onRejectedObj_, IsCallable(onRejectedObj_));
-  RootedObject onResolvedObj(cx, onResolvedObj_);
-  RootedObject onRejectedObj(cx, onRejectedObj_);
-
-  if (IsWrapper(promiseObj) && !CheckedUnwrap(promiseObj)) {
-    ReportAccessDenied(cx);
-    return false;
-  }
-  MOZ_ASSERT(CheckedUnwrap(promiseObj)->is<PromiseObject>());
-
-  RootedValue onFulfilled(cx, ObjectOrNullValue(onResolvedObj));
+  cx->check(promiseObj, onFulfilledObj, onRejectedObj);
+
+  MOZ_ASSERT_IF(onFulfilledObj, IsCallable(onFulfilledObj));
+  MOZ_ASSERT_IF(onRejectedObj, IsCallable(onRejectedObj));
+
+  RootedValue onFulfilled(cx, ObjectOrNullValue(onFulfilledObj));
   RootedValue onRejected(cx, ObjectOrNullValue(onRejectedObj));
   return OriginalPromiseThen(cx, promiseObj, onFulfilled, onRejected, resultObj,
                              createDependent);
 }
 
 JS_PUBLIC_API JSObject* JS::CallOriginalPromiseThen(
     JSContext* cx, JS::HandleObject promiseObj, JS::HandleObject onResolvedObj,
     JS::HandleObject onRejectedObj) {
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -3451,39 +3451,43 @@ extern JS_PUBLIC_API bool ResolvePromise
 extern JS_PUBLIC_API bool RejectPromise(JSContext* cx,
                                         JS::HandleObject promiseObj,
                                         JS::HandleValue rejectionValue);
 
 /**
  * Calls the current compartment's original Promise.prototype.then on the
  * given `promise`, with `onResolve` and `onReject` passed as arguments.
  *
- * Asserts if the passed-in `promise` object isn't an unwrapped instance of
- * `Promise` or a subclass or `onResolve` and `onReject` aren't both either
- * `nullptr` or callable objects.
+ * Throws a TypeError if `promise` isn't a Promise (or possibly a different
+ * error if it's a security wrapper or dead object proxy).
+ *
+ * Asserts that `onFulfilled` and `onRejected` are each either callable or
+ * null.
  */
 extern JS_PUBLIC_API JSObject* CallOriginalPromiseThen(
-    JSContext* cx, JS::HandleObject promise, JS::HandleObject onResolve,
-    JS::HandleObject onReject);
+    JSContext* cx, JS::HandleObject promise, JS::HandleObject onFulfilled,
+    JS::HandleObject onRejected);
 
 /**
  * Unforgeable, optimized version of the JS builtin Promise.prototype.then.
  *
  * Takes a Promise instance and `onResolve`, `onReject` callables to enqueue
  * as reactions for that promise. In difference to Promise.prototype.then,
  * this doesn't create and return a new Promise instance.
  *
- * Asserts if the passed-in `promise` object isn't an unwrapped instance of
- * `Promise` or a subclass or `onResolve` and `onReject` aren't both callable
- * objects.
+ * Throws a TypeError if `promise` isn't a Promise (or possibly a different
+ * error if it's a security wrapper or dead object proxy).
+ *
+ * Asserts that `onFulfilled` and `onRejected` are each either callable or
+ * null.
  */
 extern JS_PUBLIC_API bool AddPromiseReactions(JSContext* cx,
                                               JS::HandleObject promise,
-                                              JS::HandleObject onResolve,
-                                              JS::HandleObject onReject);
+                                              JS::HandleObject onFulfilled,
+                                              JS::HandleObject onRejected);
 
 // This enum specifies whether a promise is expected to keep track of
 // information that is useful for embedders to implement user activation
 // behavior handling as specified in the HTML spec:
 // https://html.spec.whatwg.org/multipage/interaction.html#triggered-by-user-activation
 // By default, promises created by SpiderMonkey do not make any attempt to keep
 // track of information about whether an activation behavior was being processed
 // when the original promise in a promise chain was created.  If the embedder
--- a/js/src/vm/Compartment-inl.h
+++ b/js/src/vm/Compartment-inl.h
@@ -113,122 +113,107 @@ namespace detail {
  * Return the name of class T as a static null-terminated ASCII string constant
  * (for error messages).
  */
 template <class T>
 const char* ClassName() {
   return T::class_.name;
 }
 
-template <class T>
-MOZ_MUST_USE T* UnwrapAndTypeCheckThisSlowPath(JSContext* cx, HandleValue val,
-                                               const char* methodName) {
-  if (!val.isObject()) {
-    JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr,
-                               JSMSG_INCOMPATIBLE_PROTO, ClassName<T>(),
-                               methodName, InformalValueTypeName(val));
-    return nullptr;
-  }
-
-  JSObject* obj = &val.toObject();
-  if (IsWrapper(obj)) {
-    obj = CheckedUnwrap(obj);
-    if (!obj) {
-      ReportAccessDenied(cx);
-      return nullptr;
-    }
-  }
-
-  if (!obj->is<T>()) {
-    JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr,
-                               JSMSG_INCOMPATIBLE_PROTO, ClassName<T>(),
-                               methodName, InformalValueTypeName(val));
-    return nullptr;
-  }
-
-  return &obj->as<T>();
-}
-
-template <class T>
-MOZ_MUST_USE T* UnwrapAndTypeCheckArgumentSlowPath(JSContext* cx,
-                                                   CallArgs& args,
-                                                   const char* methodName,
-                                                   int argIndex) {
-  Value val = args.get(argIndex);
+template <class T, class ErrorCallback>
+MOZ_MUST_USE T* UnwrapAndTypeCheckValueSlowPath(JSContext* cx,
+                                                HandleValue value,
+                                                ErrorCallback throwTypeError) {
   JSObject* obj = nullptr;
-  if (val.isObject()) {
-    obj = &val.toObject();
+  if (value.isObject()) {
+    obj = &value.toObject();
     if (IsWrapper(obj)) {
       obj = CheckedUnwrap(obj);
       if (!obj) {
         ReportAccessDenied(cx);
         return nullptr;
       }
     }
   }
 
   if (!obj || !obj->is<T>()) {
-    ToCStringBuf cbuf;
-    if (char* numStr = NumberToCString(cx, &cbuf, argIndex + 1, 10)) {
-      JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr,
-                                 JSMSG_WRONG_TYPE_ARG, numStr, methodName,
-                                 ClassName<T>(), InformalValueTypeName(val));
-    }
+    throwTypeError();
     return nullptr;
   }
 
   return &obj->as<T>();
 }
 
 }  // namespace detail
 
 /**
- * Remove all wrappers from `val` and try to downcast the result to `T`.
+ * Remove all wrappers from `val` and try to downcast the result to class `T`.
+ *
+ * DANGER: The result may not be same-compartment with `cx`.
+ *
+ * This calls `throwTypeError` if the value isn't an object, cannot be
+ * unwrapped, or isn't an instance of the expected type. `throwTypeError` must
+ * in fact throw a TypeError (or OOM trying).
+ */
+template <class T, class ErrorCallback>
+inline MOZ_MUST_USE T* UnwrapAndTypeCheckValue(JSContext* cx, HandleValue value,
+                                               ErrorCallback throwTypeError) {
+  static_assert(!std::is_convertible<T*, Wrapper*>::value,
+                "T can't be a Wrapper type; this function discards wrappers");
+  cx->check(value);
+  if (value.isObject() && value.toObject().is<T>()) {
+    return &value.toObject().as<T>();
+  }
+  return detail::UnwrapAndTypeCheckValueSlowPath<T>(cx, value, throwTypeError);
+}
+
+/**
+ * Remove all wrappers from `args.thisv()` and try to downcast the result to
+ * class `T`.
  *
  * DANGER: The result may not be same-compartment with `cx`.
  *
  * This throws a TypeError if the value isn't an object, cannot be unwrapped,
  * or isn't an instance of the expected type.
  */
 template <class T>
 inline MOZ_MUST_USE T* UnwrapAndTypeCheckThis(JSContext* cx, CallArgs& args,
                                               const char* methodName) {
-  static_assert(!std::is_convertible<T*, Wrapper*>::value,
-                "T can't be a Wrapper type; this function discards wrappers");
-
   HandleValue thisv = args.thisv();
-  cx->check(thisv);
-  if (thisv.isObject() && thisv.toObject().is<T>()) {
-    return &thisv.toObject().as<T>();
-  }
-  return detail::UnwrapAndTypeCheckThisSlowPath<T>(cx, thisv, methodName);
+  return UnwrapAndTypeCheckValue<T>(cx, thisv, [cx, methodName, thisv] {
+    JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr,
+                               JSMSG_INCOMPATIBLE_PROTO, detail::ClassName<T>(),
+                               methodName, InformalValueTypeName(thisv));
+  });
 }
 
 /**
  * Remove all wrappers from `args[argIndex]` and try to downcast the result to
  * class `T`.
  *
  * DANGER: The result may not be same-compartment with `cx`.
  *
  * This throws a TypeError if the specified argument is missing, isn't an
  * object, cannot be unwrapped, or isn't an instance of the expected type.
  */
 template <class T>
 inline MOZ_MUST_USE T* UnwrapAndTypeCheckArgument(JSContext* cx, CallArgs& args,
                                                   const char* methodName,
                                                   int argIndex) {
-  static_assert(!std::is_convertible<T*, Wrapper*>::value,
-                "T can't be a Wrapper type; this function discards wrappers");
-
-  Value val = args.get(argIndex);
-  if (val.isObject() && val.toObject().is<T>()) {
-    return &val.toObject().as<T>();
-  }
-  return detail::UnwrapAndTypeCheckArgumentSlowPath<T>(cx, args, methodName,
-                                                       argIndex);
+  HandleValue val = args.get(argIndex);
+  return UnwrapAndTypeCheckValue<T>(cx, val, [cx, val, methodName, argIndex] {
+    ToCStringBuf cbuf;
+    if (char* numStr = NumberToCString(cx, &cbuf, argIndex + 1, 10)) {
+      JS_ReportErrorNumberLatin1(
+          cx, GetErrorMessage, nullptr, JSMSG_WRONG_TYPE_ARG, numStr,
+          methodName, detail::ClassName<T>(), InformalValueTypeName(val));
+    } else {
+      ReportOutOfMemory(cx);
+    }
+  });
 }
 
 /**
  * Unwrap an object of a known type.
  *
  * If `obj` is an object of class T, this returns a pointer to that object. If
  * `obj` is a wrapper for such an object, this tries to unwrap the object and
  * return a pointer to it. If access is denied, or `obj` was a wrapper but has