Bug 1486596 - Hold a WeakRef to EventDispatcher from native code r=jchen
authorJames Willcox <snorp@snorp.net>
Sat, 01 Sep 2018 16:48:08 -0500
changeset 487742 7e5bfc676ff76850c341dc0e17787cc761c1b49d
parent 487741 0ac1fbefa4f4d91b3ce310319c2b231a058a6a72
child 487743 c2b5dfcbd692d0c713ace2076d00b8d973c9a400
push id246
push userfmarier@mozilla.com
push dateSat, 13 Oct 2018 00:15:40 +0000
reviewersjchen
bugs1486596
milestone64.0a1
Bug 1486596 - Hold a WeakRef to EventDispatcher from native code r=jchen
mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
widget/android/EventDispatcher.cpp
widget/android/EventDispatcher.h
widget/android/jni/Refs.h
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
@@ -71,35 +71,47 @@ public final class EventDispatcher exten
     public EventDispatcher(final NativeQueue queue) {
         mNativeQueue = queue;
     }
 
     private boolean isReadyForDispatchingToGecko() {
         return mNativeQueue.isReady();
     }
 
-    @WrapForJNI(dispatchTo = "gecko") @Override // JNIObject
+    @WrapForJNI @Override // JNIObject
     protected native void disposeNative();
 
     @WrapForJNI private static final int DETACHED = 0;
     @WrapForJNI private static final int ATTACHED = 1;
     @WrapForJNI private static final int REATTACHING = 2;
 
     @WrapForJNI(calledFrom = "gecko")
     private synchronized void setAttachedToGecko(final int state) {
         if (mAttachedToGecko && state == DETACHED) {
-            if (GeckoThread.isRunning()) {
-                disposeNative();
-            } else {
-                GeckoThread.queueNativeCall(this, "disposeNative");
-            }
+            dispose(false);
         }
         mAttachedToGecko = (state == ATTACHED);
     }
 
+    private void dispose(boolean force) {
+        final Handler geckoHandler = ThreadUtils.sGeckoHandler;
+        if (geckoHandler == null) {
+            return;
+        }
+
+        geckoHandler.post(new Runnable() {
+            @Override
+            public void run() {
+                if (force || !mAttachedToGecko) {
+                    disposeNative();
+                }
+            }
+        });
+    }
+
     private <T> void registerListener(final Class<?> listType,
                                       final Map<String, List<T>> listenersMap,
                                       final T listener,
                                       final String[] events) {
         try {
             synchronized (listenersMap) {
                 for (final String event : events) {
                     if (event == null) {
@@ -344,16 +356,21 @@ public final class EventDispatcher exten
                         listener.handleMessage(type, message, wrappedCallback);
                     }
                 });
             }
             return true;
         }
     }
 
+    @Override
+    protected void finalize() throws Throwable {
+        dispose(true);
+    }
+
     private static class NativeCallbackDelegate extends JNIObject implements EventCallback {
         @WrapForJNI(calledFrom = "gecko")
         private NativeCallbackDelegate() {
         }
 
         @Override // JNIObject
         protected void disposeNative() {
             // We dispose in finalize().
--- a/widget/android/EventDispatcher.cpp
+++ b/widget/android/EventDispatcher.cpp
@@ -610,16 +610,17 @@ class JavaCallbackDelegate final : publi
         AutoJSContext cx;
 
         jni::Object::LocalRef data(jni::GetGeckoThreadEnv());
         nsresult rv = BoxData(NS_LITERAL_STRING("callback"), cx, aData, data,
                               /* ObjectOnly */ false);
         NS_ENSURE_SUCCESS(rv, JS_IsExceptionPending(cx) ? NS_OK : rv);
 
         dom::AutoNoJSAPI nojsapi;
+
         (java::EventCallback(*mCallback).*aCall)(data);
         return NS_OK;
     }
 
 public:
     explicit JavaCallbackDelegate(java::EventCallback::Param aCallback)
         : mCallback(jni::GetGeckoThreadEnv(), aCallback)
     {}
@@ -829,27 +830,28 @@ EventDispatcher::Dispatch(JS::HandleValu
         if (!aCallback || !aFinalizer) {
             return DispatchOnGecko(list, event, aData, aCallback);
         }
         nsCOMPtr<nsIAndroidEventCallback> callback(
                 new FinalizingCallbackDelegate(aCallback, aFinalizer));
         return DispatchOnGecko(list, event, aData, callback);
     }
 
-    if (!mDispatcher) {
+    java::EventDispatcher::LocalRef dispatcher(mDispatcher);
+    if (!dispatcher) {
         return NS_OK;
     }
 
     jni::Object::LocalRef data(jni::GetGeckoThreadEnv());
     nsresult rv = BoxData(event, aCx, aData, data, /* ObjectOnly */ true);
     NS_ENSURE_SUCCESS(rv, JS_IsExceptionPending(aCx) ? NS_OK : rv);
 
     dom::AutoNoJSAPI nojsapi;
-    mDispatcher->DispatchToThreads(event, data,
-                                   WrapCallback(aCallback, aFinalizer));
+    dispatcher->DispatchToThreads(event, data,
+                                  WrapCallback(aCallback, aFinalizer));
     return NS_OK;
 }
 
 nsresult
 EventDispatcher::Dispatch(const char16_t* aEvent,
                           java::GeckoBundle::Param aData,
                           nsIAndroidEventCallback* aCallback)
 {
@@ -867,21 +869,22 @@ EventDispatcher::Dispatch(const char16_t
         }
         JS::RootedValue data(jsapi.cx());
         nsresult rv = UnboxData(/* Event */ nullptr, jsapi.cx(), aData, &data,
                                 /* BundleOnly */ true);
         NS_ENSURE_SUCCESS(rv, rv);
         return DispatchOnGecko(list, event, data, aCallback);
     }
 
-    if (!mDispatcher) {
+    java::EventDispatcher::LocalRef dispatcher(mDispatcher);
+    if (!dispatcher) {
         return NS_OK;
     }
 
-    mDispatcher->DispatchToThreads(event, aData, WrapCallback(aCallback));
+    dispatcher->DispatchToThreads(event, aData, WrapCallback(aCallback));
     return NS_OK;
 }
 
 nsresult
 EventDispatcher::IterateEvents(JSContext* aCx, JS::HandleValue aEvents,
                                IterateEventsCallback aCallback,
                                nsIAndroidEventListener* aListener)
 {
@@ -996,66 +999,53 @@ EventDispatcher::UnregisterListener(nsIA
 
 void
 EventDispatcher::Attach(java::EventDispatcher::Param aDispatcher,
                         nsPIDOMWindowOuter* aDOMWindow)
 {
     MOZ_ASSERT(NS_IsMainThread());
     MOZ_ASSERT(aDispatcher);
 
-    if (mDispatcher) {
-        if (mDispatcher == aDispatcher) {
+    java::EventDispatcher::LocalRef dispatcher(mDispatcher);
+
+    if (dispatcher) {
+        if (dispatcher == aDispatcher) {
             // Only need to update the window.
             mDOMWindow = aDOMWindow;
             return;
         }
-        mAttachCount--;
-        mDispatcher->SetAttachedToGecko(java::EventDispatcher::REATTACHING);
+        dispatcher->SetAttachedToGecko(java::EventDispatcher::REATTACHING);
     }
 
-    java::EventDispatcher::LocalRef dispatcher(aDispatcher);
-
+    dispatcher = java::EventDispatcher::LocalRef(aDispatcher);
     NativesBase::AttachNative(dispatcher, this);
     mDispatcher = dispatcher;
     mDOMWindow = aDOMWindow;
 
     dispatcher->SetAttachedToGecko(java::EventDispatcher::ATTACHED);
-    mAttachCount++;
 }
 
 void
 EventDispatcher::Detach()
 {
     MOZ_ASSERT(NS_IsMainThread());
     MOZ_ASSERT(mDispatcher);
 
-    // SetAttachedToGecko will call disposeNative for us. disposeNative will be
-    // called later on the Gecko thread to make sure all pending
-    // dispatchToGecko calls have completed.
-    mAttachCount--;
-    mDispatcher->SetAttachedToGecko(java::EventDispatcher::DETACHED);
+    java::EventDispatcher::GlobalRef dispatcher(mDispatcher);
+
+    // SetAttachedToGecko will call disposeNative for us later on the Gecko
+    // thread to make sure all pending dispatchToGecko calls have completed.
+    if (dispatcher) {
+        dispatcher->SetAttachedToGecko(java::EventDispatcher::DETACHED);
+    }
+
     mDispatcher = nullptr;
     mDOMWindow = nullptr;
 }
 
-void
-EventDispatcher::DisposeNative(const java::EventDispatcher::LocalRef& aInstance)
-{
-    JNIEnv* const env = jni::GetGeckoThreadEnv();
-    const auto natives = reinterpret_cast<RefPtr<EventDispatcher>*>(
-            jni::GetNativeHandle(env, aInstance.Get()));
-    MOZ_CATCH_JNI_EXCEPTION(env);
-
-    if (!(*natives)->mAttachCount) {
-        // Only actually dispose if we haven't attached again between calling
-        // Detach() and calling DisposeNative().
-        NativesBase::DisposeNative(aInstance);
-    }
-}
-
 bool
 EventDispatcher::HasGeckoListener(jni::String::Param aEvent)
 {
     // Can be called from any thread.
     MutexAutoLock lock(mLock);
     return !!mListenersMap.Get(aEvent->ToString());
 }
 
--- a/widget/android/EventDispatcher.h
+++ b/widget/android/EventDispatcher.h
@@ -49,22 +49,20 @@ public:
     void DispatchToGecko(jni::String::Param aEvent,
                          jni::Object::Param aData,
                          jni::Object::Param aCallback);
 
     static nsresult UnboxBundle(JSContext* aCx,
                                 jni::Object::Param aData,
                                 JS::MutableHandleValue aOut);
 
-    static void DisposeNative(const java::EventDispatcher::LocalRef& aInstance);
-
+    using NativesBase::DisposeNative;
 private:
-    java::EventDispatcher::GlobalRef mDispatcher;
+    java::EventDispatcher::WeakRef mDispatcher;
     nsCOMPtr<nsPIDOMWindowOuter> mDOMWindow;
-    int32_t mAttachCount{0};
 
     virtual ~EventDispatcher() {}
 
     struct ListenersList {
         nsCOMArray<nsIAndroidEventListener> listeners{/* count */ 1};
         // 0 if the list can be modified
         uint32_t lockCount{0};
         // true if this list has a listener that is being unregistered
--- a/widget/android/jni/Refs.h
+++ b/widget/android/jni/Refs.h
@@ -261,18 +261,18 @@ public:
     {
         return mEnv->IsInstanceOf(
                 Ref::mInstance, typename T::Context(mEnv, nullptr).ClassRef());
     }
 
     bool operator==(const Ref& other) const
     {
         // Treat two references of the same object as being the same.
-        return Ref::mInstance == other.mInstance || JNI_FALSE !=
-                mEnv->IsSameObject(Ref::mInstance, other.mInstance);
+        return Ref::mInstance == other.Get() || JNI_FALSE !=
+                mEnv->IsSameObject(Ref::mInstance, other.Get());
     }
 
     bool operator!=(const Ref& other) const
     {
         return !operator==(other);
     }
 
     bool operator==(decltype(nullptr)) const