Bug 1436276. Bindings should create their return promises in the current compartment even when called over Xrays. r=bholley

These are cases that are implementing the "convert an exception to a Promise"
steps of the Web IDL spec.  Typically the exception is thrown in the current
compartment; the Promise returned should simply match that.  Otherwise we can
end up with a situation in which the promise doesn't actaully have access to
its rejection value, which will cause problems if someone uses then() on the
promise: the return value of the then() call will get a sanitized exception
instead of the real one.

We _could_ try to match the actual compartment of the exception, in theory.
But it's not clear why this would be preferable to using the current
compartment, even if there were cases in which the exception _doesn't_ match
the current compartment.  Which there likely are not.

MozReview-Commit-ID: Ac2BHIHxfvY

--HG--
rename : dom/promise/tests/test_promise_argument.html => dom/promise/tests/test_promise_retval.html
rename : dom/promise/tests/test_promise_argument_xrays.html => dom/promise/tests/test_promise_retval_xrays.html

dom/bindings/BindingUtils.cpp

@@ -2913,9 +2913,7 @@ GenericBindingGetter(JSContext* cx, unsigned argc, JS::Value* vp)
 bool
 GenericPromiseReturningBindingGetter(JSContext* cx, unsigned argc, JS::Value* vp)
 {
-  // Make sure to save the callee before someone maybe messes with rval().
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-  JS::Rooted<JSObject*> callee(cx, &args.callee());
 
   // We could invoke GenericBindingGetter here, but that involves an
   // extra call.  Manually inline it instead.
@@ -2923,8 +2921,7 @@ GenericPromiseReturningBindingGetter(JSContext* cx, unsigned argc, JS::Value* vp
   prototypes::ID protoID = static_cast<prototypes::ID>(info->protoID);
   if (!args.thisv().isObject()) {
     ThrowInvalidThis(cx, args, false, protoID);
-    return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                     args.rval());
+    return ConvertExceptionToPromise(cx, args.rval());
   }
   JS::Rooted<JSObject*> obj(cx, &args.thisv().toObject());
 
@@ -2941,8 +2938,7 @@ GenericPromiseReturningBindingGetter(JSContext* cx, unsigned argc, JS::Value* vp
     if (NS_FAILED(rv)) {
       ThrowInvalidThis(cx, args, rv == NS_ERROR_XPC_SECURITY_MANAGER_VETO,
                        protoID);
-      return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                       args.rval());
+      return ConvertExceptionToPromise(cx, args.rval());
     }
   }
   MOZ_ASSERT(info->type() == JSJitInfo::Getter);
@@ -2957,8 +2953,7 @@ GenericPromiseReturningBindingGetter(JSContext* cx, unsigned argc, JS::Value* vp
 
   // Promise-returning getters always return objects
   MOZ_ASSERT(info->returnType() == JSVAL_TYPE_OBJECT);
-  return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                   args.rval());
+  return ConvertExceptionToPromise(cx, args.rval());
 }
 
 bool
@@ -3044,9 +3039,7 @@ GenericBindingMethod(JSContext* cx, unsigned argc, JS::Value* vp)
 bool
 GenericPromiseReturningBindingMethod(JSContext* cx, unsigned argc, JS::Value* vp)
 {
-  // Make sure to save the callee before someone maybe messes with rval().
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-  JS::Rooted<JSObject*> callee(cx, &args.callee());
 
   // We could invoke GenericBindingMethod here, but that involves an
   // extra call.  Manually inline it instead.
@@ -3054,8 +3047,7 @@ GenericPromiseReturningBindingMethod(JSContext* cx, unsigned argc, JS::Value* vp
   prototypes::ID protoID = static_cast<prototypes::ID>(info->protoID);
   if (!args.thisv().isObject()) {
     ThrowInvalidThis(cx, args, false, protoID);
-    return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                     args.rval());
+    return ConvertExceptionToPromise(cx, args.rval());
   }
   JS::Rooted<JSObject*> obj(cx, &args.thisv().toObject());
 
@@ -3072,8 +3064,7 @@ GenericPromiseReturningBindingMethod(JSContext* cx, unsigned argc, JS::Value* vp
     if (NS_FAILED(rv)) {
       ThrowInvalidThis(cx, args, rv == NS_ERROR_XPC_SECURITY_MANAGER_VETO,
                        protoID);
-      return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                       args.rval());
+      return ConvertExceptionToPromise(cx, args.rval());
     }
   }
   MOZ_ASSERT(info->type() == JSJitInfo::Method);
@@ -3088,16 +3079,13 @@ GenericPromiseReturningBindingMethod(JSContext* cx, unsigned argc, JS::Value* vp
 
   // Promise-returning methods always return objects
   MOZ_ASSERT(info->returnType() == JSVAL_TYPE_OBJECT);
-  return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                   args.rval());
+  return ConvertExceptionToPromise(cx, args.rval());
 }
 
 bool
 StaticMethodPromiseWrapper(JSContext* cx, unsigned argc, JS::Value* vp)
 {
-  // Make sure to save the callee before someone maybe messes with rval().
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-  JS::Rooted<JSObject*> callee(cx, &args.callee());
 
   const JSJitInfo *info = FUNCTION_VALUE_TO_JITINFO(args.calleev());
   MOZ_ASSERT(info);
@@ -3108,18 +3096,13 @@ StaticMethodPromiseWrapper(JSContext* cx, unsigned argc, JS::Value* vp)
     return true;
   }
 
-  return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                   args.rval());
+  return ConvertExceptionToPromise(cx, args.rval());
 }
 
 bool
 ConvertExceptionToPromise(JSContext* cx,
-                          JSObject* promiseScope,
                           JS::MutableHandle<JS::Value> rval)
 {
-  {
-    JSAutoCompartment ac(cx, promiseScope);
-
   JS::Rooted<JS::Value> exn(cx);
   if (!JS_GetPendingException(cx, &exn)) {
     // This is very important: if there is no pending exception here but we're
@@ -3138,10 +3121,7 @@ ConvertExceptionToPromise(JSContext* cx,
   }
 
   rval.setObject(*promise);
-  }
-
-  // Now make sure we rewrap promise back into the compartment we want
-  return JS_WrapValue(cx, rval);
+  return true;
 }
 
 /* static */

dom/bindings/BindingUtils.h

@@ -3247,11 +3247,8 @@ StaticMethodPromiseWrapper(JSContext* cx, unsigned argc, JS::Value* vp);
 // simply be propagated.  Otherwise this method will attempt to convert the
 // exception to a Promise rejected with the exception that it will store in
 // rval.
-//
-// promiseScope should be the scope in which the Promise should be created.
 bool
 ConvertExceptionToPromise(JSContext* cx,
-                          JSObject* promiseScope,
                           JS::MutableHandle<JS::Value> rval);
 
 #ifdef DEBUG

dom/bindings/Codegen.py

@@ -8843,20 +8843,13 @@ class CGGenericPromiseReturningMethod(CGAbstractBindingMethod):
     def __init__(self, descriptor):
         unwrapFailureCode = dedent("""
             ThrowInvalidThis(cx, args, %%(securityError)s, "%s");\n
-            return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                             args.rval());\n""" %
+            return ConvertExceptionToPromise(cx, args.rval());\n""" %
                                    descriptor.interface.identifier.name)
 
         name = "genericPromiseReturningMethod"
-        customCallArgs = dedent("""
-            JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-            // Make sure to save the callee before someone maybe messes with rval().
-            JS::Rooted<JSObject*> callee(cx, &args.callee());
-        """)
 
         CGAbstractBindingMethod.__init__(self, descriptor, name,
                                          JSNativeArguments(),
-                                         callArgs=customCallArgs,
                                          unwrapFailureCode=unwrapFailureCode)
 
     def generate_code(self):
@@ -8873,8 +8866,7 @@ class CGGenericPromiseReturningMethod(CGAbstractBindingMethod):
             }
 
             MOZ_ASSERT(info->returnType() == JSVAL_TYPE_OBJECT);
-            return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                             args.rval());
+            return ConvertExceptionToPromise(cx, args.rval());
             """))
 
 
@@ -8920,15 +8912,11 @@ class CGMethodPromiseWrapper(CGAbstractStaticMethod):
     def definition_body(self):
         return fill(
             """
-            // Make sure to save the callee before someone maybe messes
-            // with rval().
-            JS::Rooted<JSObject*> callee(cx, &args.callee());
             bool ok = ${methodName}(${args});
             if (ok) {
               return true;
             }
-            return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                             args.rval());
+            return ConvertExceptionToPromise(cx, args.rval());
             """,
             methodName=self.method.name,
             args=", ".join(arg.name for arg in self.args))
@@ -9211,21 +9199,13 @@ class CGGenericPromiseReturningGetter(CGAbstractBindingMethod):
         unwrapFailureCode = fill(
             """
             ThrowInvalidThis(cx, args, %%(securityError)s, "${iface}");
-            return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                             args.rval());
+            return ConvertExceptionToPromise(cx, args.rval());
             """,
             iface=descriptor.interface.identifier.name)
         name = "genericPromiseReturningGetter"
-        customCallArgs = dedent(
-            """
-            JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-            // Make sure to save the callee before someone maybe messes with rval().
-            JS::Rooted<JSObject*> callee(cx, &args.callee());
-            """)
 
         CGAbstractBindingMethod.__init__(self, descriptor, name,
                                          JSNativeArguments(),
-                                         callArgs=customCallArgs,
                                          unwrapFailureCode=unwrapFailureCode)
 
     def generate_code(self):
@@ -9243,8 +9223,7 @@ class CGGenericPromiseReturningGetter(CGAbstractBindingMethod):
             }
 
             MOZ_ASSERT(info->returnType() == JSVAL_TYPE_OBJECT);
-            return ConvertExceptionToPromise(cx, xpc::XrayAwareCalleeGlobal(callee),
-                                             args.rval());
+            return ConvertExceptionToPromise(cx, args.rval());
             """))
 
 
@@ -9388,14 +9367,7 @@ class CGGetterPromiseWrapper(CGAbstractStaticMethod):
             if (ok) {
               return true;
             }
-            JS::Rooted<JSObject*> globalForPromise(cx);
-            // We can't use xpc::XrayAwareCalleeGlobal here because we have no
-            // callee.  Use our hacky version instead.
-            if (!xpc::XrayAwareCalleeGlobalForSpecializedGetters(cx, obj,
-                                                                 &globalForPromise)) {
-              return false;
-            }
-            return ConvertExceptionToPromise(cx, globalForPromise, args.rval());
+            return ConvertExceptionToPromise(cx, args.rval());
             """,
             getterName=self.getter.name,
             args=", ".join(arg.name for arg in self.args))

dom/bindings/test/TestFunctions.cpp

@@ -101,6 +101,13 @@ TestFunctions::TestThrowNsresultFromNative(ErrorResult& aError)
   aError = impl->TestThrowNsresultFromNative();
 }
 
+already_AddRefed<Promise>
+TestFunctions::ThrowToRejectPromise(GlobalObject& aGlobal, ErrorResult& aError)
+{
+  aError.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+  return nullptr;
+}
+
 bool
 TestFunctions::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto,
                           JS::MutableHandle<JSObject*> aWrapper)

dom/bindings/test/TestFunctions.h

@@ -42,6 +42,9 @@ public:
 
   void TestThrowNsresult(ErrorResult& aError);
   void TestThrowNsresultFromNative(ErrorResult& aError);
+  static already_AddRefed<Promise>
+  ThrowToRejectPromise(GlobalObject& aGlobal,
+                       ErrorResult& aError);
 
   bool WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto,
                   JS::MutableHandle<JSObject*> aWrapper);

dom/promise/tests/chrome.ini

@@ -8,3 +8,6 @@ support-files = file_promise_xrays.html
 [test_promise_argument_xrays.html]
 support-files = file_promise_xrays.html file_promise_argument_tests.js
 skip-if = debug == false
+[test_promise_retval_xrays.html]
+support-files = file_promise_xrays.html file_promise_retval_tests.js
+skip-if = debug == false

dom/promise/tests/file_promise_argument_tests.js

@@ -14,7 +14,7 @@
  * 4) A subframe (frames[0]) which can be used as a second global for creating
  *    promises.
  */
-var label = parent;
+var label = "parent";
 
 function passBasicPromise() {
   var p1 = Promise.resolve();

dom/promise/tests/file_promise_retval_tests.js

@@ -0,0 +1,41 @@
+/*
+ * This file is meant to provide common infrastructure for several consumers.
+ * The consumer is expected to define the following things:
+ *
+ * 1) A verifyPromiseGlobal function which does whatever test the consumer
+ *    wants.  This function is passed a promise and the global whose
+ *    TestFunctions was used to get the promise.
+ * 2) A expectedExceptionGlobal function which is handed the global whose
+ *    TestFunctions was used to trigger the exception and should return the
+ *    global the exception is expected to live in.
+ * 3) A subframe (frames[0]) which can be used as a second global for creating
+ *    promises.
+ */
+var label = "parent";
+
+function testThrownException(global) {
+  var p = global.TestFunctions.throwToRejectPromise();
+  verifyPromiseGlobal(p, global, "throwToRejectPromise return value");
+  return p.then(() => {}).catch((err) => {
+    var expected = expectedExceptionGlobal(global)
+    is(SpecialPowers.unwrap(SpecialPowers.Cu.getGlobalForObject(err)),
+       expected,
+       "Should have an exception object from the right global too");
+    ok(err instanceof expected.DOMException,
+       "Should have a DOMException here");
+    is(Object.getPrototypeOf(err), expected.DOMException.prototype,
+       "Should have a DOMException from the right global");
+    is(err.name, "InvalidStateError", "Should have the right DOMException");
+  });
+}
+
+function runPromiseRetvalTests(finishFunc) {
+  Promise.resolve()
+    .then(testThrownException.bind(undefined, window))
+    .then(testThrownException.bind(undefined, frames[0]))
+    .then(finishFunc)
+    .catch(function(e) {
+      ok(false, `Exception thrown: ${e}@${location.pathname}:${e.lineNumber}:${e.columnNumber}`);
+      finishFunc();
+    });
+}

dom/promise/tests/mochitest.ini

@@ -23,3 +23,6 @@ skip-if = debug == false
 [test_promise_callback_retval.html]
 support-files = file_promise_argument_tests.js
 skip-if = debug == false
+[test_promise_retval.html]
+support-files = file_promise_retval_tests.js
+skip-if = debug == false

dom/promise/tests/test_promise_argument_xrays.html

@@ -79,6 +79,7 @@ function nextTest() {
 }
 
 addLoadEvent(function() {
+  frames[0].label = "child";
   SpecialPowers.pushPrefEnv({set: [['dom.expose_test_interfaces', true]]},
                             nextTest);
 });

dom/promise/tests/test_promise_retval.html

@@ -0,0 +1,51 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1436276.
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1436276.</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script src="file_promise_retval_tests.js"></script>
+  <script type="application/javascript">
+  /** Test for Bug 1436276. **/
+  SimpleTest.waitForExplicitFinish();
+
+  function verifyPromiseGlobal(p, global, msg) {
+    // SpecialPowers.Cu.getGlobalForObject returns a SpecialPowers wrapper for
+    // the actual global.  We want to grab the underlying object.
+    var globalWrapper = SpecialPowers.Cu.getGlobalForObject(p);
+    is(SpecialPowers.unwrap(globalWrapper), global,
+       msg + " should come from " + global.label);
+  }
+
+  function expectedExceptionGlobal(global) {
+    // We should end up with an exception from "global".
+    return global;
+  }
+
+  function getPromise(global, arg) {
+    return global.TestFunctions.passThroughPromise(arg);
+  }
+
+  addLoadEvent(function() {
+    frames[0].label = "child";
+    SpecialPowers.pushPrefEnv({set: [['dom.expose_test_interfaces', true]]},
+                              runPromiseRetvalTests.bind(undefined,
+							 SimpleTest.finish));
+  });
+  </script>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1323324">Mozilla Bug 1323324</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <!-- A subframe so we have another global to work with -->
+  <iframe></iframe>
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>

dom/promise/tests/test_promise_retval_xrays.html

@@ -0,0 +1,94 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1436276.
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1436276.</title>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://global/skin"/>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1436276.">Mozilla Bug 1436276.</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+<iframe id="t" src="http://example.org/chrome/dom/promise/tests/file_promise_xrays.html"></iframe>
+</div>
+
+<pre id="test">
+<script src="file_promise_retval_tests.js"></script>
+<script type="application/javascript">
+
+var win = $("t").contentWindow;
+
+/** Test for Bug 1233324 **/
+SimpleTest.waitForExplicitFinish();
+
+function testLoadComplete() {
+  is(win.location.href, $("t").src, "Should have loaded the right thing");
+  nextTest();
+}
+
+function testHaveXray() {
+  is(typeof win.Promise.race, "function", "Should see a race() function");
+  var exception;
+  try {
+    win.Promise.wrappedJSObject.race;
+  } catch (e) {
+    exception = e;
+  }
+  is(exception, "Getting race", "Should have thrown the right exception");
+  is(win.wrappedJSObject.setupThrew, false, "Setup should not have thrown");
+  nextTest();
+}
+
+function verifyPromiseGlobal(p, _, msg) {
+  // SpecialPowers.Cu.getGlobalForObject returns a SpecialPowers wrapper for
+  // the actual global.  We want to grab the underlying object.
+  var global = SpecialPowers.unwrap(SpecialPowers.Cu.getGlobalForObject(p));
+
+  // We expect our global to always be "window" here, because we're working over
+  // Xrays.
+  is(global, window, msg + " should come from " + window.label);
+}
+
+function expectedExceptionGlobal(_) {
+  // We should end up with an exception from "window" no matter what global
+  // was involved to start with, because we're working over Xrays.
+  return window;
+}
+
+function getPromise(global, arg) {
+  return global.TestFunctions.passThroughPromise(arg);
+}
+
+function testPromiseRetvals() {
+  runPromiseRetvalTests(nextTest);
+}
+
+var tests = [
+  testLoadComplete,
+  testHaveXray,
+  testPromiseRetvals,
+];
+
+function nextTest() {
+  if (tests.length == 0) {
+    SimpleTest.finish();
+    return;
+  }
+  tests.shift()();
+}
+
+addLoadEvent(function() {
+  frames[0].label = "child";
+  SpecialPowers.pushPrefEnv({set: [['dom.expose_test_interfaces', true]]},
+                            nextTest);
+});
+
+</script>
+</pre>
+</body>
+</html>

dom/webidl/TestFunctions.webidl

@@ -44,4 +44,8 @@ interface TestFunctions {
   void testThrowNsresult();
   [Throws]
   void testThrowNsresultFromNative();
+
+  // Throws an InvalidStateError to auto-create a rejected promise.
+  [Throws]
+  static Promise<any> throwToRejectPromise();
 };

js/xpconnect/src/Sandbox.cpp

@@ -314,12 +314,11 @@ SandboxFetch(JSContext* cx, JS::HandleObject scope, const CallArgs& args)
 static bool SandboxFetchPromise(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
-    RootedObject callee(cx, &args.callee());
     RootedObject scope(cx, JS::CurrentGlobalOrNull(cx));
     if (SandboxFetch(cx, scope, args)) {
         return true;
     }
-    return ConvertExceptionToPromise(cx, scope, args.rval());
+    return ConvertExceptionToPromise(cx, args.rval());
 }