Bug 792890. Fix JS-wrapping of callback interfaces to just return the underlying JS object. r=peterv
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 10 Oct 2012 15:57:57 -0400
changeset 109880 2803709dc340deb32dc82ac4d67433cf01f7b00b
parent 109879 c24f40ee97cb316c5a9e21e3aa3b6fe6c282d690
child 109881 f6753216c72b0a96e12954effbb745bb4a40e531
push id16254
push userbzbarsky@mozilla.com
push dateWed, 10 Oct 2012 19:58:50 +0000
treeherdermozilla-inbound@2803709dc340 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv
bugs792890
milestone19.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 792890. Fix JS-wrapping of callback interfaces to just return the underlying JS object. r=peterv There are several changes here: 1) When wrapping a callback interface object for JS, just extract the underlying JSObject from inside it and hand that object out. 2) Flag callback interface descriptors as "not concrete" (only matters for cases when they have constants on the interface object) and not wrappercached (will catch bugs if someone tries to treat them as a Gecko object). 3) Fix a preexisting bug in sequence wrapping where we'd try to JS_DefineElement twice if we were wrapping a null value for a sequence of nullable interface objects.
dom/bindings/BindingUtils.cpp
dom/bindings/BindingUtils.h
dom/bindings/Bindings.conf
dom/bindings/Codegen.py
dom/bindings/Configuration.py
dom/bindings/test/TestBindingHeader.h
dom/bindings/test/TestCodeGen.webidl
--- a/dom/bindings/BindingUtils.cpp
+++ b/dom/bindings/BindingUtils.cpp
@@ -6,16 +6,17 @@
 
 #include <stdarg.h>
 
 #include "BindingUtils.h"
 
 #include "WrapperFactory.h"
 #include "xpcprivate.h"
 #include "XPCQuickStubs.h"
+#include "nsIXPConnect.h"
 
 namespace mozilla {
 namespace dom {
 
 JSErrorFormatString ErrorFormatString[] = {
 #define MSG_DEF(_name, _argc, _str) \
   { _str, _argc, JSEXN_TYPEERR },
 #include "mozilla/dom/Errors.msg"
@@ -624,16 +625,29 @@ HasPropertyOnPrototype(JSContext* cx, JS
   }
   MOZ_ASSERT(js::IsProxy(proxy) && js::GetProxyHandler(proxy) == handler);
 
   bool found;
   // We ignore an error from GetPropertyOnPrototype.
   return !GetPropertyOnPrototype(cx, proxy, id, &found, NULL) || found;
 }
 
+bool
+WrapCallbackInterface(JSContext *cx, JSObject *scope, nsISupports* callback,
+                      JS::Value* vp)
+{
+  nsCOMPtr<nsIXPConnectWrappedJS> wrappedJS = do_QueryInterface(callback);
+  MOZ_ASSERT(wrappedJS, "How can we not have an XPCWrappedJS here?");
+  JSObject* obj;
+  DebugOnly<nsresult> rv = wrappedJS->GetJSObject(&obj);
+  MOZ_ASSERT(NS_SUCCEEDED(rv) && obj, "What are we wrapping?");
+  *vp = JS::ObjectValue(*obj);
+  return JS_WrapValue(cx, vp);
+}
+
 JSObject*
 GetXrayExpandoChain(JSObject* obj)
 {
   MOZ_ASSERT(IsDOMObject(obj));
   JS::Value v = IsDOMProxy(obj) ? js::GetProxyExtra(obj, JSPROXYSLOT_XRAY_EXPANDO)
                                 : js::GetReservedSlot(obj, DOM_XRAY_EXPANDO_SLOT);
   return v.isUndefined() ? nullptr : &v.toObject();
 }
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -720,16 +720,20 @@ WrapObject(JSContext* cx, JSObject* scop
 template<>
 inline bool
 WrapObject<JSObject>(JSContext* cx, JSObject* scope, JSObject* p, JS::Value* vp)
 {
   vp->setObjectOrNull(p);
   return true;
 }
 
+bool
+WrapCallbackInterface(JSContext *cx, JSObject *scope, nsISupports* callback,
+                      JS::Value* vp);
+
 template<typename T>
 static inline JSObject*
 WrapNativeParent(JSContext* cx, JSObject* scope, const T& p)
 {
   if (!GetParentPointer(p))
     return scope;
 
   nsWrapperCache* cache = GetWrapperCache(p);
--- a/dom/bindings/Bindings.conf
+++ b/dom/bindings/Bindings.conf
@@ -21,17 +21,18 @@
 #   * headerFile - The file in which the nativeType is declared (defaults
 #                  to an educated guess).
 #   * castable - Indicates whether the value in the wrapper can be cast to
 #                nativeType, or whether it needs to be QI-ed (defaults to True
 #                for everything but callback interfaces and external interfaces,
 #                for which it defaults to false and is not allowed to be set
 #                at all).
 #   * concrete - Indicates whether there exist objects with this interface as
-#                their primary interface (defaults to True).
+#                their primary interface.  Always False for callback interfaces.
+#                defaults to True otherwise.
 #   * prefable - Indicates whether this bindings should be disabled if the
 #                global pref for Web IDL bindings is set to false.  This is a
 #                risk mitigation strategy and it will cause all of the Web IDL
 #                bindings marked as prefable to fall back to the xpconnect
 #                bindings in case something goes wrong.  This defaults to False.
 #                Setting this on objects which only have Web IDL bindings does
 #                not make any sense.
 #                Cannot be set on external interfaces.
@@ -47,18 +48,20 @@
 #   * binaryNames - Dict for mapping method and attribute names to different
 #                   names when calling the native methods (defaults to an empty
 #                   dict). The keys are the property names as they appear in the
 #                   .webidl file and the values are the names as they should be
 #                   in the WebIDL.
 #   * wrapperCache: True if this object is a wrapper cache.  Objects that are
 #                   not can only be returned from a limited set of methods,
 #                   cannot be prefable, and must ensure that they disallow
-#                   XPConnect wrapping.  Always true for worker descriptors.
-#                   Defaults to true.
+#                   XPConnect wrapping.  Always false for callback interfaces.
+#                   Always true for worker descriptors for non-callback
+#                   interfaces.  Defaults to true for non-worker non-callback
+#                   descriptors.
 #   * nativeOwnership: Describes how the native object is held. 4 possible
 #                      types: worker object ("worker"), non-refcounted object
 #                      ("owned"), refcounted non-nsISupports object
 #                      ("refcounted") or nsISupports ("nsisupports").
 #                      Non-refcounted objects need to inherit from
 #                      mozilla::dom::NonRefcountedDOMObject and preferably use
 #                      MOZ_COUNT_CTOR/MOZ_COUNT_DTOR in their
 #                      constructor/destructor so they participate in leak
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -2766,47 +2766,55 @@ if (%s.IsNull()) {
 
         # Now do non-nullable sequences.  We use setting the element
         # in the array as our succcess code because when we succeed in
         # wrapping that's what we should do.
         innerTemplate = wrapForType(
             type.inner, descriptorProvider,
             {
                 'result' :  "%s[i]" % result,
-                'successCode': ("if (!JS_DefineElement(cx, returnArray, i, tmp,\n"
-                                "                      NULL, NULL, JSPROP_ENUMERATE)) {\n"
-                                "  return false;\n"
-                                "}"),
+                'successCode': "break;",
                 'jsvalRef': "tmp",
                 'jsvalPtr': "&tmp",
                 'isCreator': isCreator
                 }
             )
-        innerTemplate = CGIndenter(CGGeneric(innerTemplate)).define()
+        innerTemplate = CGIndenter(CGGeneric(innerTemplate), 4).define()
         return (("""
 uint32_t length = %s.Length();
 JSObject *returnArray = JS_NewArrayObject(cx, length, NULL);
 if (!returnArray) {
   return false;
 }
 jsval tmp;
 for (uint32_t i = 0; i < length; ++i) {
+  // Control block to let us common up the JS_DefineElement calls when there
+  // are different ways to succeed at wrapping the object.
+  do {
 %s
+  } while (0);
+  if (!JS_DefineElement(cx, returnArray, i, tmp,
+                        nullptr, nullptr, JSPROP_ENUMERATE)) {
+    return false;
+  }
 }\n""" % (result, innerTemplate)) + setValue("JS::ObjectValue(*returnArray)"), False)
 
     if type.isGeckoInterface():
         descriptor = descriptorProvider.getDescriptor(type.unroll().inner.identifier.name)
         if type.nullable():
             wrappingCode = ("if (!%s) {\n" % (result) +
                             CGIndenter(CGGeneric(setValue("JSVAL_NULL"))).define() + "\n" +
                             "}\n")
         else:
             wrappingCode = ""
-        if (not descriptor.interface.isExternal() and
-            not descriptor.interface.isCallback()):
+
+        if descriptor.interface.isCallback():
+            wrap = "WrapCallbackInterface(cx, obj, %s, ${jsvalPtr})" % result
+            failed = None
+        elif not descriptor.interface.isExternal():
             if descriptor.wrapperCache:
                 wrapMethod = "WrapNewBindingObject"
             else:
                 if not isCreator:
                     raise MethodNotCreatorError(descriptor.interface.identifier.name)
                 wrapMethod = "WrapNewBindingNonWrapperCachedObject"
             wrap = "%s(cx, ${obj}, %s, ${jsvalPtr})" % (wrapMethod, result)
             # We don't support prefable stuff in workers.
@@ -2819,24 +2827,25 @@ for (uint32_t i = 0; i < length; ++i) {
                           "return false;")
             else:
                 if descriptor.notflattened:
                     raise TypeError("%s is prefable but not flattened; "
                                     "fallback won't work correctly" %
                                     descriptor.interface.identifier.name)
                 # Try old-style wrapping for bindings which might be preffed off.
                 failed = wrapAndSetPtr("HandleNewBindingWrappingFailure(cx, ${obj}, %s, ${jsvalPtr})" % result)
-            wrappingCode += wrapAndSetPtr(wrap, failed)
         else:
             if descriptor.notflattened:
                 getIID = "&NS_GET_IID(%s), " % descriptor.nativeType
             else:
                 getIID = ""
             wrap = "WrapObject(cx, ${obj}, %s, %s${jsvalPtr})" % (result, getIID)
-            wrappingCode += wrapAndSetPtr(wrap)
+            failed = None
+
+        wrappingCode += wrapAndSetPtr(wrap, failed)
         return (wrappingCode, False)
 
     if type.isString():
         if type.nullable():
             return (wrapAndSetPtr("xpc::StringToJsval(cx, %s, ${jsvalPtr})" % result), False)
         else:
             return (wrapAndSetPtr("xpc::NonVoidStringToJsval(cx, %s, ${jsvalPtr})" % result), False)
 
--- a/dom/bindings/Configuration.py
+++ b/dom/bindings/Configuration.py
@@ -182,17 +182,19 @@ class Descriptor(DescriptorProvider):
 
         self.notflattened = desc.get('notflattened', False)
         self.register = desc.get('register', True)
 
         self.hasXPConnectImpls = desc.get('hasXPConnectImpls', False)
 
         # If we're concrete, we need to crawl our ancestor interfaces and mark
         # them as having a concrete descendant.
-        self.concrete = desc.get('concrete', not self.interface.isExternal())
+        self.concrete = (not self.interface.isExternal() and
+                         not self.interface.isCallback() and
+                         desc.get('concrete', True))
         if self.concrete:
             self.proxy = False
             operations = {
                 'IndexedGetter': None,
                 'IndexedSetter': None,
                 'IndexedCreator': None,
                 'IndexedDeleter': None,
                 'NamedGetter': None,
@@ -258,17 +260,18 @@ class Descriptor(DescriptorProvider):
         else:
             self.nativeOwnership = desc.get('nativeOwnership', 'nsisupports')
             if not self.nativeOwnership in ['owned', 'refcounted', 'nsisupports']:
                 raise TypeError("Descriptor for %s has unrecognized value (%s) "
                                 "for nativeOwnership" %
                                 (self.interface.identifier.name, self.nativeOwnership))
         self.customTrace = desc.get('customTrace', self.workers)
         self.customFinalize = desc.get('customFinalize', self.workers)
-        self.wrapperCache = self.workers or desc.get('wrapperCache', True)
+        self.wrapperCache = (not self.interface.isCallback() and
+                             (self.workers or desc.get('wrapperCache', True)))
 
         if not self.wrapperCache and self.prefable:
             raise TypeError("Descriptor for %s is prefable but not wrappercached" %
                             self.interface.identifier.name)
 
         def make_name(name):
             return name + "_workers" if self.workers else name
         self.name = make_name(interface.identifier.name)
--- a/dom/bindings/test/TestBindingHeader.h
+++ b/dom/bindings/test/TestBindingHeader.h
@@ -271,17 +271,19 @@ public:
   void ReceiveSequenceOfNullableInts(nsTArray< Nullable<int32_t> >&);
   void ReceiveNullableSequenceOfNullableInts(Nullable< nsTArray< Nullable<int32_t> > >&);
   void PassSequence(const Sequence<int32_t> &);
   void PassNullableSequence(const Nullable< Sequence<int32_t> >&);
   void PassSequenceOfNullableInts(const Sequence<Nullable<int32_t> >&);
   void PassOptionalSequenceOfNullableInts(const Optional<Sequence<Nullable<int32_t> > > &);
   void PassOptionalNullableSequenceOfNullableInts(const Optional<Nullable<Sequence<Nullable<int32_t> > > > &);
   void ReceiveCastableObjectSequence(nsTArray< nsRefPtr<TestInterface> > &);
+  void ReceiveCallbackObjectSequence(nsTArray< nsRefPtr<TestCallbackInterface> > &);
   void ReceiveNullableCastableObjectSequence(nsTArray< nsRefPtr<TestInterface> > &);
+  void ReceiveNullableCallbackObjectSequence(nsTArray< nsRefPtr<TestCallbackInterface> > &);
   void ReceiveCastableObjectNullableSequence(Nullable< nsTArray< nsRefPtr<TestInterface> > >&);
   void ReceiveNullableCastableObjectNullableSequence(Nullable< nsTArray< nsRefPtr<TestInterface> > >&);
   void ReceiveWeakCastableObjectSequence(nsTArray<TestInterface*> &);
   void ReceiveWeakNullableCastableObjectSequence(nsTArray<TestInterface*> &);
   void ReceiveWeakCastableObjectNullableSequence(Nullable< nsTArray<TestInterface*> >&);
   void ReceiveWeakNullableCastableObjectNullableSequence(Nullable< nsTArray<TestInterface*> >&);
   void PassCastableObjectSequence(const Sequence< OwningNonNull<TestInterface> >&);
   void PassNullableCastableObjectSequence(const Sequence< nsRefPtr<TestInterface> > &);
--- a/dom/bindings/test/TestCodeGen.webidl
+++ b/dom/bindings/test/TestCodeGen.webidl
@@ -193,17 +193,19 @@ interface TestInterface {
   sequence<long?> receiveSequenceOfNullableInts();
   sequence<long?>? receiveNullableSequenceOfNullableInts();
   void passSequence(sequence<long> arg);
   void passNullableSequence(sequence<long>? arg);
   void passSequenceOfNullableInts(sequence<long?> arg);
   void passOptionalSequenceOfNullableInts(optional sequence<long?> arg);
   void passOptionalNullableSequenceOfNullableInts(optional sequence<long?>? arg);
   sequence<TestInterface> receiveCastableObjectSequence();
+  sequence<TestCallbackInterface> receiveCallbackObjectSequence();
   sequence<TestInterface?> receiveNullableCastableObjectSequence();
+  sequence<TestCallbackInterface?> receiveNullableCallbackObjectSequence();
   sequence<TestInterface>? receiveCastableObjectNullableSequence();
   sequence<TestInterface?>? receiveNullableCastableObjectNullableSequence();
   sequence<TestInterface> receiveWeakCastableObjectSequence();
   sequence<TestInterface?> receiveWeakNullableCastableObjectSequence();
   sequence<TestInterface>? receiveWeakCastableObjectNullableSequence();
   sequence<TestInterface?>? receiveWeakNullableCastableObjectNullableSequence();
   void passCastableObjectSequence(sequence<TestInterface> arg);
   void passNullableCastableObjectSequence(sequence<TestInterface?> arg);