From b00f1b075fa84a29af18fb34e174b71998072122 Mon Sep 17 00:00:00 2001 From: Jim Chen Date: Thu, 16 Mar 2017 23:30:54 -0400 Subject: [PATCH] Bug 1344892 - Let native calls dispatch to XPCOM event queue; r=snorp Bug 1344892 - 1. Add option to dispatch to priority queue; r=snorp For the regular "gecko" option, change to dispatching to the XPCOM event queue, and add a new "gecko_priority" option that dispatches calls to the widget event queue. GeckoThread.waitOnGecko is changed to wait on both the widget queue and the XPCOM queue. nsAppShell::SyncRunEvent is changed to avoid a possible deadlock condition involving locking sAppShellLock twice. Bug 1344892 - 2. Update dispatchTo = "gecko" options; r=snorp Update some existing dispatchTo = "gecko" options to "gecko_priority", which typically involve UI events or JNI management calls like disposeNative. As a rule, disposeNative is dispatched to the queue with the least priority among the queues that other native members of the same class dispatch to (i.e. "gecko_priority" if all other native members dispatch to "gecko_priority", or "gecko" if any native members dispatch to "gecko"). Bug 1344892 - 3. Update auto-generated bindings; r=me --- .../annotationProcessors/AnnotationInfo.java | 1 + .../java/org/mozilla/gecko/ZoomedView.java | 2 +- .../mozilla/gecko/AndroidGamepadManager.java | 6 +-- .../java/org/mozilla/gecko/GeckoAppShell.java | 6 +-- .../mozilla/gecko/annotation/WrapForJNI.java | 19 +++++++--- .../java/org/mozilla/gecko/gfx/LayerView.java | 2 +- .../gecko/gfx/NativePanZoomController.java | 2 +- widget/android/EventDispatcher.cpp | 2 +- widget/android/GeneratedJNINatives.h | 8 +--- widget/android/GeneratedJNIWrappers.cpp | 3 -- widget/android/GeneratedJNIWrappers.h | 31 +++------------ widget/android/fennec/FennecJNIWrappers.h | 2 +- widget/android/jni/Natives.h | 28 ++++++++++++-- widget/android/jni/Utils.cpp | 2 +- widget/android/jni/Utils.h | 11 ++++-- widget/android/nsAppShell.cpp | 38 ++++++++++--------- widget/android/nsWindow.cpp | 2 + 17 files changed, 87 insertions(+), 78 deletions(-) diff --git a/build/annotationProcessors/AnnotationInfo.java b/build/annotationProcessors/AnnotationInfo.java index a8dbc53ce943..0b4332058cc5 100644 --- a/build/annotationProcessors/AnnotationInfo.java +++ b/build/annotationProcessors/AnnotationInfo.java @@ -30,6 +30,7 @@ public class AnnotationInfo { public enum DispatchTarget { GECKO, + GECKO_PRIORITY, PROXY, CURRENT; diff --git a/mobile/android/base/java/org/mozilla/gecko/ZoomedView.java b/mobile/android/base/java/org/mozilla/gecko/ZoomedView.java index 21e874343c4c..f4a123428f39 100644 --- a/mobile/android/base/java/org/mozilla/gecko/ZoomedView.java +++ b/mobile/android/base/java/org/mozilla/gecko/ZoomedView.java @@ -780,7 +780,7 @@ public class ZoomedView extends FrameLayout implements LayerView.DynamicToolbarL return ((System.nanoTime() - lastStartTimeReRender) < MINIMUM_DELAY_BETWEEN_TWO_RENDER_CALLS_NS); } - @WrapForJNI(dispatchTo = "gecko") + @WrapForJNI(dispatchTo = "gecko_priority") private static native void requestZoomedViewData(ByteBuffer buffer, int tabId, int xPos, int yPos, int width, int height, float scale); diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/AndroidGamepadManager.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/AndroidGamepadManager.java index 5aa9d9ee8b8d..70157d86dcf0 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/AndroidGamepadManager.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/AndroidGamepadManager.java @@ -129,11 +129,11 @@ public class AndroidGamepadManager { } } - @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko") + @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko_priority") private static native void onGamepadChange(int id, boolean added); - @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko") + @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko_priority") private static native void onButtonChange(int id, int button, boolean pressed, float value); - @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko") + @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko_priority") private static native void onAxisChange(int id, boolean[] valid, float[] values); private static boolean sStarted; diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java index 217134d5a3e5..6f24273b6fea 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java @@ -250,11 +250,7 @@ public class GeckoAppShell return sLayerView; } - // Synchronously notify a Gecko observer; must be called from Gecko thread. - @WrapForJNI(calledFrom = "gecko") - public static native void syncNotifyObservers(String topic, String data); - - @WrapForJNI(stubName = "NotifyObservers", dispatchTo = "proxy") + @WrapForJNI(stubName = "NotifyObservers", dispatchTo = "gecko") private static native void nativeNotifyObservers(String topic, String data); @RobocopTarget diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/annotation/WrapForJNI.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/annotation/WrapForJNI.java index 358ed5d568a7..99afca0caf2b 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/annotation/WrapForJNI.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/annotation/WrapForJNI.java @@ -30,8 +30,11 @@ public @interface WrapForJNI { /** * Action to take if member access returns an exception. - * One of "abort", "ignore", or "nsresult". "nsresult" is not supported for native - * methods. + * - "abort" will cause a crash if there is a pending exception. + * - "ignore" will not handle any pending exceptions; it is then the caller's + * responsibility to handle exceptions. + * - "nsresult" will clear any pending exceptions and return an error code; not + * supported for native methods. */ String exceptionMode() default "abort"; @@ -43,9 +46,15 @@ public @interface WrapForJNI { /** * The thread that the method call will be dispatched to. - * One of "current", "gecko", or "proxy". Not supported for non-native methods, - * fields, and constructors. Only void-return methods are supported for anything other - * than current thread. + * - "current" indicates no dispatching; only supported value for fields, + * constructors, non-native methods, and non-void native methods. + * - "gecko" indicates dispatching to the Gecko XPCOM (nsThread) event queue. + * - "gecko_priority" indicates dispatching to the Gecko widget + * (nsAppShell) event queue; in most cases, events in the widget event + * queue (aka native event queue) are favored over events in the XPCOM + * event queue. + * - "proxy" indicates dispatching to a proxy function as a function object; see + * widget/jni/Natives.h. */ String dispatchTo() default "current"; } diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java index c13627f2dc0e..d6f3e66e9380 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java @@ -80,7 +80,7 @@ public class LayerView extends FrameLayout { /* package */ native void attachToJava(GeckoLayerClient layerClient, NativePanZoomController npzc); - @WrapForJNI(calledFrom = "any", dispatchTo = "gecko") + @WrapForJNI(calledFrom = "any", dispatchTo = "gecko_priority") /* package */ native void onSizeChanged(int windowWidth, int windowHeight, int screenWidth, int screenHeight); diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java index 6b643c85b421..ed5b841076d6 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java @@ -220,7 +220,7 @@ class NativePanZoomController extends JNIObject implements PanZoomController { mDestroyed = false; } - @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko") @Override // JNIObject + @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko_priority") @Override // JNIObject protected native void disposeNative(); @Override diff --git a/widget/android/EventDispatcher.cpp b/widget/android/EventDispatcher.cpp index 3ad487dfceaf..b458ce22a60c 100644 --- a/widget/android/EventDispatcher.cpp +++ b/widget/android/EventDispatcher.cpp @@ -650,7 +650,7 @@ public: // Invoke callbacks synchronously if we're already on Gecko thread. return aCall(); } - nsAppShell::PostEvent(Move(aCall)); + NS_DispatchToMainThread(NS_NewRunnableFunction(Move(aCall))); } static void Finalize(const CallbackDelegate::LocalRef& aInstance) diff --git a/widget/android/GeneratedJNINatives.h b/widget/android/GeneratedJNINatives.h index b385c67af367..a3571c4f432d 100644 --- a/widget/android/GeneratedJNINatives.h +++ b/widget/android/GeneratedJNINatives.h @@ -101,7 +101,7 @@ template class GeckoAppShell::Natives : public mozilla::jni::NativeImpl { public: - static const JNINativeMethod methods[8]; + static const JNINativeMethod methods[7]; }; template @@ -133,11 +133,7 @@ const JNINativeMethod GeckoAppShell::Natives::methods[] = { mozilla::jni::MakeNativeMethod( mozilla::jni::NativeStub - ::template Wrap<&Impl::ReportJavaCrash>), - - mozilla::jni::MakeNativeMethod( - mozilla::jni::NativeStub - ::template Wrap<&Impl::SyncNotifyObservers>) + ::template Wrap<&Impl::ReportJavaCrash>) }; template diff --git a/widget/android/GeneratedJNIWrappers.cpp b/widget/android/GeneratedJNIWrappers.cpp index 9f8bf2eee139..ef359be290ed 100644 --- a/widget/android/GeneratedJNIWrappers.cpp +++ b/widget/android/GeneratedJNIWrappers.cpp @@ -668,9 +668,6 @@ auto GeckoAppShell::StartGeckoServiceChildProcess(mozilla::jni::String::Param a0 return mozilla::jni::Method::Call(GeckoAppShell::Context(), nullptr, a0, a1, a2, a3); } -constexpr char GeckoAppShell::SyncNotifyObservers_t::name[]; -constexpr char GeckoAppShell::SyncNotifyObservers_t::signature[]; - constexpr char GeckoAppShell::UnlockProfile_t::name[]; constexpr char GeckoAppShell::UnlockProfile_t::signature[]; diff --git a/widget/android/GeneratedJNIWrappers.h b/widget/android/GeneratedJNIWrappers.h index b2264cc49ba5..4e42f2c58f5c 100644 --- a/widget/android/GeneratedJNIWrappers.h +++ b/widget/android/GeneratedJNIWrappers.h @@ -66,7 +66,7 @@ public: static const mozilla::jni::CallingThread callingThread = mozilla::jni::CallingThread::UI; static const mozilla::jni::DispatchTarget dispatchTarget = - mozilla::jni::DispatchTarget::GECKO; + mozilla::jni::DispatchTarget::GECKO_PRIORITY; }; struct OnButtonChange_t { @@ -87,7 +87,7 @@ public: static const mozilla::jni::CallingThread callingThread = mozilla::jni::CallingThread::UI; static const mozilla::jni::DispatchTarget dispatchTarget = - mozilla::jni::DispatchTarget::GECKO; + mozilla::jni::DispatchTarget::GECKO_PRIORITY; }; struct OnGamepadAdded_t { @@ -127,7 +127,7 @@ public: static const mozilla::jni::CallingThread callingThread = mozilla::jni::CallingThread::UI; static const mozilla::jni::DispatchTarget dispatchTarget = - mozilla::jni::DispatchTarget::GECKO; + mozilla::jni::DispatchTarget::GECKO_PRIORITY; }; struct Start_t { @@ -1500,7 +1500,7 @@ public: static const mozilla::jni::CallingThread callingThread = mozilla::jni::CallingThread::ANY; static const mozilla::jni::DispatchTarget dispatchTarget = - mozilla::jni::DispatchTarget::PROXY; + mozilla::jni::DispatchTarget::GECKO; }; struct NotifyAlertListener_t { @@ -1882,25 +1882,6 @@ public: static auto StartGeckoServiceChildProcess(mozilla::jni::String::Param, mozilla::jni::ObjectArray::Param, int32_t, int32_t) -> int32_t; - struct SyncNotifyObservers_t { - typedef GeckoAppShell Owner; - typedef void ReturnType; - typedef void SetterType; - typedef mozilla::jni::Args< - mozilla::jni::String::Param, - mozilla::jni::String::Param> Args; - static constexpr char name[] = "syncNotifyObservers"; - static constexpr char signature[] = - "(Ljava/lang/String;Ljava/lang/String;)V"; - static const bool isStatic = true; - static const mozilla::jni::ExceptionMode exceptionMode = - mozilla::jni::ExceptionMode::ABORT; - static const mozilla::jni::CallingThread callingThread = - mozilla::jni::CallingThread::GECKO; - static const mozilla::jni::DispatchTarget dispatchTarget = - mozilla::jni::DispatchTarget::CURRENT; - }; - struct UnlockProfile_t { typedef GeckoAppShell Owner; typedef bool ReturnType; @@ -3933,7 +3914,7 @@ public: static const mozilla::jni::CallingThread callingThread = mozilla::jni::CallingThread::ANY; static const mozilla::jni::DispatchTarget dispatchTarget = - mozilla::jni::DispatchTarget::GECKO; + mozilla::jni::DispatchTarget::GECKO_PRIORITY; }; struct Reattach_t { @@ -4074,7 +4055,7 @@ public: static const mozilla::jni::CallingThread callingThread = mozilla::jni::CallingThread::UI; static const mozilla::jni::DispatchTarget dispatchTarget = - mozilla::jni::DispatchTarget::GECKO; + mozilla::jni::DispatchTarget::GECKO_PRIORITY; }; struct HandleMotionEvent_t { diff --git a/widget/android/fennec/FennecJNIWrappers.h b/widget/android/fennec/FennecJNIWrappers.h index 9be91d6a7ba8..8317a2ff5ba5 100644 --- a/widget/android/fennec/FennecJNIWrappers.h +++ b/widget/android/fennec/FennecJNIWrappers.h @@ -595,7 +595,7 @@ public: static const mozilla::jni::CallingThread callingThread = mozilla::jni::CallingThread::ANY; static const mozilla::jni::DispatchTarget dispatchTarget = - mozilla::jni::DispatchTarget::GECKO; + mozilla::jni::DispatchTarget::GECKO_PRIORITY; }; static const mozilla::jni::CallingThread callingThread = diff --git a/widget/android/jni/Natives.h b/widget/android/jni/Natives.h index 662053d1552a..c818c739e196 100644 --- a/widget/android/jni/Natives.h +++ b/widget/android/jni/Natives.h @@ -3,6 +3,8 @@ #include +#include "nsThreadUtils.h" + #include "mozilla/IndexSequence.h" #include "mozilla/Move.h" #include "mozilla/RefPtr.h" @@ -501,6 +503,21 @@ struct Dispatcher HasThisArg, Args...>(Forward(args)...)); } + template + static typename EnableIf< + Traits::dispatchTarget == DispatchTarget::GECKO_PRIORITY, void>::Type + Run(ThisArg thisArg, ProxyArgs&&... args) + { + // For a static method, do not forward the "this arg" (i.e. the class + // local ref) if the implementation does not request it. This saves us + // a pair of calls to add/delete global ref. + DispatchToGeckoPriorityQueue(MakeUnique>(HasThisArg || !IsStatic ? thisArg : nullptr, + Forward(args)...)); + } + template static typename EnableIf< @@ -510,16 +527,19 @@ struct Dispatcher // For a static method, do not forward the "this arg" (i.e. the class // local ref) if the implementation does not request it. This saves us // a pair of calls to add/delete global ref. - DispatchToGeckoThread(MakeUnique>(HasThisArg || !IsStatic ? thisArg : nullptr, - Forward(args)...)); + Args...>(HasThisArg || !IsStatic ? thisArg : nullptr, + Forward(args)...))); } template static typename EnableIf< Traits::dispatchTarget == DispatchTarget::CURRENT, void>::Type - Run(ProxyArgs&&... args) {} + Run(ProxyArgs&&... args) + { + MOZ_CRASH("Unreachable code"); + } }; } // namespace detail diff --git a/widget/android/jni/Utils.cpp b/widget/android/jni/Utils.cpp index dbbd74dc302c..c065800ed6df 100644 --- a/widget/android/jni/Utils.cpp +++ b/widget/android/jni/Utils.cpp @@ -287,7 +287,7 @@ jclass GetClassRef(JNIEnv* aEnv, const char* aClassName) return nullptr; } -void DispatchToGeckoThread(UniquePtr&& aCall) +void DispatchToGeckoPriorityQueue(UniquePtr&& aCall) { class AbstractCallEvent : public nsAppShell::Event { diff --git a/widget/android/jni/Utils.h b/widget/android/jni/Utils.h index 38e0b6b0c889..0d5b07ebb8cd 100644 --- a/widget/android/jni/Utils.h +++ b/widget/android/jni/Utils.h @@ -50,9 +50,14 @@ enum class DispatchTarget // 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. + // Call is dispatched asynchronously on the Gecko thread to the XPCOM + // (nsThread) event queue. Method must return void. GECKO, + // Call is dispatched asynchronously on the Gecko thread to the widget + // (nsAppShell) event queue. In most cases, events in the widget event + // queue (aka native event queue) are favored over events in the XPCOM + // event queue. Method must return void. + GECKO_PRIORITY, }; @@ -133,7 +138,7 @@ struct AbstractCall virtual void operator()() = 0; }; -void DispatchToGeckoThread(UniquePtr&& aCall); +void DispatchToGeckoPriorityQueue(UniquePtr&& aCall); /** * Returns whether Gecko is running in a Fennec environment, as determined by diff --git a/widget/android/nsAppShell.cpp b/widget/android/nsAppShell.cpp index 75ef581160e5..4301a719eb1f 100644 --- a/widget/android/nsAppShell.cpp +++ b/widget/android/nsAppShell.cpp @@ -146,8 +146,22 @@ public: static void WaitOnGecko() { - struct NoOpEvent : nsAppShell::Event { - void Run() override {} + struct NoOpRunnable : Runnable + { + NS_IMETHOD Run() override { return NS_OK; } + }; + + struct NoOpEvent : nsAppShell::Event + { + void Run() override + { + // We cannot call NS_DispatchToMainThread from within + // WaitOnGecko itself because the thread that is calling + // WaitOnGecko may not be an nsThread, and may not be able to do + // a sync dispatch. + NS_DispatchToMainThread(do_AddRef(new NoOpRunnable()), + NS_DISPATCH_SYNC); + } }; nsAppShell::SyncRunEvent(NoOpEvent()); } @@ -254,20 +268,6 @@ public: MOZ_CRASH("Uncaught Java exception"); } - static void SyncNotifyObservers(jni::String::Param aTopic, - jni::String::Param aData) - { - MOZ_RELEASE_ASSERT(NS_IsMainThread()); - NotifyObservers(aTopic, aData); - } - - template - static void OnNativeCall(Functor&& aCall) - { - MOZ_ASSERT(aCall.IsTarget(&NotifyObservers)); - NS_DispatchToMainThread(NS_NewRunnableFunction(mozilla::Move(aCall))); - } - static void NotifyObservers(jni::String::Param aTopic, jni::String::Param aData) { @@ -429,8 +429,10 @@ nsAppShell::nsAppShell() nsAppShell::~nsAppShell() { { + // Release any thread waiting for a sync call to finish. MutexAutoLock lock(*sAppShellLock); sAppShell = nullptr; + mSyncRunFinished.NotifyAll(); } while (mEventQueue.Pop(/* mayWait */ false)) { @@ -720,13 +722,13 @@ nsAppShell::SyncRunEvent(Event&& event, bool finished = false; auto runAndNotify = [&event, &finished] { - mozilla::MutexAutoLock shellLock(*sAppShellLock); - nsAppShell* const appShell = sAppShell; + nsAppShell* const appShell = nsAppShell::Get(); if (MOZ_UNLIKELY(!appShell || appShell->mSyncRunQuit)) { return; } event.Run(); finished = true; + mozilla::MutexAutoLock shellLock(*sAppShellLock); appShell->mSyncRunFinished.NotifyAll(); }; diff --git a/widget/android/nsWindow.cpp b/widget/android/nsWindow.cpp index fc98cf8244c4..c2ec68bd568b 100644 --- a/widget/android/nsWindow.cpp +++ b/widget/android/nsWindow.cpp @@ -852,6 +852,8 @@ public: mozilla::Move(aCall)), &LayerViewEvent::MakeEvent); return; } + + MOZ_CRASH("Unexpected call"); } static LayerViewSupport*