Backed out 6 changesets (bug 843829, bug 845862) because of broken mochitest-5

Backed out changeset 1df3bdadb7ce (bug 843829)
Backed out changeset 64f001fe04fb (bug 843829)
Backed out changeset 57652d8f0827 (bug 843829)
Backed out changeset 2e889cd77a48 (bug 843829)
Backed out changeset 97d16e7beb27 (bug 843829)
Backed out changeset 6c6ab0e54917 (bug 845862)

Landed on a CLOSED TREE

content/xbl/src/nsXBLProtoImplField.cpp

@@ -17,7 +17,6 @@
 #include "nsXBLPrototypeBinding.h"
 #include "mozilla/dom/BindingUtils.h"
 #include "xpcpublic.h"
-#include "WrapperFactory.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
@@ -252,36 +251,8 @@ FieldGetterImpl(JSContext *cx, JS::CallArgs args)
 static JSBool
 FieldGetter(JSContext *cx, unsigned argc, JS::Value *vp)
 {
-  // FieldGetter generally lives in the XBL scope, and is defined as a cross-
-  // compartment wrapper on the in-content XBL prototype object. When content
-  // accesses the field for the first time, it ends up invoking the wrapped
-  // FieldGetter on the prototype, which enters the XBL scope, landing us here.
-  // We then use the nativeCall machinery to re-enter the content compartment
-  // (unwrapping |this|), define the field on the in-content |this|, and return
-  // the value of the field to the caller.
-  //
-  // There's one hitch, though. When code in the XBL scope accesses a field on
-  // the content object, we waive the usual Xray vision granted to XBL scopes
-  // in order to do the access, because there isn't really anything else sane to
-  // do. In this sequence of events, the chrome caller invokes a get() for the
-  // field on the Xrayed element. XrayWrapper::get bounces to BaseProxyHandler::get,
-  // Which invokes XrayWrapper::getPropertyDescriptor. This detects the field
-  // access, creates a waived version of the wrapper, and does a lookup for the
-  // property on the waived wrapper. This would normally result in the resulting
-  // getter being transitively waived, which would cause said getter to properly
-  // waive Xray on its return value when it is eventually invoked (by the XBL
-  // scope) further down in BaseProxyHandler::get. However, this getter is
-  // FieldGetter, which actually lives in the XBL scope, meaning that we end up
-  // stripping all the wrappers off, effectively losing track of the fact that
-  // we meant to be waiving Xray here.
-  //
-  // Since fields are already doing this special Xray waiving stuff, the simplest
-  // solution seems to be to waive Xray on the |this| object before invoking
-  // CallNonGenericMethod. This means that the nativeCall trap of WaiveXrayWrapper
-  // will properly waive the result on the way back. Whew.
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-  return xpc::WrapperFactory::WaiveXrayAndWrap(cx, args.mutableThisv().address()) &&
+  return JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldGetterImpl>
-         JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldGetterImpl>
                                  (cx, args);
 }
 
@@ -320,12 +291,7 @@ static JSBool
 FieldSetter(JSContext *cx, unsigned argc, JS::Value *vp)
 {
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-  // It's probably not actually necessary to waive Xray here given that
+  return JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldSetterImpl>
-  // FieldSetter doesn't return everything, but it's good to maintain
-  // consistency with FieldGetter. See the comment there for more details on
-  // why we do this.
-  return xpc::WrapperFactory::WaiveXrayAndWrap(cx, args.mutableThisv().address()) &&
-         JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldSetterImpl>
                                  (cx, args);
 }
 

content/xbl/test/file_bug821850.xhtml

@@ -44,8 +44,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=821850
           is(bound.objectField.bar.a, 1, "Field Xrays work on objects");
           is(bound.contentField.foo, 10, "Field Xrays work on content objects");
           var hole = bound.contentField.rabbit.hole;
-          ok(hole.win === XPCNativeWrapper.unwrap(window), "Xray vision remains waived when hitting a native object");
+          is(hole.win, window, "We gain back Xray vision when hitting a native object");
-          ok(!Cu.isXrayWrapper(hole.win), "Xray is waived");
+          ok(Cu.isXrayWrapper(hole.win), "Really is Xray");
 
           // This gets invoked by an event handler.
           window.finish = function() {
@@ -71,20 +71,6 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=821850
             return "method:" + arg;
           </body>
         </method>
-        <method name="passMeAJSObject">
-          <parameter name="arg" />
-          <body>
-            is(typeof arg.prop, 'undefined', "No properties");
-            is(Object.getOwnPropertyNames(arg).length, 0, "Should have no own properties");
-            try {
-              arg.foo = 2;
-              ok(true, "Stuff fails silently");
-            } catch (e) {
-              ok(false, "Stuff should fail silently");
-            }
-            is(typeof arg.foo, 'undefined', "Shouldn't place props");
-          </body>
-        </method>
         <property name="prop">
           <getter>return this._prop;</getter>
           <setter>this._prop = "set:" + val;</setter>
@@ -134,14 +120,11 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=821850
     bound.prop = "someOtherVal";
     is(bound.prop, "set:someOtherVal", "Can set properties from content");
 
-    // Make sure we can't pass JS objects to the XBL scope.
-    var proto = bound.__proto__;
-    proto.passMeAJSObject({prop: 2});
-
     //
     // Try sticking a bunch of stuff on the prototype object.
     //
 
+    var proto = bound.__proto__;
     proto.someExpando = 201;
     is(bound.someExpando, 201, "Can stick non-XBL properties on the proto");
 

js/xpconnect/tests/unit/test_bug845862.js

@@ -1,13 +0,0 @@
-const Cu = Components.utils;
-
-function run_test() {
-  // We rely on the crazy "wantXrays:false also causes values return from the
-  // sandbox to be waived" behavior, because it's the simplest way to get
-  // waivers out of the sandbox (which has no native objects). :-(
-  var sb = new Cu.Sandbox('http://www.example.com', {wantXrays: false});
-  Cu.evalInSandbox("this.foo = {}; Object.defineProperty(foo, 'bar', {get: function() {return {};}});", sb);
-  do_check_true(sb.foo != XPCNativeWrapper(sb.foo), "sb.foo is waived");
-  var desc = Object.getOwnPropertyDescriptor(sb.foo, 'bar');
-  var b = desc.get();
-  do_check_true(b != XPCNativeWrapper(b), "results from accessor descriptors are waived");
-}

js/xpconnect/tests/unit/xpcshell.ini

@@ -17,7 +17,6 @@ tail =
 [test_bug809652.js]
 [test_bug813901.js]
 [test_bug845201.js]
-[test_bug845862.js]
 [test_bug849730.js]
 [test_bug851895.js]
 [test_bug854558.js]

js/xpconnect/wrappers/AccessCheck.h

@@ -53,30 +53,6 @@ struct Opaque : public Policy {
     }
 };
 
-// This policy is designed to protect privileged callers from untrusted non-
-// Xrayable objects. Nothing is allowed, and nothing throws.
-struct GentlyOpaque : public Policy {
-    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act) {
-        return false;
-    }
-    static bool deny(js::Wrapper::Action act) {
-        return true;
-    }
-    static bool allowNativeCall(JSContext *cx, JS::IsAcceptableThis test, JS::NativeImpl impl)
-    {
-        // We allow nativeCall here because the alternative is throwing (which
-        // happens in SecurityWrapper::nativeCall), which we don't want. There's
-        // unlikely to be too much harm to letting this through, because this
-        // wrapper is only used to wrap less-privileged objects in more-privileged
-        // scopes, so unwrapping here only drops privileges.
-        return true;
-    }
-
-    static bool isSafeToUnwrap() {
-        return false;
-    }
-};
-
 // This policy only permits access to the object if the subject can touch
 // system objects.
 struct OnlyIfSubjectIsSystem : public Policy {

js/xpconnect/wrappers/FilteringWrapper.cpp

@@ -143,30 +143,6 @@ FilteringWrapper<Base, Policy>::nativeCall(JSContext *cx, JS::IsAcceptableThis t
     return Base::Restrictive::nativeCall(cx, test, impl, args);
 }
 
-template <typename Base, typename Policy>
-bool
-FilteringWrapper<Base, Policy>::defaultValue(JSContext *cx, JS::Handle<JSObject*> obj,
-                                             JSType hint, MutableHandleValue vp)
-{
-    return Base::defaultValue(cx, obj, hint, vp);
-}
-
-// With our entirely-opaque wrapper, the DefaultValue algorithm throws,
-// causing spurious exceptions. Manually implement something benign.
-template<>
-bool
-FilteringWrapper<CrossCompartmentSecurityWrapper, GentlyOpaque>
-                ::defaultValue(JSContext *cx, JS::Handle<JSObject*> obj,
-                               JSType hint, MutableHandleValue vp)
-{
-    JSString *str = JS_NewStringCopyZ(cx, "[Opaque]");
-    if (!str)
-        return false;
-    vp.set(JS::StringValue(str));
-    return true;
-}
-
-
 template <typename Base, typename Policy>
 bool
 FilteringWrapper<Base, Policy>::enter(JSContext *cx, JS::Handle<JSObject*> wrapper,
@@ -203,7 +179,6 @@ FilteringWrapper<Base, Policy>::enter(JSContext *cx, JS::Handle<JSObject*> wrapp
 #define NNXOW FilteringWrapper<CrossCompartmentSecurityWrapper, Opaque>
 #define CW FilteringWrapper<SameCompartmentSecurityWrapper, ComponentsObjectPolicy>
 #define XCW FilteringWrapper<CrossCompartmentSecurityWrapper, ComponentsObjectPolicy>
-#define GO FilteringWrapper<CrossCompartmentSecurityWrapper, GentlyOpaque>
 template<> SOW SOW::singleton(WrapperFactory::SOW_FLAG);
 template<> SCSOW SCSOW::singleton(WrapperFactory::SOW_FLAG);
 template<> XOW XOW::singleton(0);
@@ -213,12 +188,9 @@ template<> NNXOW NNXOW::singleton(0);
 template<> CW CW::singleton(0);
 template<> XCW XCW::singleton(0);
 
-template<> GO GO::singleton(0);
-
 template class SOW;
 template class XOW;
 template class DXOW;
 template class NNXOW;
 template class ChromeObjectWrapperBase;
-template class GO;
 }

js/xpconnect/wrappers/FilteringWrapper.h

@@ -39,8 +39,6 @@ class FilteringWrapper : public Base {
     virtual bool nativeCall(JSContext *cx, JS::IsAcceptableThis test, JS::NativeImpl impl,
                             JS::CallArgs args) MOZ_OVERRIDE;
 
-    virtual bool defaultValue(JSContext *cx, JS::Handle<JSObject*> obj, JSType hint, JS::MutableHandleValue vp) MOZ_OVERRIDE;
-
     virtual bool enter(JSContext *cx, JS::Handle<JSObject*> wrapper, JS::Handle<jsid> id,
                        js::Wrapper::Action act, bool *bp) MOZ_OVERRIDE;
 

js/xpconnect/wrappers/Makefile.in

@@ -12,9 +12,6 @@ include $(DEPTH)/config/autoconf.mk
 LIBRARY_NAME	= xpcwrappers_s
 FORCE_STATIC_LIB = 1
 LIBXUL_LIBRARY = 1
-EXPORTS = \
-  WrapperFactory.h \
-  $(NULL)
 
 CPPSRCS = \
   AccessCheck.cpp \

js/xpconnect/wrappers/WaiveXrayWrapper.cpp

@@ -15,25 +15,6 @@
 
 namespace xpc {
 
-static bool
-WaiveAccessors(JSContext *cx, js::PropertyDescriptor *desc)
-{
-    if ((desc->attrs & JSPROP_GETTER) && desc->getter) {
-        JS::Value v = JS::ObjectValue(*JS_FUNC_TO_DATA_PTR(JSObject *, desc->getter));
-        if (!WrapperFactory::WaiveXrayAndWrap(cx, &v))
-            return false;
-        desc->getter = js::CastAsJSPropertyOp(&v.toObject());
-    }
-
-    if ((desc->attrs & JSPROP_SETTER) && desc->setter) {
-        JS::Value v = JS::ObjectValue(*JS_FUNC_TO_DATA_PTR(JSObject *, desc->setter));
-        if (!WrapperFactory::WaiveXrayAndWrap(cx, &v))
-            return false;
-        desc->setter = js::CastAsJSStrictPropertyOp(&v.toObject());
-    }
-    return true;
-}
-
 WaiveXrayWrapper::WaiveXrayWrapper(unsigned flags) : js::CrossCompartmentWrapper(flags)
 {
 }
@@ -48,7 +29,7 @@ WaiveXrayWrapper::getPropertyDescriptor(JSContext *cx, JS::Handle<JSObject*>wrap
                                         unsigned flags)
 {
     return CrossCompartmentWrapper::getPropertyDescriptor(cx, wrapper, id, desc, flags) &&
-           WrapperFactory::WaiveXrayAndWrap(cx, &desc->value) && WaiveAccessors(cx, desc);
+           WrapperFactory::WaiveXrayAndWrap(cx, &desc->value);
 }
 
 bool
@@ -57,7 +38,7 @@ WaiveXrayWrapper::getOwnPropertyDescriptor(JSContext *cx, JS::Handle<JSObject*>
                                            unsigned flags)
 {
     return CrossCompartmentWrapper::getOwnPropertyDescriptor(cx, wrapper, id, desc, flags) &&
-           WrapperFactory::WaiveXrayAndWrap(cx, &desc->value) && WaiveAccessors(cx, desc);
+           WrapperFactory::WaiveXrayAndWrap(cx, &desc->value);
 }
 
 bool
@@ -85,14 +66,4 @@ WaiveXrayWrapper::construct(JSContext *cx, JS::Handle<JSObject*> wrapper,
            WrapperFactory::WaiveXrayAndWrap(cx, rval.address());
 }
 
-// NB: This is important as the other side of a handshake with FieldGetter. See
-// nsXBLProtoImplField.cpp.
-bool
-WaiveXrayWrapper::nativeCall(JSContext *cx, JS::IsAcceptableThis test,
-                             JS::NativeImpl impl, JS::CallArgs args)
-{
-    return CrossCompartmentWrapper::nativeCall(cx, test, impl, args) &&
-           WrapperFactory::WaiveXrayAndWrap(cx, args.rval().address());
-}
-
 }

js/xpconnect/wrappers/WaiveXrayWrapper.h

@@ -36,9 +36,6 @@ class WaiveXrayWrapper : public js::CrossCompartmentWrapper {
                            unsigned argc, js::Value *argv,
                            JS::MutableHandle<JS::Value> rval) MOZ_OVERRIDE;
 
-    virtual bool nativeCall(JSContext *cx, JS::IsAcceptableThis test,
-                            JS::NativeImpl impl, JS::CallArgs args) MOZ_OVERRIDE;
-
     static WaiveXrayWrapper singleton;
 };
 

js/xpconnect/wrappers/WrapperFactory.cpp

@@ -293,10 +293,6 @@ DEBUG_CheckUnwrapSafety(JSObject *obj, js::Wrapper *handler,
     } else if (AccessCheck::needsSystemOnlyWrapper(obj)) {
         // SOWs have a dynamic unwrap check, so we can't really say anything useful
         // about them here :-(
-    } else if (handler == &FilteringWrapper<CrossCompartmentSecurityWrapper, GentlyOpaque>::singleton) {
-        // We explicitly use a SecurityWrapper to protect privileged callers from
-        // less-privileged objects that they should never see. Skip the check in
-        // this case.
     } else {
         // Otherwise, it should depend on whether the target subsumes the origin.
         MOZ_ASSERT(handler->isSafeToUnwrap() == AccessCheck::subsumes(target, origin));
@@ -365,7 +361,6 @@ WrapperFactory::Rewrap(JSContext *cx, JSObject *existing, JSObject *obj,
     bool targetSubsumesOrigin = AccessCheck::subsumes(target, origin);
     bool sameOrigin = targetSubsumesOrigin && originSubsumesTarget;
     XrayType xrayType = GetXrayType(obj);
-    bool waiveXrayFlag = flags & WAIVE_XRAY_WRAPPER_FLAG;
 
     // By default we use the wrapped proto of the underlying object as the
     // prototype for our wrapper, but we may select something different below.
@@ -400,20 +395,6 @@ WrapperFactory::Rewrap(JSContext *cx, JSObject *existing, JSObject *obj,
                                     OnlyIfSubjectIsSystem>::singleton;
     }
 
-    // Normally, a non-xrayable non-waived content object that finds itself in
-    // a privileged scope is wrapped with a CrossCompartmentWrapper, even though
-    // the lack of a waiver _really_ should give it an opaque wrapper. This is
-    // a bit too entrenched to change for content-chrome, but we can at least fix
-    // it for XBL scopes.
-    //
-    // See bug 843829.
-    else if (targetSubsumesOrigin && !originSubsumesTarget &&
-             !waiveXrayFlag && xrayType == NotXray &&
-             IsXBLScope(target))
-    {
-        wrapper = &FilteringWrapper<CrossCompartmentSecurityWrapper, GentlyOpaque>::singleton;
-    }
-
     //
     // Now, handle the regular cases.
     //
@@ -436,7 +417,8 @@ WrapperFactory::Rewrap(JSContext *cx, JSObject *existing, JSObject *obj,
 
         // If Xrays are warranted, the caller may waive them for non-security
         // wrappers.
-        bool waiveXrays = wantXrays && !securityWrapper && waiveXrayFlag;
+        bool waiveXrays = wantXrays && !securityWrapper &&
+                          (flags & WAIVE_XRAY_WRAPPER_FLAG);
 
         wrapper = SelectWrapper(securityWrapper, wantXrays, xrayType, waiveXrays);
     }

js/xpconnect/wrappers/XrayWrapper.cpp

@@ -1303,7 +1303,7 @@ XrayWrapper<Base, Traits>::~XrayWrapper()
 namespace XrayUtils {
 
 bool
-NeedsWaive(JSContext *cx, HandleObject wrapper, HandleId id)
+IsTransparent(JSContext *cx, HandleObject wrapper, HandleId id)
 {
     // We dynamically waive Xray vision for XBL bindings accessing fields
     // on bound elements, since there's no way to access such things sanely
@@ -1456,11 +1456,18 @@ XrayWrapper<Base, Traits>::getPropertyDescriptor(JSContext *cx, HandleObject wra
 
     typename Traits::ResolvingIdImpl resolving(wrapper, id);
 
-    if (XrayUtils::NeedsWaive(cx, wrapper, id)) {
+    // Redirect access straight to the wrapper if we should be transparent.
-        RootedObject waived(cx, WrapperFactory::WaiveXray(cx, wrapper));
+    if (XrayUtils::IsTransparent(cx, wrapper, id)) {
-        if (!waived || !JS_WrapObject(cx, waived.address()))
+        RootedObject obj(cx, Traits::getTargetObject(wrapper));
+        {
+            JSAutoCompartment ac(cx, obj);
+            if (!JS_GetPropertyDescriptorById(cx, obj, id, flags, desc))
                 return false;
-        return JS_GetPropertyDescriptorById(cx, waived, id, flags, desc);
+        }
+
+        if (desc->obj)
+            desc->obj = wrapper;
+        return JS_WrapPropertyDescriptor(cx, desc);
     }
 
     if (!holder)
@@ -1592,16 +1599,17 @@ XrayWrapper<Base, Traits>::getOwnPropertyDescriptor(JSContext *cx, HandleObject
 
     // NB: Nothing we do here acts on the wrapped native itself, so we don't
     // enter our policy.
+    // Redirect access straight to the wrapper if we should be transparent.
+    if (XrayUtils::IsTransparent(cx, wrapper, id)) {
+        RootedObject obj(cx, Traits::getTargetObject(wrapper));
+        {
+            JSAutoCompartment ac(cx, obj);
+            if (!JS_GetPropertyDescriptorById(cx, obj, id, flags, desc))
+                return false;
+        }
 
-    if (XrayUtils::NeedsWaive(cx, wrapper, id)) {
+        desc->obj = (desc->obj == obj) ? wrapper.get() : nullptr; // XXX
-        RootedObject waived(cx, WrapperFactory::WaiveXray(cx, wrapper));
+        return JS_WrapPropertyDescriptor(cx, desc);
-        if (!waived || !JS_WrapObject(cx, waived.address()))
-            return false;
-        if (!JS_GetPropertyDescriptorById(cx, waived, id, flags, desc))
-            return false;
-        if (desc->obj != waived)
-            desc->obj = nullptr;
-        return true;
     }
 
     if (!Traits::singleton.resolveOwnProperty(cx, *this, wrapper, holder, id, desc, flags))
@@ -1628,11 +1636,14 @@ XrayWrapper<Base, Traits>::defineProperty(JSContext *cx, HandleObject wrapper,
                                           HandleId id, PropertyDescriptor *desc)
 {
     assertEnteredPolicy(cx, wrapper, id);
-    if (XrayUtils::NeedsWaive(cx, wrapper, id)) {
+    // Redirect access straight to the wrapper if we should be transparent.
-        RootedObject waived(cx, WrapperFactory::WaiveXray(cx, wrapper));
+    if (XrayUtils::IsTransparent(cx, wrapper, id)) {
-        if (!waived || !JS_WrapObject(cx, waived.address()))
+        RootedObject obj(cx, Traits::getTargetObject(wrapper));
+        JSAutoCompartment ac(cx, obj);
+        if (!JS_WrapPropertyDescriptor(cx, desc))
             return false;
-        return JS_DefinePropertyById(cx, waived, id, desc->value, desc->getter, desc->setter,
+
+        return JS_DefinePropertyById(cx, obj, id, desc->value, desc->getter, desc->setter,
                                      desc->attrs);
     }
 
@@ -1687,6 +1698,19 @@ XrayWrapper<Base, Traits>::delete_(JSContext *cx, HandleObject wrapper,
                                    HandleId id, bool *bp)
 {
     assertEnteredPolicy(cx, wrapper, id);
+    // Redirect access straight to the wrapper if we should be transparent.
+    if (XrayUtils::IsTransparent(cx, wrapper, id)) {
+        RootedObject obj(cx, Traits::getTargetObject(wrapper));
+
+        JSAutoCompartment ac(cx, obj);
+
+        JSBool b;
+        RootedValue v(cx);
+        if (!JS_DeletePropertyById2(cx, obj, id, v.address()) || !JS_ValueToBoolean(cx, v, &b))
+            return false;
+        *bp = !!b;
+        return true;
+    }
 
     // Check the expando object.
     RootedObject target(cx, Traits::getTargetObject(wrapper));
@@ -1711,6 +1735,13 @@ XrayWrapper<Base, Traits>::enumerate(JSContext *cx, HandleObject wrapper, unsign
                                      AutoIdVector &props)
 {
     assertEnteredPolicy(cx, wrapper, JSID_VOID);
+    // Redirect access straight to the wrapper if we should be transparent.
+    if (XrayUtils::IsTransparent(cx, wrapper, JSID_VOIDHANDLE)) {
+        RootedObject obj(cx, Traits::getTargetObject(wrapper));
+        JSAutoCompartment ac(cx, obj);
+        return js::GetPropertyNames(cx, obj, flags, &props);
+    }
+
     if (!AccessCheck::wrapperSubsumes(wrapper)) {
         JS_ReportError(cx, "Not allowed to enumerate cross origin objects");
         return false;