Bug 726949. Instead of using the given proto for the sandbox directly, use a proxy that forwards to the given proto but rebinds all getters/setters/methods to use the given proto, not the sandbox global, as this. r=bholley, a=tracking-firefox

The code in XPCQuickStubs.h just moved from XPCQuickStubs.cpp.

js/src/jsobj.cpp

@@ -3181,7 +3181,8 @@ CopySlots(JSContext *cx, JSObject *from, JSObject *to)
 
     size_t n = 0;
     if (from->isWrapper() &&
-        (Wrapper::wrapperHandler(from)->flags() & Wrapper::CROSS_COMPARTMENT)) {
+        (Wrapper::wrapperHandler(from)->flags() &
+         Wrapper::CROSS_COMPARTMENT)) {
         to->setSlot(0, from->getSlot(0));
         to->setSlot(1, from->getSlot(1));
         n = 2;

js/src/jswrapper.cpp

@@ -91,7 +91,7 @@ js::UnwrapObjectChecked(JSContext *cx, JSObject *obj)
 {
     while (obj->isWrapper()) {
         JSObject *wrapper = obj;
-        Wrapper *handler = Wrapper::wrapperHandler(obj);
+        AbstractWrapper *handler = AbstractWrapper::wrapperHandler(obj);
         bool rvOnFailure;
         if (!handler->enter(cx, wrapper, JSID_VOID,
                             Wrapper::PUNCTURE, &rvOnFailure))
@@ -112,11 +112,13 @@ js::IsCrossCompartmentWrapper(const JSObject *wrapper)
            !!(Wrapper::wrapperHandler(wrapper)->flags() & Wrapper::CROSS_COMPARTMENT);
 }
 
-AbstractWrapper::AbstractWrapper() : ProxyHandler(&sWrapperFamily)
+AbstractWrapper::AbstractWrapper(unsigned flags) :
+    ProxyHandler(&sWrapperFamily),
+    mFlags(flags)
 {
 }
 
-Wrapper::Wrapper(unsigned flags) : mFlags(flags)
+Wrapper::Wrapper(unsigned flags) : AbstractWrapper(flags)
 {
 }
 
@@ -394,10 +396,11 @@ AbstractWrapper::wrappedObject(const JSObject *wrapper)
     return GetProxyPrivate(wrapper).toObjectOrNull();
 }
 
-Wrapper *
+AbstractWrapper *
 AbstractWrapper::wrapperHandler(const JSObject *wrapper)
 {
-    return static_cast<Wrapper *>(GetProxyHandler(wrapper));
+    JS_ASSERT(wrapper->isWrapper());
+    return static_cast<AbstractWrapper *>(GetProxyHandler(wrapper));
 }
 
 bool

js/src/jswrapper.h

@@ -51,15 +51,18 @@ namespace js {
 
 class DummyFrameGuard;
 
-/* Base class that just implements no-op forwarding methods for funamental
+/* Base class that just implements no-op forwarding methods for fundamental
  * traps. This is meant to be used as a base class for ProxyHandlers that
  * want transparent forwarding behavior but don't want to use the derived
  * traps and other baggage of js::Wrapper.
  */
 class JS_FRIEND_API(AbstractWrapper) : public ProxyHandler
 {
+    unsigned mFlags;
   public:
-    explicit AbstractWrapper();
+    unsigned flags() const { return mFlags; }
+
+    explicit AbstractWrapper(unsigned flags);
 
     /* ES5 Harmony fundamental wrapper traps. */
     virtual bool getPropertyDescriptor(JSContext *cx, JSObject *wrapper, jsid id, bool set,
@@ -106,16 +109,13 @@ class JS_FRIEND_API(AbstractWrapper) : public ProxyHandler
     virtual void leave(JSContext *cx, JSObject *wrapper);
 
     static JSObject *wrappedObject(const JSObject *wrapper);
-    static Wrapper *wrapperHandler(const JSObject *wrapper);
+    static AbstractWrapper *wrapperHandler(const JSObject *wrapper);
 };
 
 /* No-op wrapper handler base class. */
 class JS_FRIEND_API(Wrapper) : public AbstractWrapper
 {
-    unsigned mFlags;
   public:
-    unsigned flags() const { return mFlags; }
-
     explicit Wrapper(unsigned flags);
 
     typedef enum { PermitObjectAccess, PermitPropertyAccess, DenyAccess } Permission;

js/xpconnect/src/XPCComponents.cpp

@@ -3066,6 +3066,93 @@ class Identity : public nsISupports
 
 NS_IMPL_ISUPPORTS0(Identity)
 
+xpc::SandboxProxyHandler xpc::sandboxProxyHandler;
+
+template<typename Op>
+bool BindPropertyOp(JSContext *cx, JSObject *targetObj, Op& op,
+                    PropertyDescriptor *desc, jsid id, unsigned attrFlag)
+{
+    if (!op) {
+        return true;
+    }
+
+    JSObject *func;
+    if (desc->attrs & attrFlag) {
+        // Already an object
+        func = JS_FUNC_TO_DATA_PTR(JSObject *, op);
+    } else {
+        // We have an actual property op.  For getters, we use 0
+        // args, for setters we use 1 arg.
+        uint32_t args = (attrFlag == JSPROP_GETTER) ? 0 : 1;
+        func = GeneratePropertyOp(cx, desc->obj, id, args, op);
+        if (!func)
+            return false;
+    }
+    func = JS_BindCallable(cx, func, targetObj);
+    if (!func)
+        return false;
+    op = JS_DATA_TO_FUNC_PTR(Op, func);
+    desc->attrs |= attrFlag;
+    return true;
+}
+
+bool
+xpc::SandboxProxyHandler::getPropertyDescriptor(JSContext *cx, JSObject *proxy,
+                                                jsid id, bool set,
+                                                PropertyDescriptor *desc)
+{
+    JSObject *obj = wrappedObject(proxy);
+    JS_ASSERT(js::GetObjectCompartment(obj) == js::GetObjectCompartment(proxy));
+    // XXXbz Not sure about the JSRESOLVE_QUALIFIED here, but we have
+    // no way to tell for sure whether to use it.
+    if (!JS_GetPropertyDescriptorById(cx, obj, id,
+                                      (set ? JSRESOLVE_ASSIGNING : 0) | JSRESOLVE_QUALIFIED,
+                                      desc))
+        return false;
+
+    if (!desc->obj)
+        return true; // No property, nothing to do
+
+    // Now fix up the getter/setter/value as needed to be bound to desc->obj
+    // Don't mess with holder_get and holder_set, though, because those rely on
+    // the "vp is prefilled with the value in the slot" behavior that property
+    // ops can in theory rely on, but our property op forwarder doesn't know how
+    // to make that happen.  Since we really only need to rebind the DOM methods
+    // here, not rebindings holder_get and holder_set is OK.
+    if (desc->getter != xpc::holder_get &&
+        !BindPropertyOp(cx, obj, desc->getter, desc, id, JSPROP_GETTER))
+        return false;
+    if (desc->setter != xpc::holder_set &&
+        !BindPropertyOp(cx, obj, desc->setter, desc, id, JSPROP_SETTER))
+        return false;
+    if (desc->value.isObject()) {
+        JSObject* val = &desc->value.toObject();
+        if (JS_ObjectIsCallable(cx, val)) {
+            val = JS_BindCallable(cx, val, obj);
+            if (!val)
+                return false;
+            desc->value = ObjectValue(*val);
+        }
+    }
+
+    return true;
+}
+
+bool
+xpc::SandboxProxyHandler::getOwnPropertyDescriptor(JSContext *cx,
+                                                   JSObject *proxy,
+                                                   jsid id, bool set,
+                                                   PropertyDescriptor *desc)
+{
+    if (!getPropertyDescriptor(cx, proxy, id, set, desc))
+        return false;
+
+    if (desc->obj != wrappedObject(proxy))
+        desc->obj = nsnull;
+
+    return true;
+}
+
 nsresult
 xpc_CreateSandboxObject(JSContext * cx, jsval * vp, nsISupports *prinOrSop, JSObject *proto,
                         bool wantXrays, const nsACString &sandboxName, nsISupports *identityPtr)
@@ -3134,6 +3221,20 @@ xpc_CreateSandboxObject(JSContext * cx, jsval * vp, nsISupports *prinOrSop, JSOb
                 proto = JSVAL_TO_OBJECT(v);
             }
 
+            // Now check what sort of thing we've got in |proto|
+            JSObject *unwrappedProto = js::UnwrapObject(proto, false);
+            js::Class *unwrappedClass = js::GetObjectClass(unwrappedProto);
+            if (IS_WRAPPER_CLASS(unwrappedClass) ||
+                mozilla::dom::bindings::IsDOMClass(Jsvalify(unwrappedClass))) {
+                // Wrap it up in a proxy that will do the right thing in terms
+                // of this-binding for methods.
+                proto = js::NewProxyObject(cx, &xpc::sandboxProxyHandler,
+                                           ObjectValue(*proto), nsnull,
+                                           sandbox);
+                if (!proto)
+                    return NS_ERROR_OUT_OF_MEMORY;
+            }
+
             ok = JS_SetPrototype(cx, sandbox, proto);
             if (!ok)
                 return NS_ERROR_XPC_UNEXPECTED;

js/xpconnect/src/XPCQuickStubs.cpp

@@ -116,54 +116,6 @@ LookupInterfaceOrAncestor(PRUint32 tableSize, const xpc_qsHashEntry *table,
     return entry;
 }
 
-// Apply |op| to |obj|, |id|, and |vp|. If |op| is a setter, treat the assignment as lenient.
-template<typename Op>
-static inline JSBool ApplyPropertyOp(JSContext *cx, Op op, JSObject *obj, jsid id, jsval *vp);
-
-template<>
-inline JSBool
-ApplyPropertyOp<JSPropertyOp>(JSContext *cx, JSPropertyOp op, JSObject *obj, jsid id, jsval *vp)
-{
-    return op(cx, obj, id, vp);
-}
-
-template<>
-inline JSBool
-ApplyPropertyOp<JSStrictPropertyOp>(JSContext *cx, JSStrictPropertyOp op, JSObject *obj,
-                                    jsid id, jsval *vp)
-{
-    return op(cx, obj, id, true, vp);
-}
-
-template<typename Op>
-static JSBool
-PropertyOpForwarder(JSContext *cx, unsigned argc, jsval *vp)
-{
-    // Layout:
-    //   this = our this
-    //   property op to call = callee reserved slot 0
-    //   name of the property = callee reserved slot 1
-
-    JSObject *callee = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp));
-    JSObject *obj = JS_THIS_OBJECT(cx, vp);
-    if (!obj)
-        return false;
-
-    jsval v = js::GetFunctionNativeReserved(callee, 0);
-
-    JSObject *ptrobj = JSVAL_TO_OBJECT(v);
-    Op *popp = static_cast<Op *>(JS_GetPrivate(ptrobj));
-
-    v = js::GetFunctionNativeReserved(callee, 1);
-
-    jsval argval = (argc > 0) ? JS_ARGV(cx, vp)[0] : JSVAL_VOID;
-    jsid id;
-    if (!JS_ValueToId(cx, argval, &id))
-        return false;
-    JS_SET_RVAL(cx, vp, argval);
-    return ApplyPropertyOp<Op>(cx, *popp, obj, id, vp);
-}
-
 static void
 PointerFinalize(JSFreeOp *fop, JSObject *obj)
 {
@@ -171,44 +123,13 @@ PointerFinalize(JSFreeOp *fop, JSObject *obj)
     delete popp;
 }
 
-static JSClass
+JSClass
 PointerHolderClass = {
     "Pointer", JSCLASS_HAS_PRIVATE,
     JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_StrictPropertyStub,
     JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, PointerFinalize
 };
 
-template<typename Op>
-static JSObject *
-GeneratePropertyOp(JSContext *cx, JSObject *obj, jsid id, unsigned argc, Op pop)
-{
-    // The JS engine provides two reserved slots on function objects for
-    // XPConnect to use. Use them to stick the necessary info here.
-    JSFunction *fun =
-        js::NewFunctionByIdWithReserved(cx, PropertyOpForwarder<Op>, argc, 0, obj, id);
-    if (!fun)
-        return nsnull;
-
-    JSObject *funobj = JS_GetFunctionObject(fun);
-
-    JS::AutoObjectRooter tvr(cx, funobj);
-
-    // Unfortunately, we cannot guarantee that Op is aligned. Use a
-    // second object to work around this.
-    JSObject *ptrobj = JS_NewObject(cx, &PointerHolderClass, nsnull, funobj);
-    if (!ptrobj)
-        return nsnull;
-    Op *popp = new Op;
-    if (!popp)
-        return nsnull;
-    *popp = pop;
-    JS_SetPrivate(ptrobj, popp);
-
-    js::SetFunctionNativeReserved(funobj, 0, OBJECT_TO_JSVAL(ptrobj));
-    js::SetFunctionNativeReserved(funobj, 1, js::IdToJsval(id));
-    return funobj;
-}
-
 static JSBool
 ReifyPropertyOps(JSContext *cx, JSObject *obj, jsid id, unsigned orig_attrs,
                  JSPropertyOp getter, JSStrictPropertyOp setter,

js/xpconnect/src/XPCQuickStubs.h

@@ -45,6 +45,8 @@
 
 #include "nsINode.h"
 
+#include "jsatom.h"
+
 /* XPCQuickStubs.h - Support functions used only by quick stubs. */
 
 class XPCCallContext;
@@ -680,4 +682,85 @@ xpc_qsSameResult(PRInt32 result1, PRInt32 result2)
 #define XPC_QS_ASSERT_CONTEXT_OK(cx) ((void) 0)
 #endif
 
+// Apply |op| to |obj|, |id|, and |vp|. If |op| is a setter, treat the assignment as lenient.
+template<typename Op>
+static inline JSBool ApplyPropertyOp(JSContext *cx, Op op, JSObject *obj, jsid id, jsval *vp);
+
+template<>
+inline JSBool
+ApplyPropertyOp<JSPropertyOp>(JSContext *cx, JSPropertyOp op, JSObject *obj, jsid id, jsval *vp)
+{
+    return op(cx, obj, id, vp);
+}
+
+template<>
+inline JSBool
+ApplyPropertyOp<JSStrictPropertyOp>(JSContext *cx, JSStrictPropertyOp op, JSObject *obj,
+                                    jsid id, jsval *vp)
+{
+    return op(cx, obj, id, true, vp);
+}
+
+template<typename Op>
+JSBool
+PropertyOpForwarder(JSContext *cx, unsigned argc, jsval *vp)
+{
+    // Layout:
+    //   this = our this
+    //   property op to call = callee reserved slot 0
+    //   name of the property = callee reserved slot 1
+
+    JSObject *callee = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp));
+    JSObject *obj = JS_THIS_OBJECT(cx, vp);
+    if (!obj)
+        return false;
+
+    jsval v = js::GetFunctionNativeReserved(callee, 0);
+
+    JSObject *ptrobj = JSVAL_TO_OBJECT(v);
+    Op *popp = static_cast<Op *>(JS_GetPrivate(ptrobj));
+
+    v = js::GetFunctionNativeReserved(callee, 1);
+
+    jsval argval = (argc > 0) ? JS_ARGV(cx, vp)[0] : JSVAL_VOID;
+    jsid id;
+    if (!JS_ValueToId(cx, v, &id))
+        return false;
+    JS_SET_RVAL(cx, vp, argval);
+    return ApplyPropertyOp<Op>(cx, *popp, obj, id, vp);
+}
+
+extern JSClass PointerHolderClass;
+
+template<typename Op>
+JSObject *
+GeneratePropertyOp(JSContext *cx, JSObject *obj, jsid id, unsigned argc, Op pop)
+{
+    // The JS engine provides two reserved slots on function objects for
+    // XPConnect to use. Use them to stick the necessary info here.
+    JSFunction *fun =
+        js::NewFunctionByIdWithReserved(cx, PropertyOpForwarder<Op>, argc, 0, obj, id);
+    if (!fun)
+        return nsnull;
+
+    JSObject *funobj = JS_GetFunctionObject(fun);
+
+    JS::AutoObjectRooter tvr(cx, funobj);
+
+    // Unfortunately, we cannot guarantee that Op is aligned. Use a
+    // second object to work around this.
+    JSObject *ptrobj = JS_NewObject(cx, &PointerHolderClass, nsnull, funobj);
+    if (!ptrobj)
+        return nsnull;
+    Op *popp = new Op;
+    if (!popp)
+        return nsnull;
+    *popp = pop;
+    JS_SetPrivate(ptrobj, popp);
+
+    js::SetFunctionNativeReserved(funobj, 0, OBJECT_TO_JSVAL(ptrobj));
+    js::SetFunctionNativeReserved(funobj, 1, js::IdToJsval(id));
+    return funobj;
+}
+
 #endif /* xpcquickstubs_h___ */

js/xpconnect/tests/chrome/Makefile.in

@@ -78,6 +78,7 @@ _CHROME_FILES = \
 		test_weakmaps.xul \
 		test_exnstack.xul \
 		test_weakref.xul \
+		test_bug726949.xul \
 		$(NULL)
 
 # Disabled until this test gets updated to test the new proxy based

js/xpconnect/tests/chrome/test_bug726949.xul

@@ -0,0 +1,37 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
+<?xml-stylesheet type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"?>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=726949
+-->
+<window title="Mozilla Bug 726949"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
+
+  <!-- test results are displayed in the html:body -->
+  <body xmlns="http://www.w3.org/1999/xhtml">
+  <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=726949"
+     target="_blank">Mozilla Bug 726949</a>
+  </body>
+
+  <!-- test code goes here -->
+  <script type="application/javascript">
+  <![CDATA[
+  /** Test for Bug 726949 **/
+      var Cu = Components.utils;
+      var s = new Cu.Sandbox(window, { sandboxPrototype: window } );
+      var t;
+      var desc;
+      try {
+          t = Cu.evalInSandbox('top', s);
+          is(t, window.top, "Should have gotten the right thing back");
+          desc = Cu.evalInSandbox('Object.getOwnPropertyDescriptor(Object.getPrototypeOf(this), "Cu")', s);
+          isnot(desc, undefined,
+		"Should have an own 'Cu' property");
+	  is(desc.value, Cu, "Should have the right value");
+      } catch (e) {
+          ok(false, "Should not get an exception: " + e);
+      }
+  ]]>
+  </script>
+</window>

js/xpconnect/wrappers/XrayWrapper.cpp

@@ -63,13 +63,6 @@ static const uint32_t JSSLOT_WN = 0;
 static const uint32_t JSSLOT_RESOLVING = 1;
 static const uint32_t JSSLOT_EXPANDO = 2;
 
-static JSBool
-holder_get(JSContext *cx, JSObject *holder, jsid id, jsval *vp);
-
-static JSBool
-holder_set(JSContext *cx, JSObject *holder, jsid id, JSBool strict, jsval *vp);
-
-
 static XPCWrappedNative *GetWrappedNative(JSObject *obj);
 
 namespace XrayUtils {
@@ -325,8 +318,14 @@ static inline JSObject *
 FindWrapper(JSObject *wrapper)
 {
     while (!js::IsWrapper(wrapper) ||
-           !(Wrapper::wrapperHandler(wrapper)->flags() & WrapperFactory::IS_XRAY_WRAPPER_FLAG)) {
-        wrapper = js::GetObjectProto(wrapper);
+           !(AbstractWrapper::wrapperHandler(wrapper)->flags() &
+             WrapperFactory::IS_XRAY_WRAPPER_FLAG)) {
+        if (js::IsWrapper(wrapper) &&
+            js::GetProxyHandler(wrapper) == &sandboxProxyHandler) {
+            wrapper = SandboxProxyHandler::wrappedObject(wrapper);
+        } else {
+            wrapper = js::GetObjectProto(wrapper);
+        }
         // NB: we must eventually hit our wrapper.
     }
 
@@ -368,7 +367,7 @@ XPCWrappedNativeXrayTraits::isResolving(JSContext *cx, JSObject *holder,
 // Some DOM objects have shared properties that don't have an explicit
 // getter/setter and rely on the class getter/setter. We install a
 // class getter/setter on the holder object to trigger them.
-static JSBool
+JSBool
 holder_get(JSContext *cx, JSObject *wrapper, jsid id, jsval *vp)
 {
     wrapper = FindWrapper(wrapper);
@@ -392,7 +391,7 @@ holder_get(JSContext *cx, JSObject *wrapper, jsid id, jsval *vp)
     return true;
 }
 
-static JSBool
+JSBool
 holder_set(JSContext *cx, JSObject *wrapper, jsid id, JSBool strict, jsval *vp)
 {
     wrapper = FindWrapper(wrapper);

js/xpconnect/wrappers/XrayWrapper.h

@@ -50,6 +50,11 @@ class XPCWrappedNative;
 
 namespace xpc {
 
+JSBool
+holder_get(JSContext *cx, JSObject *holder, jsid id, jsval *vp);
+JSBool
+holder_set(JSContext *cx, JSObject *holder, jsid id, JSBool strict, jsval *vp);
+
 namespace XrayUtils {
 
 extern JSClass HolderClass;
@@ -112,4 +117,18 @@ class XrayWrapper : public Base {
 typedef XrayWrapper<js::CrossCompartmentWrapper, ProxyXrayTraits > XrayProxy;
 typedef XrayWrapper<js::CrossCompartmentWrapper, DOMXrayTraits > XrayDOM;
 
+class SandboxProxyHandler : public js::AbstractWrapper {
+public:
+    SandboxProxyHandler() : js::AbstractWrapper(0)
+    {
+    }
+
+    virtual bool getPropertyDescriptor(JSContext *cx, JSObject *proxy, jsid id,
+                                       bool set, js::PropertyDescriptor *desc);
+    virtual bool getOwnPropertyDescriptor(JSContext *cx, JSObject *proxy,
+                                          jsid id, bool set,
+                                          js::PropertyDescriptor *desc);
+};
+
+extern SandboxProxyHandler sandboxProxyHandler;
 }