Bug 1530413 - Inconsistent error reporting for non-function object RHS to instanceof, r=bzbarsky,tcampbell
authorAnny Gakhokidze <agakhokidze@mozilla.com>
Tue, 15 Oct 2019 19:55:17 +0000
changeset 497719 4c4f1cd4e616db8719816e7d57720869353b3918
parent 497718 dc242b12d4e2f5ed0043f7a3c4e20be1e21c88de
child 497720 5287a548f20b5d6adf3f06293578a563d32b692b
push id36695
push usercsabou@mozilla.com
push dateWed, 16 Oct 2019 05:20:00 +0000
treeherdermozilla-central@7b19086f802c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky, tcampbell
bugs1530413, 1530292
milestone71.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 1530413 - Inconsistent error reporting for non-function object RHS to instanceof, r=bzbarsky,tcampbell Additionally, this fixes test dom/tests/mochitest/bugs/test_bug1530292.html which fails if fission is enabled. Differential Revision: https://phabricator.services.mozilla.com/D47779
dom/base/MaybeCrossOriginObject.cpp
dom/tests/mochitest/bugs/mochitest.ini
dom/tests/mochitest/bugs/test_bug1530292.html
dom/tests/mochitest/bugs/test_instanceof_error_message.html
js/src/jit-test/tests/modules/instanceof-error-message.js
js/src/proxy/BaseProxyHandler.cpp
--- a/dom/base/MaybeCrossOriginObject.cpp
+++ b/dom/base/MaybeCrossOriginObject.cpp
@@ -474,24 +474,36 @@ bool MaybeCrossOriginObject<Base>::hasIn
     // object by seeing any such definitions when we should not.
     JS::Rooted<JS::Value> val(cx, JS::ObjectValue(*proxy));
     if (!MaybeWrapValue(cx, &val)) {
       return false;
     }
     return js::ReportIsNotFunction(cx, val);
   }
 
-  // Safe to enter the realm of "proxy" and do the normal thing of looking up
-  // @@hasInstance, etc.
-  JSAutoRealm ar(cx, proxy);
-  JS::Rooted<JS::Value> val(cx, v);
-  if (!MaybeWrapValue(cx, &val)) {
+  // We need to wrap `proxy` into our compartment or enter proxy's realm
+  // and wrap `v` into proxy's compartment because at this point `v` and `proxy`
+  // might no longer be same-compartment. One solution is to enter the realm of
+  // `proxy` and look up @@hasInstance there. However, that will lead to
+  // incorrect error reporting because the mechanism for reporting the "not a
+  // function" exception only works correctly if we are in the realm of the
+  // script that encountered the instanceof expression. Thus, we don't want to
+  // switch realms and will wrap `proxy` into our current compartment and lookup
+  // @@hasInstance. Note that accesses to get @@hasInstance on `proxy` after it
+  // is wrapped in the `cx` compartment will still work because `cx` and `proxy`
+  // are same-origin.
+  JS::Rooted<JSObject*> proxyWrap(cx, proxy);
+  if (!MaybeWrapObject(cx, &proxyWrap)) {
     return false;
   }
-  return JS::InstanceofOperator(cx, proxy, val, bp);
+  // We are not calling BaseProxyHandler::hasInstance here because it expects
+  // `proxy` to be passed as the object. However, `proxy`, as a
+  // MaybeCrossOriginObject, may not be in current cx->realm() and we may now
+  // have a cross-compartment wrapper for `proxy`.
+  return JS::InstanceofOperator(cx, proxyWrap, v, bp);
 }
 
 // Force instantiations of the out-of-line template methods we need.
 template class MaybeCrossOriginObject<js::Wrapper>;
 template class MaybeCrossOriginObject<DOMProxyHandler>;
 
 }  // namespace dom
 }  // namespace mozilla
--- a/dom/tests/mochitest/bugs/mochitest.ini
+++ b/dom/tests/mochitest/bugs/mochitest.ini
@@ -152,12 +152,12 @@ skip-if = toolkit == 'android' #Windows 
 [test_window_bar.html]
 skip-if = toolkit == 'android'
 [test_bug1022869.html]
 [test_bug1112040.html]
 [test_bug1160342_marquee.html]
 [test_bug1171215.html]
 support-files = window_bug1171215.html
 [test_bug1530292.html]
-fail-if = fission
+[test_instanceof_error_message.html]
 [test_bug467035.html]
 [test_no_find_showDialog.html]
 skip-if = toolkit == 'android' # Bug 1358633 - window.find doesn't work for Android
--- a/dom/tests/mochitest/bugs/test_bug1530292.html
+++ b/dom/tests/mochitest/bugs/test_bug1530292.html
@@ -15,23 +15,23 @@ https://bugzilla.mozilla.org/show_bug.cg
   addLoadEvent(() => {
     // This is really a crashtest, but we need some cross-origin (and hence
     // cross-compartment) windows to test it, so can't use the crashtest
     // harness.
     try {
       ({} instanceof frames[0]);
       ok(false, "Should have thrown for same-process window");
     } catch (e) {
-      is(e.message, "frames[0] is not a function");
+      is(e.message, "frames[0] is not a function", "frames[0] is not a function");
     }
     try {
       ({} instanceof frames[1]);
       ok(false, "Should have thrown for maybe other-process window");
     } catch (e) {
-      is(e.message, "frames[1] is not a function");
+      is(e.message, "frames[1] is not a function", "frames[1] is not a function");
     }
     SimpleTest.finish();
   });
   </script>
 </head>
 <body>
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1530292">Mozilla Bug 1530292</a>
 <p id="display"></p>
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/bugs/test_instanceof_error_message.html
@@ -0,0 +1,111 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1530413
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1530413</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script type="application/javascript">
+  add_task(() => {
+    var Cu = SpecialPowers.Cu;
+    try {
+      // Test case 1
+      ({} instanceof {});
+      ok(false, "Should throw a type error");
+    } catch (e) {
+      is(e.message, "({}) is not a function", "We should get a correct error message when calling instanceof");
+    }
+    // Test case 2
+    try {
+      ({} instanceof document);
+      ok(false, "Should throw a type error");
+    } catch (e) {
+      is(e.message, "document is not a function", "We should get a correct error message when calling instanceof");
+    }
+    // Test case 3
+    try {
+      ({} instanceof new Proxy(document, {}));
+      ok(false, "Should throw a type error");
+    } catch (e) {
+      is(e.message, "(new Proxy(...)) is not a function", "We should get a correct error message when calling instanceof");
+    }
+    // Test case 4 - Verify invoking instanceof on an object from a different compartment yields the same error
+    var sandbox = SpecialPowers.unwrap(Cu.Sandbox(this, { sameZoneAs: this, freshCompartment: true, wantXrays: false }));
+    sandbox.window = window;
+    sandbox.crossCompartmentObject = {}; // object created in the test compartment
+    try {
+      Cu.evalInSandbox("({} instanceof window);", sandbox);
+      ok(false, "Should throw a type error");
+    } catch (e) {
+      is(e.message, "window is not a function", "We should get a correct error message when calling instanceof");
+    }
+
+    // Test case 5 - Verify we get the same error when the LHS is an object created in a different compartment
+    try {
+      Cu.evalInSandbox("(crossCompartmentObject instanceof window);", sandbox);
+      ok(false, "Should throw a type error");
+    } catch (e) {
+      is(e.message, "window is not a function", "We should get a correct error message when calling instanceof");
+    }
+
+    // Test case 6 - Test that we are correctly wrapping the window into sandbox's compartment
+    window[Symbol.hasInstance] = function(instance) {
+      instance.window = this;
+      return true;
+    }
+    var x = Cu.evalInSandbox("(crossCompartmentObject instanceof window);", sandbox);
+    ok(x, "Symbol.hasInstance for window should return true");
+    is(sandbox.crossCompartmentObject.window, window, "We shouldn't leak the window");
+    delete window[Symbol.hasInstance];
+    Cu.nukeSandbox(sandbox);
+
+    // Test case 7 - Test instanceof with RHS being a same-origin Xray to a Window
+    sandbox = SpecialPowers.unwrap(Cu.Sandbox(this, { sameZoneAs: this, freshCompartment: true}));
+    sandbox.window = window;
+    sandbox.crossCompartmentObject = {};
+
+    window[Symbol.hasInstance] = function(instance) {
+      instance.window = this;
+      return true;
+    }
+    try {
+      Cu.evalInSandbox("(crossCompartmentObject instanceof window);", sandbox);
+      ok(false, "Should throw a type error");
+    } catch (e) {
+      is(e.message, "window is not a function",
+        "We should get a correct error thrown when the RHS of instanceof is an Xray to a Window.");
+    }
+    delete window[Symbol.hasInstance];
+    Cu.nukeSandbox(sandbox);
+
+    // Test case 8 - Test instanceof with RHS being a same-origin Xray waiver
+    sandbox = SpecialPowers.unwrap(Cu.Sandbox(this, { sameZoneAs: this, freshCompartment: true}));
+    sandbox.window = window;
+    sandbox.crossCompartmentObject = {};
+    sandbox.waiveXrays = Cu.waiveXrays;
+
+    window[Symbol.hasInstance] = function(instance) {
+      instance.window = this;
+      return true;
+    }
+    Cu.evalInSandbox("(crossCompartmentObject instanceof waiveXrays(window));", sandbox);
+    ok(x, "Symbol.hasInstance for window should return true");
+    is(sandbox.crossCompartmentObject.window, window,
+      "The window pointed to by the crossCompartmentObject should be the same as the window in our compartment");
+    delete window[Symbol.hasInstance];
+    Cu.nukeSandbox(sandbox);
+  });
+  </script>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1530413">Mozilla Bug 1530413</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>
\ No newline at end of file
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/modules/instanceof-error-message.js
@@ -0,0 +1,14 @@
+function getInstanceOfErrorMessage(x) {
+    try {
+        var result = {} instanceof x;
+    }
+    catch (e) {
+        return e.message;
+    }
+}
+
+// Error message for a Module Namespace Exotic Object should be same as normal
+// non-callable when using 'instanceof'
+import('empty.js').then(
+    m => assertEq(getInstanceOfErrorMessage(m),
+                  getInstanceOfErrorMessage({})));
--- a/js/src/proxy/BaseProxyHandler.cpp
+++ b/js/src/proxy/BaseProxyHandler.cpp
@@ -321,20 +321,18 @@ bool BaseProxyHandler::nativeCall(JSCont
                                   NativeImpl impl, const CallArgs& args) const {
   ReportIncompatible(cx, args);
   return false;
 }
 
 bool BaseProxyHandler::hasInstance(JSContext* cx, HandleObject proxy,
                                    MutableHandleValue v, bool* bp) const {
   assertEnteredPolicy(cx, proxy, JSID_VOID, GET);
-  RootedValue val(cx, ObjectValue(*proxy.get()));
-  ReportValueError(cx, JSMSG_BAD_INSTANCEOF_RHS, JSDVG_SEARCH_STACK, val,
-                   nullptr);
-  return false;
+  cx->check(proxy, v);
+  return JS::InstanceofOperator(cx, proxy, v, bp);
 }
 
 bool BaseProxyHandler::getBuiltinClass(JSContext* cx, HandleObject proxy,
                                        ESClass* cls) const {
   *cls = ESClass::Other;
   return true;
 }