Bug 1430164. Stop doing weird sandbox-callable-wrapping for DOM constructors. r=bholley
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 12 Jan 2018 14:28:28 -0500
changeset 453439 8be5c5375a9010dbf91d415ccf5950a3c7c1a2c0
parent 453438 b8ded26d5d1960d2cb3cf8e46409643d811ba333
child 453440 9d7620e3e09673ffb57ba2c112c4004c6fbf50ac
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1430164
milestone59.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 1430164. Stop doing weird sandbox-callable-wrapping for DOM constructors. r=bholley This fixes DOM constructor identity in web extension content scripts to work as expected. MozReview-Commit-ID: Ab4sFWiMU6c
dom/bindings/BindingUtils.h
js/xpconnect/src/Sandbox.cpp
js/xpconnect/tests/chrome/chrome.ini
js/xpconnect/tests/chrome/test_bug1430164.html
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -2612,16 +2612,30 @@ inline bool
 UseDOMXray(JSObject* obj)
 {
   const js::Class* clasp = js::GetObjectClass(obj);
   return IsDOMClass(clasp) ||
          JS_IsNativeFunction(obj, Constructor) ||
          IsDOMIfaceAndProtoClass(clasp);
 }
 
+inline bool
+IsDOMConstructor(JSObject* obj)
+{
+  if (JS_IsNativeFunction(obj, dom::Constructor)) {
+    // NamedConstructor, like Image
+    return true;
+  }
+
+  const js::Class* clasp = js::GetObjectClass(obj);
+  // Check for a DOM interface object.
+  return dom::IsDOMIfaceAndProtoClass(clasp) &&
+         dom::DOMIfaceAndProtoJSClass::FromJSClass(clasp)->mType == dom::eInterface;
+}
+
 #ifdef DEBUG
 inline bool
 HasConstructor(JSObject* obj)
 {
   return JS_IsNativeFunction(obj, Constructor) ||
          js::GetObjectClass(obj)->getConstruct();
 }
 #endif
--- a/js/xpconnect/src/Sandbox.cpp
+++ b/js/xpconnect/src/Sandbox.cpp
@@ -751,16 +751,29 @@ bool WrapAccessorFunction(JSContext* cx,
     RootedObject func(cx, JS_FUNC_TO_DATA_PTR(JSObject*, op));
     func = WrapCallable(cx, func, sandboxProtoProxy);
     if (!func)
         return false;
     op = JS_DATA_TO_FUNC_PTR(Op, func.get());
     return true;
 }
 
+static bool
+IsMaybeWrappedDOMConstructor(JSObject* obj)
+{
+    // We really care about the underlying object here, which might be wrapped
+    // in cross-compartment wrappers.
+    obj = js::CheckedUnwrap(obj);
+    if (!obj) {
+        return false;
+    }
+
+    return dom::IsDOMConstructor(obj);
+}
+
 bool
 xpc::SandboxProxyHandler::getPropertyDescriptor(JSContext* cx,
                                                 JS::Handle<JSObject*> proxy,
                                                 JS::Handle<jsid> id,
                                                 JS::MutableHandle<PropertyDescriptor> desc) const
 {
     JS::RootedObject obj(cx, wrappedObject(proxy));
 
@@ -775,17 +788,21 @@ xpc::SandboxProxyHandler::getPropertyDes
     if (!WrapAccessorFunction(cx, desc.getter(), desc.address(),
                               JSPROP_GETTER, proxy))
         return false;
     if (!WrapAccessorFunction(cx, desc.setter(), desc.address(),
                               JSPROP_SETTER, proxy))
         return false;
     if (desc.value().isObject()) {
         RootedObject val (cx, &desc.value().toObject());
-        if (JS::IsCallable(val)) {
+        if (JS::IsCallable(val) &&
+            // Don't wrap DOM constructors: they don't care about the "this"
+            // they're invoked with anyway, being constructors.  And if we wrap
+            // them here we break invariants like Node == Node and whatnot.
+            !IsMaybeWrappedDOMConstructor(val)) {
             val = WrapCallable(cx, val, proxy);
             if (!val)
                 return false;
             desc.value().setObject(*val);
         }
     }
 
     return true;
--- a/js/xpconnect/tests/chrome/chrome.ini
+++ b/js/xpconnect/tests/chrome/chrome.ini
@@ -84,16 +84,17 @@ skip-if = os == 'win' || os == 'mac' || 
 [test_bug1050049.html]
 [test_bug1065185.html]
 [test_bug1074863.html]
 [test_bug1092477.xul]
 [test_bug1124898.html]
 [test_bug1126911.html]
 [test_bug1281071.xul]
 [test_bug1390159.xul]
+[test_bug1430164.html]
 [test_chrometoSource.xul]
 [test_cloneInto.xul]
 [test_cows.xul]
 [test_discardSystemSource.xul]
 [test_documentdomain.xul]
 [test_doublewrappedcompartments.xul]
 [test_evalInSandbox.xul]
 [test_evalInWindow.xul]
new file mode 100644
--- /dev/null
+++ b/js/xpconnect/tests/chrome/test_bug1430164.html
@@ -0,0 +1,32 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1430164
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1430164</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"/>
+  <script type="application/javascript">
+  /** Test for Bug 1430164 **/
+      var Cu = Components.utils;
+      var s = new Cu.Sandbox(window, { sandboxPrototype: window } );
+      var t;
+      var desc;
+      try {
+          t = Cu.evalInSandbox('Node === Node', s);
+          ok(t, "Object identity should be sane");
+
+          t = Cu.evalInSandbox('Node === this.Node', s);
+          ok(t, "Node should match this.Node in toplevel");
+
+          t = Cu.evalInSandbox('Node === window.Node', s);
+          ok(t, "Node should match window.Node");
+      } catch (e) {
+          ok(false, "Should not get an exception: " + e);
+      }
+  </script>
+</head>
+</html>