Bug 1170760 part 10. Add subclassing support to Promise::Resolve. r=baku,efaust

dom/bindings/Bindings.conf

@@ -1949,7 +1949,13 @@ DOMInterfaces = {
         # Keep this in sync with TestExampleInterface
         'headerFile': 'TestBindingHeader.h',
         'register': False
-        }
+        },
+
+'TestInterfaceWithPromiseConstructorArg' : {
+        'headerFile': 'TestBindingHeader.h',
+        'register': False,
+        },
+
 }
 
 # These are temporary, until they've been converted to use new DOM bindings

dom/bindings/Codegen.py

@@ -5093,14 +5093,94 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None,
             templateBody += 'static_assert(IsRefcounted<%s>::value, "We can only store refcounted classes.");' % typeName
 
         if isPromise:
+            # Per spec, what we're supposed to do is take the original
+            # Promise.resolve and call it with the original Promise as this
+            # value to make a Promise out of whatever value we actually have
+            # here.  The question is which global we should use.  There are
+            # several cases to consider:
+            #
+            # 1) Normal call to API with a Promise argument.  This is a case the
+            #    spec covers, and we should be using the current Realm's
+            #    Promise.  That means the current compartment.
+            # 2) Call to API with a Promise argument over Xrays.  In practice,
+            #    this sort of thing seems to be used for giving an API
+            #    implementation a way to wait for conclusion of an asyc
+            #    operation, _not_ to expose the Promise to content code.  So we
+            #    probably want to allow callers to use such an API in a
+            #    "natural" way, by passing chrome-side promises; indeed, that
+            #    may be all that the caller has to represent their async
+            #    operation.  That means we really need to do the
+            #    Promise.resolve() in the caller (chrome) compartment: if we do
+            #    it in the content compartment, we will try to call .then() on
+            #    the chrome promise while in the content compartment, which will
+            #    throw and we'll just get a rejected Promise.  Note that this is
+            #    also the reason why a caller who has a chrome Promise
+            #    representing an async operation can't itself convert it to a
+            #    content-side Promise (at least not without some serious
+            #    gyrations).
+            # 3) Promise return value from a callback or callback interface.
+            #    This is in theory a case the spec covers but in practice it
+            #    really doesn't define behavior here because it doesn't define
+            #    what Realm we're in after the callback returns, which is when
+            #    the argument conversion happens.  We will use the current
+            #    compartment, which is the compartment of the callable (which
+            #    may itself be a cross-compartment wrapper itself), which makes
+            #    as much sense as anything else. In practice, such an API would
+            #    once again be providing a Promise to signal completion of an
+            #    operation, which would then not be exposed to anyone other than
+            #    our own implementation code.
+            # 4) Return value from a JS-implemented interface.  In this case we
+            #    have a problem.  Our current compartment is the compartment of
+            #    the JS implementation.  But if the JS implementation returned
+            #    a page-side Promise (which is a totally sane thing to do, and
+            #    in fact the right thing to do given that this return value is
+            #    going right to content script) then we don't want to
+            #    Promise.resolve with our current compartment Promise, because
+            #    that will wrap it up in a chrome-side Promise, which is
+            #    decidedly _not_ what's desired here.  So in that case we
+            #    should really unwrap the return value and use the global of
+            #    the result.  CheckedUnwrap should be good enough for that; if
+            #    it fails, then we're failing unwrap while in a
+            #    system-privileged compartment, so presumably we have a dead
+            #    object wrapper.  Just error out.  Do NOT fall back to using
+            #    the current compartment instead: that will return a
+            #    system-privileged rejected (because getting .then inside
+            #    resolve() failed) Promise to the caller, which they won't be
+            #    able to touch.  That's not helpful.  If we error out, on the
+            #    other hand, they will get a content-side rejected promise.
+            #    Same thing if the value returned is not even an object.
+            if isCallbackReturnValue == "JSImpl":
+                # Case 4 above.  Note that globalObj defaults to the current
+                # compartment global.  Note that we don't use $*{exceptionCode}
+                # here because that will try to aRv.Throw(NS_ERROR_UNEXPECTED)
+                # which we don't really want here.
+                assert exceptionCode == "aRv.Throw(NS_ERROR_UNEXPECTED);\nreturn nullptr;\n"
+                getPromiseGlobal = fill(
+                    """
+                    if (!$${val}.isObject()) {
+                      aRv.ThrowTypeError<MSG_NOT_OBJECT>(NS_LITERAL_STRING("${sourceDescription}"));
+                      return nullptr;
+                    }
+                    JSObject* unwrappedVal = js::CheckedUnwrap(&$${val}.toObject());
+                    if (!unwrappedVal) {
+                      // A slight lie, but not much of one, for a dead object wrapper.
+                      aRv.ThrowTypeError<MSG_NOT_OBJECT>(NS_LITERAL_STRING("${sourceDescription}"));
+                      return nullptr;
+                    }
+                    globalObj = js::GetGlobalForObjectCrossCompartment(unwrappedVal);
+                    """,
+                    sourceDescription=sourceDescription)
+            else:
+                getPromiseGlobal = ""
+
             templateBody = fill(
                 """
-                { // Scope for our GlobalObject and ErrorResult
+                { // Scope for our GlobalObject, ErrorResult, JSAutoCompartment,
+                  // etc.
 
-                  // Might as well use CurrentGlobalOrNull here; that will at
-                  // least give us the same behavior as if the caller just called
-                  // Promise.resolve() themselves.
                   JS::Rooted<JSObject*> globalObj(cx, JS::CurrentGlobalOrNull(cx));
+                  $*{getPromiseGlobal}
+                  JSAutoCompartment ac(cx, globalObj);
                   GlobalObject promiseGlobal(cx, globalObj);
                   if (promiseGlobal.Failed()) {
                     $*{exceptionCode}
@@ -5112,12 +5192,25 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None,
                     $*{exceptionCode}
                   }
                   JS::Rooted<JS::Value> resolveThisv(cx, JS::ObjectValue(*promiseCtor));
-                  $${declName} = Promise::Resolve(promiseGlobal, resolveThisv, $${val}, promiseRv);
+                  JS::Rooted<JS::Value> resolveResult(cx);
+                  JS::Rooted<JS::Value> valueToResolve(cx, $${val});
+                  if (!JS_WrapValue(cx, &valueToResolve)) {
+                    $*{exceptionCode}
+                  }
+                  Promise::Resolve(promiseGlobal, resolveThisv, valueToResolve,
+                                   &resolveResult, promiseRv);
                   if (promiseRv.MaybeSetPendingException(cx)) {
                     $*{exceptionCode}
                   }
+                  nsresult unwrapRv = UNWRAP_OBJECT(Promise, &resolveResult.toObject(), $${declName});
+                  if (NS_FAILED(unwrapRv)) { // Quite odd
+                    promiseRv.Throw(unwrapRv);
+                    promiseRv.MaybeSetPendingException(cx);
+                    $*{exceptionCode}
+                  }
                 }
                 """,
+                getPromiseGlobal=getPromiseGlobal,
                 exceptionCode=exceptionCode)
         elif not descriptor.skipGen and not descriptor.interface.isConsequential() and not descriptor.interface.isExternal():
             if failureCode is not None:

dom/bindings/test/TestBindingHeader.h

@@ -1371,6 +1371,18 @@ public:
   virtual nsISupports* GetParentObject();
 };
 
+class TestInterfaceWithPromiseConstructorArg : public nsISupports, public nsWrapperCache
+{
+public:
+  NS_DECL_ISUPPORTS
+
+  static
+  already_AddRefed<TestInterfaceWithPromiseConstructorArg>
+    Constructor(const GlobalObject&, Promise&, ErrorResult&);
+
+  virtual nsISupports* GetParentObject();
+};
+
 } // namespace dom
 } // namespace mozilla
 

dom/bindings/test/TestCodeGen.webidl

@@ -55,6 +55,7 @@ callback interface TestCallbackInterface {
   void passVariadicNullableTypedArray(Float32Array?... arg);
   Uint8Array receiveUint8Array();
   attribute Uint8Array uint8ArrayAttr;
+  Promise<void> receivePromise();
 };
 
 callback interface TestSingleOperationCallbackInterface {
@@ -1034,6 +1035,9 @@ dictionary Dict : ParentDict {
 
   CustomEventInit customEventInit;
   TestDictionaryTypedef dictionaryTypedef;
+
+  Promise<void> promise;
+  sequence<Promise<void>> promiseSequence;
 };
 
 dictionary ParentDict : GrandparentDict {
@@ -1161,3 +1165,7 @@ interface TestDeprecatedInterface {
   static void alsoDeprecated();
 };
 
+
+[Constructor(Promise<void> promise)]
+interface TestInterfaceWithPromiseConstructorArg {
+};

dom/promise/Promise.cpp

@@ -1006,34 +1006,77 @@ Promise::NewPromiseCapability(JSContext* aCx, nsIGlobalObject* aGlobal,
   // Step 11 doesn't need anything, since the PromiseCapability was passed in.
 }
 
-/* static */ already_AddRefed<Promise>
+/* static */ void
 Promise::Resolve(const GlobalObject& aGlobal, JS::Handle<JS::Value> aThisv,
-                 JS::Handle<JS::Value> aValue, ErrorResult& aRv)
+                 JS::Handle<JS::Value> aValue,
+                 JS::MutableHandle<JS::Value> aRetval, ErrorResult& aRv)
 {
-  // If a Promise was passed, just return it.
-  if (aValue.isObject()) {
-    JS::Rooted<JSObject*> valueObj(aGlobal.Context(), &aValue.toObject());
-    Promise* nextPromise;
-    nsresult rv = UNWRAP_OBJECT(Promise, valueObj, nextPromise);
+  // Implementation of
+  // http://www.ecma-international.org/ecma-262/6.0/#sec-promise.resolve
 
-    if (NS_SUCCEEDED(rv)) {
-      RefPtr<Promise> addRefed = nextPromise;
-      return addRefed.forget();
-    }
-  }
+  JSContext* cx = aGlobal.Context();
 
   nsCOMPtr<nsIGlobalObject> global =
     do_QueryInterface(aGlobal.GetAsSupports());
   if (!global) {
     aRv.Throw(NS_ERROR_UNEXPECTED);
-    return nullptr;
+    return;
   }
 
-  RefPtr<Promise> p = Resolve(global, aGlobal.Context(), aValue, aRv);
-  if (p) {
-    p->mFullfillmentStack = p->mAllocationStack;
+  // Steps 1 and 2.
+  if (!aThisv.isObject()) {
+    aRv.ThrowTypeError<MSG_ILLEGAL_PROMISE_CONSTRUCTOR>();
+    return;
   }
-  return p.forget();
+
+  // Step 3.  If a Promise was passed and matches our constructor, just return it.
+  if (aValue.isObject()) {
+    JS::Rooted<JSObject*> valueObj(cx, &aValue.toObject());
+    Promise* nextPromise;
+    nsresult rv = UNWRAP_OBJECT(Promise, valueObj, nextPromise);
+
+    if (NS_SUCCEEDED(rv)) {
+      JS::Rooted<JS::Value> constructor(cx);
+      if (!JS_GetProperty(cx, valueObj, "constructor", &constructor)) {
+        aRv.NoteJSContextException();
+        return;
+      }
+
+      // Cheat instead of calling JS_SameValue, since we know one's an object.
+      if (aThisv == constructor) {
+        aRetval.setObject(*valueObj);
+        return;
+      }
+    }
+  }
+
+  // Step 4.
+  PromiseCapability capability(cx);
+  NewPromiseCapability(cx, global, aThisv, false, capability, aRv);
+  // Step 5.
+  if (aRv.Failed()) {
+    return;
+  }
+
+  // Step 6.
+  Promise* p = capability.mNativePromise;
+  if (p) {
+    p->MaybeResolveInternal(cx, aValue);
+    p->mFullfillmentStack = p->mAllocationStack;
+  } else {
+    JS::Rooted<JS::Value> value(cx, aValue);
+    JS::Rooted<JS::Value> ignored(cx);
+    if (!JS::Call(cx, JS::UndefinedHandleValue /* thisVal */,
+                  capability.mResolve, JS::HandleValueArray(value),
+                  &ignored)) {
+      // Step 7.
+      aRv.NoteJSContextException();
+      return;
+    }
+  }
+
+  // Step 8.
+  aRetval.set(capability.PromiseValue());
 }
 
 /* static */ already_AddRefed<Promise>

dom/promise/Promise.h

@@ -163,9 +163,10 @@ public:
   Constructor(const GlobalObject& aGlobal, PromiseInit& aInit,
               ErrorResult& aRv, JS::Handle<JSObject*> aDesiredProto);
 
-  static already_AddRefed<Promise>
+  static void
   Resolve(const GlobalObject& aGlobal, JS::Handle<JS::Value> aThisv,
-          JS::Handle<JS::Value> aValue, ErrorResult& aRv);
+          JS::Handle<JS::Value> aValue,
+          JS::MutableHandle<JS::Value> aRetval, ErrorResult& aRv);
 
   static already_AddRefed<Promise>
   Resolve(nsIGlobalObject* aGlobal, JSContext* aCx,

dom/promise/tests/test_bug883683.html

@@ -20,7 +20,7 @@ function runTest() {
   [{}, {}, {}, {}, {}].reduce(Promise.reject);
   ok(true, "No leaks with reject?");
 
-  [{}, {}, {}, {}, {}].reduce(Promise.resolve);
+  [{}, {}, {}, {}, {}].reduce(Promise.resolve.bind(Promise));
   ok(true, "No leaks with resolve?");
 
   [{}, {}, {}, {}, {}].reduce(function(a, b, c, d) { return new Promise(function(r1, r2) { throw a; }); });

dom/promise/tests/test_promise_xrays.html

@@ -173,6 +173,38 @@ function testAll5() {
   ).then(nextTest);
 }
 
+function testResolve1() {
+  var p = win.Promise.resolve(5);
+  ok(p instanceof win.Promise, "Promise.resolve should return a promise");
+  p.then(
+    function(arg) {
+      is(arg, 5, "Should get correct Promise.resolve value");
+    },
+    function(e) {
+      ok(false, "testAll5 threw exception: " + e);
+    }
+  ).then(nextTest);
+}
+
+function testResolve2() {
+  var p = win.Promise.resolve(5);
+  var q = win.Promise.resolve(p);
+  is(q, p, "Promise.resolve should just pass through Promise values");
+  nextTest();
+}
+
+function testResolve3() {
+  var p = win.Promise.resolve(Promise.resolve(5));
+  p.then(
+    function(arg) {
+      is(arg, 5, "Promise.resolve with chrome Promise should work");
+    },
+    function(e) {
+      ok(false, "Promise.resolve with chrome Promise should not fail");
+    }
+  ).then(nextTest);
+}
+
 var tests = [
   testLoadComplete,
   testHaveXray,
@@ -185,6 +217,9 @@ var tests = [
   testAll3,
   testAll4,
   testAll5,
+  testResolve1,
+  testResolve2,
+  testResolve3,
 ];
 
 function nextTest() {

dom/webidl/Promise.webidl

@@ -20,13 +20,11 @@ callback AnyCallback = any (any value);
  Exposed=(Window,Worker,System)]
 // Need to escape "Promise" so it's treated as an identifier.
 interface _Promise {
-  // Disable the static methods when the interface object is supposed to be
-  // disabled, just in case some code decides to walk over to .constructor from
-  // the proto of a promise object or someone screws up and manages to create a
-  // Promise object in this scope without having resolved the interface object
-  // first.
-  [NewObject]
-  static Promise<any> resolve(optional any value);
+  // Have to use "any" (or "object", but "any" is simpler) as the type to
+  // support the subclassing behavior, since nothing actually requires the
+  // return value of PromiseSubclass.resolve/reject to be a Promise object.
+  [NewObject, Throws]
+  static any resolve(optional any value);
   [NewObject]
   static Promise<void> reject(optional any value);
 

testing/web-platform/tests/js/builtins/Promise-subclassing.html

@@ -127,8 +127,10 @@ promise_test(function testPromiseRace() {
   var p = LoggingPromise.race(new LoggingIterable([1, 2]));
   var log = takeLog();
   assert_array_equals(log, ["Race 1", "Constructor 1",
-                            "Next 1", "Resolve 1",
-                            "Next 2", "Resolve 2",
+                            "Next 1", "Resolve 1", "Constructor 2",
+                            "Then 1",
+                            "Next 2", "Resolve 2", "Constructor 3",
+                            "Then 2",
                             "Next 3"]);
   assert_true(p instanceof LoggingPromise);
   return p.then(function(arg) {
@@ -141,8 +143,10 @@ promise_test(function testPromiseRaceNoSpecies() {
   var p = SpeciesLessPromise.race(new LoggingIterable([1, 2]));
   var log = takeLog();
   assert_array_equals(log, ["Race 1", "Constructor 1",
-                            "Next 1", "Resolve 1",
-                            "Next 2", "Resolve 2",
+                            "Next 1", "Resolve 1", "Constructor 2",
+                            "Then 1",
+                            "Next 2", "Resolve 2", "Constructor 3",
+                            "Then 2",
                             "Next 3"]);
   assert_true(p instanceof SpeciesLessPromise);
   return p.then(function(arg) {
@@ -155,8 +159,10 @@ promise_test(function testPromiseAll() {
   var p = LoggingPromise.all(new LoggingIterable([1, 2]));
   var log = takeLog();
   assert_array_equals(log, ["All 1", "Constructor 1",
-                            "Next 1", "Resolve 1",
-                            "Next 2", "Resolve 2",
+                            "Next 1", "Resolve 1", "Constructor 2",
+                            "Then 1",
+                            "Next 2", "Resolve 2", "Constructor 3",
+                            "Then 2",
                             "Next 3"]);
   assert_true(p instanceof LoggingPromise);
   return p.then(function(arg) {
@@ -164,4 +170,40 @@ promise_test(function testPromiseAll() {
   });
 }, "Promise.all behavior");
 
+promise_test(function testPromiseResolve() {
+  clearLog();
+  var p = LoggingPromise.resolve(5);
+  var log = takeLog();
+  assert_array_equals(log, ["Resolve 1", "Constructor 1"]);
+  var q = LoggingPromise.resolve(p);
+  assert_equals(p, q,
+                "Promise.resolve with same constructor should preserve identity");
+  log = takeLog();
+  assert_array_equals(log, ["Resolve 1"]);
+
+  var r = Promise.resolve(p);
+  assert_not_equals(p, r,
+                    "Promise.resolve with different constructor should " +
+                    "create a new Promise instance (1)")
+  var s = Promise.resolve(6);
+  var u = LoggingPromise.resolve(s);
+  log = takeLog();
+  assert_array_equals(log, ["Resolve 1", "Constructor 1"]);
+  assert_not_equals(s, u,
+                    "Promise.resolve with different constructor should " +
+                    "create a new Promise instance (2)")
+
+  Object.defineProperty(s, "constructor", { value: LoggingPromise });
+  var v = LoggingPromise.resolve(s);
+  log = takeLog();
+  assert_array_equals(log, ["Resolve 1"]);
+  assert_equals(v, s, "Faking the .constructor should work");
+  assert_false(v instanceof LoggingPromise);
+
+  var results = Promise.all([p, q, r, s, u, v]);
+  return results.then(function(arg) {
+    assert_array_equals(arg, [5, 5, 5, 6, 6, 6]);
+  });
+}, "Promise.resolve behavior");
+
 </script>

toolkit/components/asyncshutdown/tests/xpcshell/test_AsyncShutdown.js

@@ -148,7 +148,7 @@ add_task(function* test_phase_removeBlocker() {
 
     do_print("Attempt to remove a blocker after wait");
     lock = makeLock(kind);
-    blocker = Promise.resolve;
+    blocker = Promise.resolve.bind(Promise);
     yield lock.wait();
     do_remove_blocker(lock, blocker, false);