Bug 1187552 - Support direct ownership of C++ objects by Java objects; r=snorp

Add a direct ownership model where the Java object owns the
corresponding C++ object directly, in addition to the WeakPtr model
where the Java object owns a WeakPtr to the C++ object. The WeakPtr
model is chosen when the implementing C++ class inherits from
SupportsWeakPtr. Otherwise, the direct ownership model is chosen. Under
the direct ownership model, a UniquePtr object must be used to attach
the containing C++ object to a Java object, to ensure ownership is
passed on to the Java object.

widget/android/jni/Natives.h

@@ -3,6 +3,9 @@
 
 #include <jni.h>
 
+#include "mozilla/Move.h"
+#include "mozilla/TypeTraits.h"
+#include "mozilla/UniquePtr.h"
 #include "mozilla/WeakPtr.h"
 #include "mozilla/jni/Accessors.h"
 #include "mozilla/jni/Refs.h"
@@ -12,53 +15,162 @@
 namespace mozilla {
 namespace jni {
 
-// Get the native pointer stored in a Java instance.
+/**
+ * C++ classes implementing instance (non-static) native methods can choose
+ * from one of two ownership models, when associating a C++ object with a Java
+ * instance.
+ *
+ * * If the C++ class inherits from mozilla::SupportsWeakPtr, weak pointers
+ *   will be used. The Java instance will store and own the pointer to a
+ *   WeakPtr object. The C++ class itself is otherwise not owned or directly
+ *   referenced. To attach a Java instance to a C++ instance, pass in a pointer
+ *   to the C++ class (i.e. MyClass*).
+ *
+ *   class MyClass : public SupportsWeakPtr<MyClass>
+ *                 , public MyJavaClass::Natives<MyClass>
+ *   {
+ *       // ...
+ *
+ *   public:
+ *       MOZ_DECLARE_WEAKREFERENCE_TYPENAME(MyClass)
+ *       using MyJavaClass::Natives<MyClass>::Dispose;
+ *
+ *       void AttachTo(const MyJavaClass::LocalRef& instance)
+ *       {
+ *           MyJavaClass::Natives<MyClass>::AttachInstance(instance, this);
+ *
+ *           // "instance" does NOT own "this", so the C++ object
+ *           // lifetime is separate from the Java object lifetime.
+ *       }
+ *   };
+ *
+ * * If the C++ class doesn't inherit from mozilla::SupportsWeakPtr, the Java
+ *   instance will store and own a pointer to the C++ object itself. This
+ *   pointer must not be stored or deleted elsewhere. To attach a Java instance
+ *   to a C++ instance, pass in a reference to a UniquePtr of the C++ class
+ *   (i.e. UniquePtr<MyClass>).
+ *
+ *   class MyClass : public MyJavaClass::Natives<MyClass>
+ *   {
+ *       // ...
+ *
+ *   public:
+ *       using MyJavaClass::Natives<MyClass>::Dispose;
+ *
+ *       static void AttachTo(const MyJavaClass::LocalRef& instance)
+ *       {
+ *           MyJavaClass::Natives<MyClass>::AttachInstance(
+ *                   instance, mozilla::MakeUnique<MyClass>());
+ *
+ *           // "instance" owns the newly created C++ object, so the C++
+ *           // object is destroyed as soon as instance.dispose() is called.
+ *       }
+ *   };
+ */
+
+namespace {
+
+uintptr_t CheckNativeHandle(JNIEnv* env, uintptr_t handle)
+{
+    if (!handle) {
+        if (!env->ExceptionCheck()) {
+            ThrowException(env, "java/lang/NullPointerException",
+                           "Null native pointer");
+        }
+        return 0;
+    }
+    return handle;
+}
+
+template<class Impl, bool UseWeakPtr = mozilla::IsBaseOf<
+                         SupportsWeakPtr<Impl>, Impl>::value /* = false */>
+struct NativePtr
+{
+    static Impl* Get(JNIEnv* env, jobject instance)
+    {
+        return reinterpret_cast<Impl*>(CheckNativeHandle(
+                env, GetNativeHandle(env, instance)));
+    }
+
+    template<class LocalRef>
+    static Impl* Get(const LocalRef& instance)
+    {
+        return Get(instance.Env(), instance.Get());
+    }
+
+    template<class LocalRef>
+    static void Set(const LocalRef& instance, UniquePtr<Impl>&& ptr)
+    {
+        Clear(instance);
+        SetNativeHandle(instance.Env(), instance.Get(),
+                          reinterpret_cast<uintptr_t>(ptr.release()));
+        HandleUncaughtException(instance.Env());
+    }
+
+    template<class LocalRef>
+    static void Clear(const LocalRef& instance)
+    {
+        UniquePtr<Impl> ptr(reinterpret_cast<Impl*>(
+                GetNativeHandle(instance.Env(), instance.Get())));
+        HandleUncaughtException(instance.Env());
+
+        if (ptr) {
+            SetNativeHandle(instance.Env(), instance.Get(), 0);
+            HandleUncaughtException(instance.Env());
+        }
+    }
+};
+
 template<class Impl>
-Impl* GetNativePtr(JNIEnv* env, jobject instance)
+struct NativePtr<Impl, /* UseWeakPtr = */ true>
 {
-    const auto ptr = reinterpret_cast<const WeakPtr<Impl>*>(
+    static Impl* Get(JNIEnv* env, jobject instance)
-            GetNativeHandle(env, instance));
+    {
-    if (!ptr) {
+        const auto ptr = reinterpret_cast<WeakPtr<Impl>*>(
-        return nullptr;
+                CheckNativeHandle(env, GetNativeHandle(env, instance)));
+        if (!ptr) {
+            return nullptr;
+        }
+
+        Impl* const impl = *ptr;
+        if (!impl) {
+            ThrowException(env, "java/lang/NullPointerException",
+                           "Native object already released");
+        }
+        return impl;
     }
 
-    Impl* const impl = *ptr;
+    template<class LocalRef>
-    if (!impl) {
+    static Impl* Get(const LocalRef& instance)
-        ThrowException(env, "java/lang/NullPointerException",
+    {
-                       "Native object already released");
+        return Get(instance.Env(), instance.Get());
     }
-    return impl;
-}
 
-template<class Impl, class LocalRef>
+    template<class LocalRef>
-Impl* GetNativePtr(const LocalRef& instance)
+    static void Set(const LocalRef& instance, SupportsWeakPtr<Impl>* ptr)
-{
+    {
-    return GetNativePtr<Impl>(instance.Env(), instance.Get());
+        Clear(instance);
-}
+        SetNativeHandle(instance.Env(), instance.Get(),
-
+                          reinterpret_cast<uintptr_t>(new WeakPtr<Impl>(ptr)));
-template<class Impl, class LocalRef>
+        HandleUncaughtException(instance.Env());
-void ClearNativePtr(const LocalRef& instance)
-{
-    JNIEnv* const env = instance.Env();
-    const auto ptr = reinterpret_cast<WeakPtr<Impl>*>(
-            GetNativeHandle(env, instance.Get()));
-    if (ptr) {
-        SetNativeHandle(env, instance.Get(), 0);
-        delete ptr;
-    } else {
-        // GetNativeHandle throws an exception when returning null.
-        MOZ_ASSERT(env->ExceptionCheck());
-        env->ExceptionClear();
     }
-}
 
-template<class Impl, class LocalRef>
+    template<class LocalRef>
-void SetNativePtr(const LocalRef& instance, Impl* ptr)
+    static void Clear(const LocalRef& instance)
-{
+    {
-    ClearNativePtr<Impl>(instance);
+        const auto ptr = reinterpret_cast<WeakPtr<Impl>*>(
-    SetNativeHandle(instance.Env(), instance.Get(),
+                GetNativeHandle(instance.Env(), instance.Get()));
-                      reinterpret_cast<uintptr_t>(new WeakPtr<Impl>(ptr)));
+        HandleUncaughtException(instance.Env());
-}
+
+        if (ptr) {
+            SetNativeHandle(instance.Env(), instance.Get(), 0);
+            HandleUncaughtException(instance.Env());
+            delete ptr;
+        }
+    }
+};
+
+} // namespace
 
 namespace detail {
 
@@ -88,7 +200,7 @@ public:
     static ReturnJNIType Wrap(JNIEnv* env, jobject instance,
                               typename TypeAdapter<Args>::JNIType... args)
     {
-        Impl* const impl = GetNativePtr<Impl>(env, instance);
+        Impl* const impl = NativePtr<Impl>::Get(env, instance);
         if (!impl) {
             return ReturnJNIType();
         }
@@ -101,7 +213,7 @@ public:
     static ReturnJNIType Wrap(JNIEnv* env, jobject instance,
                               typename TypeAdapter<Args>::JNIType... args)
     {
-        Impl* const impl = GetNativePtr<Impl>(env, instance);
+        Impl* const impl = NativePtr<Impl>::Get(env, instance);
         if (!impl) {
             return ReturnJNIType();
         }
@@ -125,7 +237,7 @@ public:
     static void Wrap(JNIEnv* env, jobject instance,
                      typename TypeAdapter<Args>::JNIType... args)
     {
-        Impl* const impl = GetNativePtr<Impl>(env, instance);
+        Impl* const impl = NativePtr<Impl>::Get(env, instance);
         if (!impl) {
             return;
         }
@@ -137,7 +249,7 @@ public:
     static void Wrap(JNIEnv* env, jobject instance,
                      typename TypeAdapter<Args>::JNIType... args)
     {
-        Impl* const impl = GetNativePtr<Impl>(env, instance);
+        Impl* const impl = NativePtr<Impl>::Get(env, instance);
         if (!impl) {
             return;
         }
@@ -245,8 +357,28 @@ public:
     }
 
 protected:
+
+    // Associate a C++ instance with a Java instance.
+    static void AttachNative(const typename Cls::LocalRef& instance,
+                             SupportsWeakPtr<Impl>* ptr)
+    {
+        static_assert(mozilla::IsBaseOf<SupportsWeakPtr<Impl>, Impl>::value,
+                      "Attach with UniquePtr&& when not using WeakPtr");
+        return NativePtr<Impl>::Set(instance, ptr);
+    }
+
+    static void AttachNative(const typename Cls::LocalRef& instance,
+                             UniquePtr<Impl>&& ptr)
+    {
+        static_assert(!mozilla::IsBaseOf<SupportsWeakPtr<Impl>, Impl>::value,
+                      "Attach with SupportsWeakPtr* when using WeakPtr");
+        return NativePtr<Impl>::Set(instance, mozilla::Move(ptr));
+    }
+
+    // Get the C++ instance associated with a Java instance.
+    // There is always a pending exception if the return value is nullptr.
     static Impl* GetNative(const typename Cls::LocalRef& instance) {
-        return GetNativePtr<Impl>(instance);
+        return NativePtr<Impl>::Get(instance);
     }
 
     NativeImpl() {
@@ -254,12 +386,8 @@ protected:
         Init();
     }
 
-    void AttachNative(const typename Cls::LocalRef& instance) {
-        SetNativePtr<>(instance, static_cast<Impl*>(this));
-    }
-
     void DisposeNative(const typename Cls::LocalRef& instance) {
-        ClearNativePtr<Impl>(instance);
+        NativePtr<Impl>::Clear(instance);
     }
 };
 

widget/android/jni/Utils.cpp

@@ -111,14 +111,8 @@ uintptr_t GetNativeHandle(JNIEnv* env, jobject instance)
         return 0;
     }
 
-    auto handle = static_cast<uintptr_t>(
+    return static_cast<uintptr_t>(
             env->GetLongField(instance, sJNIObjectHandleField));
-
-    if (!handle && !env->ExceptionCheck()) {
-        ThrowException(env, "java/lang/NullPointerException",
-                       "Null native pointer");
-    }
-    return handle;
 }
 
 void SetNativeHandle(JNIEnv* env, jobject instance, uintptr_t handle)