Bug 1530292. Fix crash when cross-compartment WindowProxy is used as RHS of instanceof. r=jandem
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 26 Feb 2019 08:50:20 +0000
changeset 519073 fec419c7471bcd3d7c2a5cbb4670106323f3252c
parent 519072 bba070f5b4d3fad2020a63b1a646374d3052a856
child 519074 66d9e6a13efc5a05b57ee83be94d9194983fb442
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1530292
milestone67.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 1530292. Fix crash when cross-compartment WindowProxy is used as RHS of instanceof. r=jandem Differential Revision: https://phabricator.services.mozilla.com/D21035
dom/base/MaybeCrossOriginObject.cpp
dom/base/MaybeCrossOriginObject.h
dom/tests/mochitest/bugs/mochitest.ini
dom/tests/mochitest/bugs/test_bug1530292.html
--- a/dom/base/MaybeCrossOriginObject.cpp
+++ b/dom/base/MaybeCrossOriginObject.cpp
@@ -446,14 +446,44 @@ bool MaybeCrossOriginObject<Base>::enume
   JS::Rooted<JSObject*> self(cx, proxy);
   if (!MaybeWrapObject(cx, &self)) {
     return false;
   }
 
   return js::GetPropertyKeys(cx, self, 0, &props);
 }
 
+template <typename Base>
+bool MaybeCrossOriginObject<Base>::hasInstance(JSContext* cx,
+                                               JS::Handle<JSObject*> proxy,
+                                               JS::MutableHandle<JS::Value> v,
+                                               bool* bp) const {
+  if (!IsPlatformObjectSameOrigin(cx, proxy)) {
+    // In the cross-origin case we never have @@hasInstance, and we're never
+    // callable, so just go ahead and report an error.  If we enter the realm of
+    // "proxy" to do that, our caller won't be able to do anything with the
+    // exception, so instead let's wrap "proxy" into our realm.  We definitely
+    // do NOT want to call JS::InstanceofOperator here after entering "proxy's"
+    // realm, because that would do the wrong thing with @@hasInstance on the
+    // 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)) {
+    return false;
+  }
+  return JS::InstanceofOperator(cx, proxy, val, 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/base/MaybeCrossOriginObject.h
+++ b/dom/base/MaybeCrossOriginObject.h
@@ -314,16 +314,24 @@ class MaybeCrossOriginObject : public Ba
 
   /**
    * Spidermonkey-internal hook for enumerating objects.
    */
   bool enumerate(JSContext* cx, JS::Handle<JSObject*> proxy,
                  JS::AutoIdVector& props) const final;
 
   /**
+   * Spidermonkey-internal hook used for instanceof.  We need to override this
+   * because otherwise we can end up doing instanceof work in the wrong
+   * compartment.
+   */
+  bool hasInstance(JSContext* cx, JS::Handle<JSObject*> proxy,
+                   JS::MutableHandle<JS::Value> v, bool* bp) const final;
+
+  /**
    * Spidermonkey-internal hook used by Object.prototype.toString.  Subclasses
    * need to implement this, because we don't know what className they want.
    * Except in the cross-origin case, when we could maybe handle it...
    */
   const char* className(JSContext* cx,
                         JS::Handle<JSObject*> proxy) const override = 0;
 };
 
--- a/dom/tests/mochitest/bugs/mochitest.ini
+++ b/dom/tests/mochitest/bugs/mochitest.ini
@@ -143,10 +143,11 @@ skip-if = toolkit == 'android' || os == 
 skip-if = toolkit == 'android' #Windows can't change size on Android
 [test_toJSON.html]
 [test_window_bar.html]
 skip-if = toolkit == 'android'
 [test_bug1022869.html]
 [test_bug1112040.html]
 [test_bug1160342_marquee.html]
 [test_bug1171215.html]
+[test_bug1530292.html]
 [test_no_find_showDialog.html]
 skip-if = toolkit == 'android' # Bug 1358633 - window.find doesn't work for Android
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/bugs/test_bug1530292.html
@@ -0,0 +1,47 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1530292
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1530292</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script type="application/javascript">
+
+  /** Test for Bug 1530292 **/
+  SimpleTest.waitForExplicitFinish();
+  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");
+    }
+    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");
+    }
+    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>
+<div id="content" style="display: none">
+  <!-- Cross-origin but same-site; must be same-process -->
+  <iframe src="//test1.mochi.test:8888/tests/dom/tests/mochitest/bugs/file_empty.html"></iframe>
+  <!-- Completely cross-origin; might be cross-process -->
+  <iframe src="//example.org/tests/dom/tests/mochitest/bugs/file_empty.html"></iframe>
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>