Bug 843829 - Explicitly add a waiver in FieldGetter and FieldSetter. r=mrbkap
☠☠ backed out by e60919ded783 ☠ ☠
authorBobby Holley <bobbyholley@gmail.com>
Tue, 02 Apr 2013 18:51:20 -0700
changeset 127132 64f001fe04fb
parent 127131 57652d8f0827
child 127133 1df3bdadb7ce
push id117
push usertomi.aarnio@nokia.com
push dateWed, 03 Apr 2013 12:07:07 +0000
reviewersmrbkap
bugs843829
milestone23.0a1
Bug 843829 - Explicitly add a waiver in FieldGetter and FieldSetter. r=mrbkap
content/xbl/src/nsXBLProtoImplField.cpp
js/xpconnect/wrappers/Makefile.in
--- a/content/xbl/src/nsXBLProtoImplField.cpp
+++ b/content/xbl/src/nsXBLProtoImplField.cpp
@@ -12,16 +12,17 @@
 #include "nsReadableUtils.h"
 #include "nsXBLProtoImplField.h"
 #include "nsIScriptContext.h"
 #include "nsIURI.h"
 #include "nsXBLSerialize.h"
 #include "nsXBLPrototypeBinding.h"
 #include "mozilla/dom/BindingUtils.h"
 #include "xpcpublic.h"
+#include "WrapperFactory.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 nsXBLProtoImplField::nsXBLProtoImplField(const PRUnichar* aName, const PRUnichar* aReadOnly)
   : mNext(nullptr),
     mFieldText(nullptr),
     mFieldTextLength(0),
@@ -246,18 +247,46 @@ FieldGetterImpl(JSContext *cx, JS::CallA
   }
   args.rval().set(v);
   return true;
 }
 
 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 JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldGetterImpl>
+  return xpc::WrapperFactory::WaiveXrayAndWrap(cx, args.mutableThisv().address()) &&
+         JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldGetterImpl>
                                  (cx, args);
 }
 
 bool
 FieldSetterImpl(JSContext *cx, JS::CallArgs args)
 {
   const JS::Value &thisv = args.thisv();
   MOZ_ASSERT(ValueHasISupportsPrivate(thisv));
@@ -286,17 +315,22 @@ FieldSetterImpl(JSContext *cx, JS::CallA
   args.rval().setUndefined();
   return true;
 }
 
 static JSBool
 FieldSetter(JSContext *cx, unsigned argc, JS::Value *vp)
 {
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-  return JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldSetterImpl>
+  // It's probably not actually necessary to waive Xray here given that
+  // 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);
 }
 
 nsresult
 nsXBLProtoImplField::InstallAccessors(JSContext* aCx,
                                       JSObject* aTargetClassObject)
 {
   MOZ_ASSERT(js::IsObjectInContextCompartment(aTargetClassObject, aCx));
--- a/js/xpconnect/wrappers/Makefile.in
+++ b/js/xpconnect/wrappers/Makefile.in
@@ -7,16 +7,19 @@ topsrcdir	= @top_srcdir@
 srcdir		= @srcdir@
 VPATH		= @srcdir@
 
 include $(DEPTH)/config/autoconf.mk
 
 LIBRARY_NAME	= xpcwrappers_s
 FORCE_STATIC_LIB = 1
 LIBXUL_LIBRARY = 1
+EXPORTS = \
+  WrapperFactory.h \
+  $(NULL)
 
 CPPSRCS = \
   AccessCheck.cpp \
   WaiveXrayWrapper.cpp \
   FilteringWrapper.cpp \
   ChromeObjectWrapper.cpp \
   XrayWrapper.cpp \
   WrapperFactory.cpp \