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 137981 64f001fe04fb86e0160e4b32cb8401527fed5648
parent 137980 57652d8f082735c1e81dba6836d1091994e3542a
child 137982 1df3bdadb7ce7dfd3dcabe990ec95bea41df50bb
push id2579
push userakeybl@mozilla.com
push dateMon, 24 Jun 2013 18:52:47 +0000
treeherdermozilla-beta@b69b7de8a05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs843829
milestone23.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 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 \