Bug 1096328 - Remove nativeOwnership from Bindings.conf, make the WrapObject signature for non-refcounted objects the same as for refcounted objects. r=bz.
authorPeter Van der Beken <peterv@propagandism.org>
Sat, 01 Nov 2014 15:00:28 +0100
changeset 237982 8e4a528db0d9da3bf35f037c17e46c3709882dde
parent 237981 ecdac08e0897a1e1846203265affecfd36810c77
child 237983 f4f784f1d227d389a50e33fa3b1c66ce3173ed0b
push idunknown
push userunknown
push dateunknown
reviewersbz
bugs1096328
milestone38.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 1096328 - Remove nativeOwnership from Bindings.conf, make the WrapObject signature for non-refcounted objects the same as for refcounted objects. r=bz. * * * [mq]: owned_fold.patch
dom/bindings/BindingUtils.h
dom/bindings/Codegen.py
dom/canvas/TextMetrics.h
dom/encoding/TextDecoder.h
dom/encoding/TextEncoder.h
dom/xslt/xpath/XPathExpression.h
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -1059,21 +1059,19 @@ WrapNewBindingNonWrapperCachedOwnedObjec
     JS::Rooted<JSObject*> scope(cx, scopeArg);
     if (js::IsWrapper(scope)) {
       scope = js::CheckedUnwrap(scope, /* stopAtOuter = */ false);
       if (!scope)
         return false;
       ac.emplace(cx, scope);
     }
 
-    bool tookOwnership = false;
     MOZ_ASSERT(js::IsObjectInContextCompartment(scope, cx));
-    obj = value->WrapObject(cx, &tookOwnership);
-    MOZ_ASSERT_IF(obj, tookOwnership);
-    if (tookOwnership) {
+    obj = value->WrapObject(cx);
+    if (obj) {
       value.forget();
     }
   }
 
   if (!obj) {
     return false;
   }
 
@@ -2518,21 +2516,16 @@ UseDOMXray(JSObject* obj)
 inline bool
 HasConstructor(JSObject* obj)
 {
   return JS_IsNativeFunction(obj, Constructor) ||
          js::GetObjectClass(obj)->construct;
 }
  #endif
  
-inline void
-MustInheritFromNonRefcountedDOMObject(NonRefcountedDOMObject*)
-{
-}
-
 template<class T, bool hasCallback=NativeHasMember<T>::JSBindingFinalized>
 struct JSBindingFinalized
 {
   static void Finalized(T* self)
   {
   }
 };
 
@@ -2750,16 +2743,111 @@ ToSupportsIsCorrect(T* aObject)
 template<class T>
 bool
 ToSupportsIsOnPrimaryInheritanceChain(T* aObject, nsWrapperCache* aCache)
 {
   return CastingAssertions<T>::ToSupportsIsOnPrimaryInheritanceChain(aObject,
                                                                      aCache);
 }
 
+// The BindingJSObjectCreator class is supposed to be used by a caller that
+// wants to create and initialise a binding JSObject. After initialisation has
+// been successfully completed it should call ForgetObject().
+// The BindingJSObjectCreator object will root the JSObject until ForgetObject()
+// is called on it. If the native object for the binding is refcounted it will
+// also hold a strong reference to it, that reference is transferred to the
+// JSObject (which holds the native in a slot) when ForgetObject() is called. If
+// the BindingJSObjectCreator object is destroyed and ForgetObject() was never
+// called on it then the JSObject's slot holding the native will be set to
+// undefined, and for a refcounted native the strong reference will be released.
+template<class T>
+class MOZ_STACK_CLASS BindingJSObjectCreator
+{
+public:
+  explicit BindingJSObjectCreator(JSContext* aCx)
+    : mObject(aCx)
+  {
+  }
+
+  ~BindingJSObjectCreator()
+  {
+    if (mObject) {
+      js::SetReservedOrProxyPrivateSlot(mObject, DOM_OBJECT_SLOT,
+                                        JS::UndefinedValue());
+    }
+  }
+
+  JS::Rooted<JSObject*>&
+  CreateProxyObject(JSContext* aCx, const js::Class* aClass,
+                    const DOMProxyHandler* aHandler,
+                    JS::Handle<JSObject*> aProto,
+                    JS::Handle<JSObject*> aParent, T* aNative)
+  {
+    js::ProxyOptions options;
+    options.setClass(aClass);
+    JS::Rooted<JS::Value> proxyPrivateVal(aCx, JS::PrivateValue(aNative));
+    mObject = js::NewProxyObject(aCx, aHandler, proxyPrivateVal, aProto,
+                                 aParent, options);
+    if (mObject) {
+      mNative = aNative;
+    }
+    return mObject;
+  }
+
+  JS::Rooted<JSObject*>&
+  CreateObject(JSContext* aCx, const JSClass* aClass,
+               JS::Handle<JSObject*> aProto, JS::Handle<JSObject*> aParent,
+               T* aNative)
+  {
+    mObject = JS_NewObject(aCx, aClass, aProto, aParent);
+    if (mObject) {
+      js::SetReservedSlot(mObject, DOM_OBJECT_SLOT, JS::PrivateValue(aNative));
+      mNative = aNative;
+    }
+    return mObject;
+  }
+
+  JSObject*
+  ForgetObject()
+  {
+    void* dummy;
+    mNative.forget(&dummy);
+
+    JSObject* obj = mObject;
+    mObject = nullptr;
+    return obj;
+  }
+
+private:
+  struct OwnedNative
+  {
+    // Make sure the native objects inherit from NonRefcountedDOMObject so
+    // that we log their ctor and dtor.
+    static_assert(IsBaseOf<NonRefcountedDOMObject, T>::value,
+                  "Non-refcounted objects with DOM bindings should inherit "
+                  "from NonRefcountedDOMObject.");
+
+    OwnedNative&
+    operator=(T* aNative)
+    {
+      return *this;
+    }
+
+    // This signature sucks, but it's the only one that will make a nsRefPtr
+    // just forget about its pointer without warning.
+    void
+    forget(void**)
+    {
+    }
+  };
+
+  JS::Rooted<JSObject*> mObject;
+  typename Conditional<IsRefcounted<T>::value, nsRefPtr<T>, OwnedNative>::Type mNative;
+};
+
 template<class T,
          bool isISupports=IsBaseOf<nsISupports, T>::value>
 class DeferredFinalizer
 {
   typedef typename Conditional<IsRefcounted<T>::value,
                                nsRefPtr<T>, nsAutoPtr<T>>::Type SmartPtr;
   typedef nsTArray<SmartPtr> SmartPtrArray;
 
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -3052,60 +3052,47 @@ class CGConstructorEnabled(CGAbstractMet
         else:
           conditionsWrapper = CGGeneric("return true;\n")
 
         body.append(conditionsWrapper)
         return body.define()
 
 
 def CreateBindingJSObject(descriptor, properties):
+    objDecl = "BindingJSObjectCreator<%s> creator(aCx);\n" % descriptor.nativeType
+
     # We don't always need to root obj, but there are a variety
     # of cases where we do, so for simplicity, just always root it.
-    objDecl = "JS::Rooted<JSObject*> obj(aCx);\n"
     if descriptor.proxy:
         create = dedent(
             """
-            JS::Rooted<JS::Value> proxyPrivateVal(aCx, JS::PrivateValue(aObject));
-            js::ProxyOptions options;
-            options.setClass(&Class.mBase);
-            obj = NewProxyObject(aCx, DOMProxyHandler::getInstance(),
-                                 proxyPrivateVal, proto, global, options);
+            const JS::Rooted<JSObject*>& obj =
+              creator.CreateProxyObject(aCx, &Class.mBase, DOMProxyHandler::getInstance(),
+                                        proto, global, aObject);
             if (!obj) {
               return nullptr;
             }
 
             """)
         if descriptor.interface.getExtendedAttribute('OverrideBuiltins'):
             create += dedent("""
                 js::SetProxyExtra(obj, JSPROXYSLOT_EXPANDO,
                                   JS::PrivateValue(&aObject->mExpandoAndGeneration));
 
                 """)
     else:
         create = dedent(
             """
-            obj = JS_NewObject(aCx, Class.ToJSClass(), proto, global);
+            const JS::Rooted<JSObject*>& obj =
+              creator.CreateObject(aCx, Class.ToJSClass(), proto, global, aObject);
             if (!obj) {
               return nullptr;
             }
-
-            js::SetReservedSlot(obj, DOM_OBJECT_SLOT, PRIVATE_TO_JSVAL(aObject));
             """)
-    create = objDecl + create
-
-    if descriptor.nativeOwnership == 'refcounted':
-        create += "NS_ADDREF(aObject);\n"
-    else:
-        create += dedent("""
-            // Make sure the native objects inherit from NonRefcountedDOMObject so that we
-            // log their ctor and dtor.
-            MustInheritFromNonRefcountedDOMObject(aObject);
-            *aTookOwnership = true;
-            """)
-    return create
+    return objDecl + create
 
 
 def InitUnforgeablePropertiesOnObject(descriptor, obj, properties, failureReturnValue=""):
     """
     properties is a PropertyArrays instance
     """
     unforgeables = []
 
@@ -3262,17 +3249,17 @@ class CGWrapWithCacheMethod(CGAbstractMe
             }
 
             $*{createObject}
 
             $*{unforgeable}
 
             aCache->SetWrapper(obj);
             $*{slots}
-            return obj;
+            return creator.ForgetObject();
             """,
             assertion=AssertInheritanceChain(self.descriptor),
             createObject=CreateBindingJSObject(self.descriptor, self.properties),
             unforgeable=InitUnforgeableProperties(self.descriptor, self.properties),
             slots=InitMemberSlots(self.descriptor, True))
 
 
 class CGWrapMethod(CGAbstractMethod):
@@ -3295,18 +3282,16 @@ class CGWrapNonWrapperCacheMethod(CGAbst
 
     properties should be a PropertyArrays instance.
     """
     def __init__(self, descriptor, properties):
         # XXX can we wrap if we don't have an interface prototype object?
         assert descriptor.interface.hasInterfacePrototypeObject()
         args = [Argument('JSContext*', 'aCx'),
                 Argument(descriptor.nativeType + '*', 'aObject')]
-        if descriptor.nativeOwnership == 'owned':
-            args.append(Argument('bool*', 'aTookOwnership'))
         CGAbstractMethod.__init__(self, descriptor, 'Wrap', 'JSObject*', args)
         self.properties = properties
 
     def definition_body(self):
         return fill(
             """
             $*{assertions}
 
@@ -3316,17 +3301,17 @@ class CGWrapNonWrapperCacheMethod(CGAbst
               return nullptr;
             }
 
             $*{createObject}
 
             $*{unforgeable}
 
             $*{slots}
-            return obj;
+            return creator.ForgetObject();
             """,
             assertions=AssertInheritanceChain(self.descriptor),
             createObject=CreateBindingJSObject(self.descriptor, self.properties),
             unforgeable=InitUnforgeableProperties(self.descriptor, self.properties),
             slots=InitMemberSlots(self.descriptor, False))
 
 
 class CGWrapGlobalMethod(CGAbstractMethod):
--- a/dom/canvas/TextMetrics.h
+++ b/dom/canvas/TextMetrics.h
@@ -25,19 +25,19 @@ public:
     MOZ_COUNT_DTOR(TextMetrics);
   }
 
   float Width() const
   {
     return width;
   }
 
-  JSObject* WrapObject(JSContext* aCx, bool* aTookOwnership)
+  JSObject* WrapObject(JSContext* aCx)
   {
-    return TextMetricsBinding::Wrap(aCx, this, aTookOwnership);
+    return TextMetricsBinding::Wrap(aCx, this);
   }
 
 private:
   float width;
 };
 
 } // namespace dom
 } // namespace mozilla
--- a/dom/encoding/TextDecoder.h
+++ b/dom/encoding/TextDecoder.h
@@ -44,19 +44,19 @@ public:
     MOZ_COUNT_CTOR(TextDecoder);
   }
 
   ~TextDecoder()
   {
     MOZ_COUNT_DTOR(TextDecoder);
   }
 
-  JSObject* WrapObject(JSContext* aCx, bool* aTookOwnership)
+  JSObject* WrapObject(JSContext* aCx)
   {
-    return TextDecoderBinding::Wrap(aCx, this, aTookOwnership);
+    return TextDecoderBinding::Wrap(aCx, this);
   }
 
   /**
    * Validates provided label and throws an exception if invalid label.
    *
    * @param aLabel       The encoding label (case insensitive) provided.
    * @param aFatal       indicates whether to throw an 'EncodingError'
    *                     exception or not when decoding.
--- a/dom/encoding/TextEncoder.h
+++ b/dom/encoding/TextEncoder.h
@@ -37,19 +37,19 @@ public:
   TextEncoder()
   {
   }
 
   virtual
   ~TextEncoder()
   {}
 
-  JSObject* WrapObject(JSContext* aCx, bool* aTookOwnership)
+  JSObject* WrapObject(JSContext* aCx)
   {
-    return TextEncoderBinding::Wrap(aCx, this, aTookOwnership);
+    return TextEncoderBinding::Wrap(aCx, this);
   }
 
 protected:
 
   /**
    * Validates provided encoding and throws an exception if invalid encoding.
    * If no encoding is provided then mEncoding is default initialised to "utf-8".
    *
--- a/dom/xslt/xpath/XPathExpression.h
+++ b/dom/xslt/xpath/XPathExpression.h
@@ -29,19 +29,19 @@ class XPathResult;
  */
 class XPathExpression MOZ_FINAL : public NonRefcountedDOMObject
 {
 public:
     XPathExpression(nsAutoPtr<Expr>&& aExpression, txResultRecycler* aRecycler,
                     nsIDocument *aDocument);
     ~XPathExpression();
 
-    JSObject* WrapObject(JSContext* aCx, bool* aTookOwnership)
+    JSObject* WrapObject(JSContext* aCx)
     {
-        return XPathExpressionBinding::Wrap(aCx, this, aTookOwnership);
+        return XPathExpressionBinding::Wrap(aCx, this);
     }
 
     already_AddRefed<XPathResult>
         Evaluate(JSContext* aCx, nsINode& aContextNode, uint16_t aType,
                  JS::Handle<JSObject*> aInResult, ErrorResult& aRv)
     {
         return EvaluateWithContext(aCx, aContextNode, 1, 1, aType, aInResult,
                                    aRv);