Bug 1292323 - Implement JNI thread checking and dispatching; r=snorp

Implement checking the calling thread of a JNI call based on the
calledFrom attribute set in WrapForJNI. Also implement automatic call
dispatching based on the dispatchTo attribute set in WrapForJNI. This
eliminates the use of UsesNativeCallProxy and UsesGeckoThreadProxy.

widget/android/jni/Accessors.h

@@ -39,7 +39,7 @@ class Accessor
     static void GetNsresult(JNIEnv* env, nsresult* rv)
     {
         if (env->ExceptionCheck()) {
-#ifdef DEBUG
+#ifdef MOZ_CHECK_JNI
             env->ExceptionDescribe();
 #endif
             env->ExceptionClear();
@@ -77,6 +77,10 @@ protected:
 
     static void BeginAccess(const Context& ctx)
     {
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
+        static_assert(Traits::dispatchTarget == DispatchTarget::CURRENT,
+                      "Dispatching not supported for method call");
+
         if (sID) {
             return;
         }
@@ -196,6 +200,10 @@ private:
 
     static void BeginAccess(const Context& ctx)
     {
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
+        static_assert(Traits::dispatchTarget == DispatchTarget::CURRENT,
+                      "Dispatching not supported for field access");
+
         if (sID) {
             return;
         }

widget/android/jni/Natives.h

@@ -178,16 +178,15 @@ struct NativePtr<Impl, /* UseWeakPtr = */ true>
 using namespace detail;
 
 /**
- * For C++ classes whose native methods all return void, they can choose to
+ * For JNI native methods that are dispatched to a proxy, i.e. using
- * have the native calls go through a proxy by inheriting from
+ * @WrapForJNI(dispatchTo = "proxy"), the implementing C++ class must provide a
- * mozilla::jni::UsesNativeCallProxy, and overriding the OnNativeCall member.
+ * OnNativeCall member. Subsequently, every native call is automatically
- * Subsequently, every native call is automatically wrapped in a functor
+ * wrapped in a functor object, and the object is passed to OnNativeCall. The
- * object, and the object is passed to OnNativeCall. The OnNativeCall
+ * OnNativeCall implementation can choose to invoke the call, save it, dispatch
- * implementation can choose to invoke the call, save it, dispatch it to a
+ * it to a different thread, etc. Each copy of functor may only be invoked
- * different thread, etc. Each copy of functor may only be invoked once.
+ * once.
  *
  * class MyClass : public MyJavaClass::Natives<MyClass>
- *               , public mozilla::jni::UsesNativeCallProxy
  * {
  *     // ...
  *
@@ -209,16 +208,6 @@ using namespace detail;
  * };
  */
 
-struct UsesNativeCallProxy
-{
-    template<class Functor>
-    static void OnNativeCall(Functor&& call)
-    {
-        // The default behavior is to invoke the call right away.
-        call();
-    }
-};
-
 namespace detail {
 
 template<class Traits, class Impl, class Args, bool IsStatic, bool IsVoid>
@@ -263,16 +252,12 @@ template<typename C> struct ProxyArg<const C&> : ProxyArg<C> {};
 template<> struct ProxyArg<StringParam> : ProxyArg<String::Ref> {};
 template<class C> struct ProxyArg<LocalRef<C>> : ProxyArg<typename C::Ref> {};
 
-// ProxyNativeCall implements the functor object that is passed to
+// ProxyNativeCall implements the functor object that is passed to OnNativeCall
-// UsesNativeCallProxy::OnNativeCall
 template<class Impl, class Owner, bool IsStatic,
          bool HasThisArg /* has instance/class local ref in the call */,
          typename... Args>
-class ProxyNativeCall
+class ProxyNativeCall : public AbstractCall
 {
-    template<class T, class I, class A, bool S, bool V>
-    friend class detail::NativeStubImpl;
-
     // "this arg" refers to the Class::LocalRef (for static methods) or
     // Owner::LocalRef (for instance methods) that we optionally (as indicated
     // by HasThisArg) pass into the destination C++ function.
@@ -298,15 +283,6 @@ class ProxyNativeCall
     // Saved arguments.
     mozilla::Tuple<typename ProxyArg<Args>::Type...> mArgs;
 
-    ProxyNativeCall(NativeCallType nativeCall,
-                    JNIEnv* env,
-                    ThisArgJNIType thisArg,
-                    typename ProxyArg<Args>::JNIType... args)
-        : mNativeCall(nativeCall)
-        , mThisArg(env, ThisArgClass::Ref::From(thisArg))
-        , mArgs(ProxyArg<Args>::From(env, args)...)
-    {}
-
     // We cannot use IsStatic and HasThisArg directly (without going through
     // extra hoops) because GCC complains about invalid overloads, so we use
     // another pair of template parameters, Static and ThisArg.
@@ -363,6 +339,15 @@ public:
 
     static const bool isStatic = IsStatic;
 
+    ProxyNativeCall(NativeCallType nativeCall,
+                    JNIEnv* env,
+                    ThisArgJNIType thisArg,
+                    typename ProxyArg<Args>::JNIType... args)
+        : mNativeCall(nativeCall)
+        , mThisArg(env, ThisArgClass::Ref::From(thisArg))
+        , mArgs(ProxyArg<Args>::From(env, args)...)
+    {}
+
     ProxyNativeCall(ProxyNativeCall&&) = default;
     ProxyNativeCall(const ProxyNativeCall&) = default;
 
@@ -381,7 +366,7 @@ public:
     void SetTarget(NativeCallType call) { mNativeCall = call; }
     template<typename T> void SetTarget(T&&) const { MOZ_CRASH(); }
 
-    void operator()()
+    void operator()() override
     {
         JNIEnv* const env = GetEnvForThread();
         typename ThisArgClass::LocalRef thisArg(env, mThisArg);
@@ -397,22 +382,28 @@ public:
     }
 };
 
-// We can only use Impl::OnNativeCall when Impl is derived from
+template<class Traits, class Impl, class O, bool S, bool V, typename... A>
-// UsesNativeCallProxy, otherwise it's a compile error. Therefore, the real
+typename mozilla::EnableIf<Traits::dispatchTarget == DispatchTarget::PROXY,
-// Dispatch function is conditional on UsesNativeCallProxy being a base class
+                           void>::Type
-// of Impl. Otherwise, the dummy Dispatch function below that is chosen during
+Dispatch(UniquePtr<ProxyNativeCall<Impl, O, S, V, A...>>&& call)
-// overload resolution. Because Dispatch is called with an rvalue, the &&
-// version is always chosen before the const& version, if possible.
-
-template<class Impl, class O, bool S, bool V, typename... A>
-typename mozilla::EnableIf<
-        mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value, void>::Type
-Dispatch(ProxyNativeCall<Impl, O, S, V, A...>&& call)
 {
-    Impl::OnNativeCall(mozilla::Move(call));
+    Impl::OnNativeCall(Move(*call));
 }
 
-template<typename T>
+template<class Traits, class Impl, class O, bool S, bool V, typename... A>
+typename mozilla::EnableIf<Traits::dispatchTarget == DispatchTarget::GECKO,
+                           void>::Type
+Dispatch(UniquePtr<ProxyNativeCall<Impl, O, S, V, A...>>&& call)
+{
+    DispatchToGeckoThread(Move(call));
+}
+
+// The dummy Dispatch function below is chosen during overload resolution if
+// none of the above conditional overloads apply. Because Dispatch is called
+// with an rvalue, the && version is always chosen before the const& version,
+// if possible.
+
+template<class Traits, typename T>
 void Dispatch(const T&) {}
 
 } // namespace detail
@@ -452,8 +443,9 @@ public:
     static MOZ_JNICALL ReturnJNIType Wrap(JNIEnv* env,
             jobject instance, typename TypeAdapter<Args>::JNIType... args)
     {
-        static_assert(!mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value,
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
-                      "Native call proxy only supports void return type");
+        static_assert(Traits::dispatchTarget == DispatchTarget::CURRENT,
+                      "Dispatched calls must have void return type");
 
         Impl* const impl = NativePtr<Impl>::Get(env, instance);
         if (!impl) {
@@ -468,8 +460,9 @@ public:
     static MOZ_JNICALL ReturnJNIType Wrap(JNIEnv* env,
             jobject instance, typename TypeAdapter<Args>::JNIType... args)
     {
-        static_assert(!mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value,
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
-                      "Native call proxy only supports void return type");
+        static_assert(Traits::dispatchTarget == DispatchTarget::CURRENT,
+                      "Dispatched calls must have void return type");
 
         Impl* const impl = NativePtr<Impl>::Get(env, instance);
         if (!impl) {
@@ -496,10 +489,12 @@ public:
     static MOZ_JNICALL void Wrap(JNIEnv* env,
             jobject instance, typename TypeAdapter<Args>::JNIType... args)
     {
-        if (mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value) {
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
-            Dispatch(ProxyNativeCall<
+
+        if (Traits::dispatchTarget != DispatchTarget::CURRENT) {
+            Dispatch<Traits>(MakeUnique<ProxyNativeCall<
                      Impl, Owner, /* IsStatic */ false, /* HasThisArg */ false,
-                     Args...>(Method, env, instance, args...));
+                     Args...>>(Method, env, instance, args...));
             return;
         }
         Impl* const impl = NativePtr<Impl>::Get(env, instance);
@@ -514,10 +509,12 @@ public:
     static MOZ_JNICALL void Wrap(JNIEnv* env,
             jobject instance, typename TypeAdapter<Args>::JNIType... args)
     {
-        if (mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value) {
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
-            Dispatch(ProxyNativeCall<
+
+        if (Traits::dispatchTarget != DispatchTarget::CURRENT) {
+            Dispatch<Traits>(MakeUnique<ProxyNativeCall<
                      Impl, Owner, /* IsStatic */ false, /* HasThisArg */ true,
-                     Args...>(Method, env, instance, args...));
+                     Args...>>(Method, env, instance, args...));
             return;
         }
         Impl* const impl = NativePtr<Impl>::Get(env, instance);
@@ -533,11 +530,14 @@ public:
     template<void (*DisposeNative) (const typename Owner::LocalRef&)>
     static MOZ_JNICALL void Wrap(JNIEnv* env, jobject instance)
     {
-        if (mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value) {
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
+
+        if (Traits::dispatchTarget != DispatchTarget::CURRENT) {
             auto cls = Class::LocalRef::Adopt(
                     env, env->GetObjectClass(instance));
-            Dispatch(ProxyNativeCall<Impl, Owner, /* IsStatic */ true,
+            Dispatch<Traits>(MakeUnique<ProxyNativeCall<
-                    /* HasThisArg */ false, const typename Owner::LocalRef&>(
+                    Impl, Owner, /* IsStatic */ true, /* HasThisArg */ false,
+                    const typename Owner::LocalRef&>>(
                     DisposeNative, env, cls.Get(), instance));
             return;
         }
@@ -561,8 +561,9 @@ public:
     static MOZ_JNICALL ReturnJNIType Wrap(JNIEnv* env,
             jclass, typename TypeAdapter<Args>::JNIType... args)
     {
-        static_assert(!mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value,
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
-                      "Native call proxy only supports void return type");
+        static_assert(Traits::dispatchTarget == DispatchTarget::CURRENT,
+                      "Dispatched calls must have void return type");
 
         return TypeAdapter<ReturnType>::FromNative(env,
                 (*Method)(TypeAdapter<Args>::ToNative(env, args)...));
@@ -573,8 +574,9 @@ public:
     static MOZ_JNICALL ReturnJNIType Wrap(JNIEnv* env,
             jclass cls, typename TypeAdapter<Args>::JNIType... args)
     {
-        static_assert(!mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value,
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
-                      "Native call proxy only supports void return type");
+        static_assert(Traits::dispatchTarget == DispatchTarget::CURRENT,
+                      "Dispatched calls must have void return type");
 
         auto clazz = Class::LocalRef::Adopt(env, cls);
         const auto res = TypeAdapter<ReturnType>::FromNative(env,
@@ -597,10 +599,12 @@ public:
     static MOZ_JNICALL void Wrap(JNIEnv* env,
             jclass cls, typename TypeAdapter<Args>::JNIType... args)
     {
-        if (mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value) {
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
-            Dispatch(ProxyNativeCall<
+
+        if (Traits::dispatchTarget != DispatchTarget::CURRENT) {
+            Dispatch<Traits>(MakeUnique<ProxyNativeCall<
                     Impl, Owner, /* IsStatic */ true, /* HasThisArg */ false,
-                    Args...>(Method, env, cls, args...));
+                    Args...>>(Method, env, cls, args...));
             return;
         }
         (*Method)(TypeAdapter<Args>::ToNative(env, args)...);
@@ -611,10 +615,12 @@ public:
     static MOZ_JNICALL void Wrap(JNIEnv* env,
             jclass cls, typename TypeAdapter<Args>::JNIType... args)
     {
-        if (mozilla::IsBaseOf<UsesNativeCallProxy, Impl>::value) {
+        MOZ_ASSERT_JNI_THREAD(Traits::callingThread);
-            Dispatch(ProxyNativeCall<
+
+        if (Traits::dispatchTarget != DispatchTarget::CURRENT) {
+            Dispatch<Traits>(MakeUnique<ProxyNativeCall<
                     Impl, Owner, /* IsStatic */ true, /* HasThisArg */ true,
-                    Args...>(Method, env, cls, args...));
+                    Args...>>(Method, env, cls, args...));
             return;
         }
         auto clazz = Class::LocalRef::Adopt(env, cls);

widget/android/jni/Refs.h

@@ -25,43 +25,6 @@ template<class Cls> class GlobalRef;
 template<class Cls> class DependentRef;
 
 
-// How exception during a JNI call should be treated.
-enum class ExceptionMode
-{
-    // Abort on unhandled excepion (default).
-    ABORT,
-    // Ignore the exception and return to caller.
-    IGNORE,
-    // Catch any exception and return a nsresult.
-    NSRESULT,
-};
-
-// Thread that a particular JNI call is allowed on.
-enum class CallingThread
-{
-    // Can be called from any thread (default).
-    ANY,
-    // Can be called from the Gecko thread.
-    GECKO,
-    // Can be called from the Java UI thread.
-    UI,
-};
-
-// If and where a JNI call will be dispatched.
-enum class DispatchTarget
-{
-    // Call happens synchronously on the calling thread (default).
-    CURRENT,
-    // Call happens synchronously on the calling thread, but the call is
-    // wrapped in a function object and is passed thru UsesNativeCallProxy.
-    // Method must return void.
-    PROXY,
-    // Call is dispatched asynchronously on the Gecko thread. Method must
-    // return void.
-    GECKO,
-};
-
-
 // Class to hold the native types of a method's arguments.
 // For example, if a method has signature (ILjava/lang/String;)V,
 // its arguments class would be jni::Args<int32_t, jni::String::Param>

widget/android/jni/Utils.cpp

@@ -7,6 +7,7 @@
 
 #include "AndroidBridge.h"
 #include "GeneratedJNIWrappers.h"
+#include "nsAppShell.h"
 
 #ifdef MOZ_CRASHREPORTER
 #include "nsExceptionHandler.h"
@@ -143,7 +144,7 @@ bool HandleUncaughtException(JNIEnv* aEnv)
         return false;
     }
 
-#ifdef DEBUG
+#ifdef MOZ_CHECK_JNI
     aEnv->ExceptionDescribe();
 #endif
 
@@ -210,5 +211,26 @@ jclass GetClassGlobalRef(JNIEnv* aEnv, const char* aClassName)
     return AndroidBridge::GetClassGlobalRef(aEnv, aClassName);
 }
 
+
+void DispatchToGeckoThread(UniquePtr<AbstractCall>&& aCall)
+{
+    class AbstractCallEvent : public nsAppShell::Event
+    {
+        UniquePtr<AbstractCall> mCall;
+
+    public:
+        AbstractCallEvent(UniquePtr<AbstractCall>&& aCall)
+            : mCall(Move(aCall))
+        {}
+
+        void Run() override
+        {
+            (*mCall)();
+        }
+    };
+
+    nsAppShell::PostEvent(MakeUnique<AbstractCallEvent>(Move(aCall)));
+}
+
 } // jni
 } // mozilla

widget/android/jni/Utils.h

@@ -3,14 +3,59 @@
 
 #include <jni.h>
 
+#include "mozilla/UniquePtr.h"
+
 #if defined(DEBUG) || !defined(RELEASE_BUILD)
+#define MOZ_CHECK_JNI
+#endif
+
+#ifdef MOZ_CHECK_JNI
+#include <pthread.h>
 #include "mozilla/Assertions.h"
+#include "APKOpen.h"
 #include "MainThreadUtils.h"
 #endif
 
 namespace mozilla {
 namespace jni {
 
+// How exception during a JNI call should be treated.
+enum class ExceptionMode
+{
+    // Abort on unhandled excepion (default).
+    ABORT,
+    // Ignore the exception and return to caller.
+    IGNORE,
+    // Catch any exception and return a nsresult.
+    NSRESULT,
+};
+
+// Thread that a particular JNI call is allowed on.
+enum class CallingThread
+{
+    // Can be called from any thread (default).
+    ANY,
+    // Can be called from the Gecko thread.
+    GECKO,
+    // Can be called from the Java UI thread.
+    UI,
+};
+
+// If and where a JNI call will be dispatched.
+enum class DispatchTarget
+{
+    // Call happens synchronously on the calling thread (default).
+    CURRENT,
+    // Call happens synchronously on the calling thread, but the call is
+    // wrapped in a function object and is passed thru UsesNativeCallProxy.
+    // Method must return void.
+    PROXY,
+    // Call is dispatched asynchronously on the Gecko thread. Method must
+    // return void.
+    GECKO,
+};
+
+
 extern JNIEnv* sGeckoThreadEnv;
 
 inline bool IsAvailable()
@@ -20,13 +65,9 @@ inline bool IsAvailable()
 
 inline JNIEnv* GetGeckoThreadEnv()
 {
-#if defined(DEBUG) || !defined(RELEASE_BUILD)
+#ifdef MOZ_CHECK_JNI
-    if (!NS_IsMainThread()) {
+    MOZ_RELEASE_ASSERT(NS_IsMainThread(), "Must be on Gecko thread");
-        MOZ_CRASH("Not on main thread");
+    MOZ_RELEASE_ASSERT(sGeckoThreadEnv, "Must have a JNIEnv");
-    }
-    if (!sGeckoThreadEnv) {
-        MOZ_CRASH("Don't have a JNIEnv");
-    }
 #endif
     return sGeckoThreadEnv;
 }
@@ -35,6 +76,21 @@ void SetGeckoThreadEnv(JNIEnv* aEnv);
 
 JNIEnv* GetEnvForThread();
 
+#ifdef MOZ_CHECK_JNI
+#define MOZ_ASSERT_JNI_THREAD(thread) \
+    do { \
+        if ((thread) == mozilla::jni::CallingThread::GECKO) { \
+            MOZ_RELEASE_ASSERT(::NS_IsMainThread()); \
+        } else if ((thread) == mozilla::jni::CallingThread::UI) { \
+            const bool isOnUiThread = ::pthread_equal(::pthread_self(), \
+                                                      ::getJavaUiThread()); \
+            MOZ_RELEASE_ASSERT(isOnUiThread); \
+        } \
+    } while (0)
+#else
+#define MOZ_ASSERT_JNI_THREAD(thread) do {} while (0)
+#endif
+
 bool ThrowException(JNIEnv *aEnv, const char *aClass,
                     const char *aMessage);
 
@@ -62,12 +118,22 @@ bool HandleUncaughtException(JNIEnv* aEnv);
         } \
     } while (0)
 
+
 uintptr_t GetNativeHandle(JNIEnv* env, jobject instance);
 
 void SetNativeHandle(JNIEnv* env, jobject instance, uintptr_t handle);
 
 jclass GetClassGlobalRef(JNIEnv* aEnv, const char* aClassName);
 
+
+struct AbstractCall
+{
+    virtual ~AbstractCall() {}
+    virtual void operator()() = 0;
+};
+
+void DispatchToGeckoThread(UniquePtr<AbstractCall>&& aCall);
+
 } // jni
 } // mozilla
 

widget/android/jni/moz.build

@@ -19,5 +19,6 @@ UNIFIED_SOURCES += [
 FINAL_LIBRARY = 'xul'
 
 LOCAL_INCLUDES += [
+    '/widget',
     '/widget/android',
 ]

widget/android/nsAppShell.h

@@ -224,18 +224,5 @@ protected:
     nsInterfaceHashtable<nsStringHashKey, nsIObserver> mObserversHash;
 };
 
-// Class that implement native JNI methods can inherit from
-// UsesGeckoThreadProxy to have the native call forwarded
-// automatically to the Gecko thread.
-class UsesGeckoThreadProxy : public mozilla::jni::UsesNativeCallProxy
-{
-public:
-    template<class Functor>
-    static void OnNativeCall(Functor&& call)
-    {
-        nsAppShell::PostEvent(mozilla::Move(call));
-    }
-};
-
 #endif // nsAppShell_h__