From 58f86b2d91f5e0cde43166c3bb6b0d02bd5e918f Mon Sep 17 00:00:00 2001 From: Andy Street Date: Tue, 1 Mar 2016 04:25:24 -0800 Subject: [PATCH] Don't execute things that may throw in Bridge/JSCExecutor dtors Summary:In testing, I've found that there's no good way to return stack traces to the server for exceptions that happen in dtor's. If the dtor is not marked nothrow(false), the exceptions are uncatchable (and bubble up as a std::abort without exception info) and if annotated properly, the program instead crashes trying to resume the stack, again a std::abort without exception info. Instead, I created a separate destroy method that can be called (and protected via fbjni) to make the dtor's no longer execute code that may throw. Note that we don't really expect the code that was previously in ~JSCExecutor() to throw, but it was in production and we had absolutely no info to help debug it. Reviewed By: mhorowitz Differential Revision: D2989999 fb-gh-sync-id: 4cf9de5e0592fe6830a9903375363a78e1339a94 shipit-source-id: 4cf9de5e0592fe6830a9903375363a78e1339a94 --- .../react/bridge/CatalystInstanceImpl.java | 1 + .../facebook/react/bridge/ReactBridge.java | 1 + ReactAndroid/src/main/jni/react/Bridge.cpp | 9 ++++++-- ReactAndroid/src/main/jni/react/Bridge.h | 5 +++++ ReactAndroid/src/main/jni/react/Executor.h | 1 + .../src/main/jni/react/JSCExecutor.cpp | 21 ++++++++++--------- ReactAndroid/src/main/jni/react/JSCExecutor.h | 1 + ReactAndroid/src/main/jni/react/Platform.cpp | 4 ---- ReactAndroid/src/main/jni/react/Platform.h | 5 ----- .../src/main/jni/react/jni/OnLoad.cpp | 13 +++++++++--- 10 files changed, 37 insertions(+), 24 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java index 18291a66f52..40618ac95c1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -235,6 +235,7 @@ public class CatalystInstanceImpl implements CatalystInstance { new Runnable() { @Override public void run() { + mBridge.destroy(); mBridge.dispose(); bridgeDisposeFuture.set(null); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java index 89b6870bec7..1e9fe67482d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java @@ -87,6 +87,7 @@ public class ReactBridge extends Countable { public native void stopProfiler(String title, String filename); private native void handleMemoryPressureModerate(); private native void handleMemoryPressureCritical(); + public native void destroy(); /** * This method will return a long representing the underlying JSGlobalContextRef pointer or diff --git a/ReactAndroid/src/main/jni/react/Bridge.cpp b/ReactAndroid/src/main/jni/react/Bridge.cpp index 1c1a2bf34dd..ed758d1f007 100644 --- a/ReactAndroid/src/main/jni/react/Bridge.cpp +++ b/ReactAndroid/src/main/jni/react/Bridge.cpp @@ -22,8 +22,7 @@ Bridge::Bridge(JSExecutorFactory* jsExecutorFactory, Callback callback) : // This must be called on the same thread on which the constructor was called. Bridge::~Bridge() { - *m_destroyed = true; - m_mainExecutor.reset(); + CHECK(*m_destroyed) << "Bridge::destroy() must be called before deallocating the Bridge!"; } void Bridge::loadApplicationScript(const std::string& script, const std::string& sourceURL) { @@ -132,4 +131,10 @@ void Bridge::callNativeModules(const std::string& callJSON, bool isEndOfBatch) { m_callback(parseMethodCalls(callJSON), isEndOfBatch); } +void Bridge::destroy() { + *m_destroyed = true; + m_mainExecutor->destroy(); + m_mainExecutor.reset(); +} + } } diff --git a/ReactAndroid/src/main/jni/react/Bridge.h b/ReactAndroid/src/main/jni/react/Bridge.h index 3868d507ff7..16edc62ac10 100644 --- a/ReactAndroid/src/main/jni/react/Bridge.h +++ b/ReactAndroid/src/main/jni/react/Bridge.h @@ -74,6 +74,11 @@ public: * TODO: get rid of isEndOfBatch */ void callNativeModules(const std::string& callJSON, bool isEndOfBatch); + + /** + * Synchronously tears down the bridge and the main executor. + */ + void destroy(); private: Callback m_callback; // This is used to avoid a race condition where a proxyCallback gets queued after ~Bridge(), diff --git a/ReactAndroid/src/main/jni/react/Executor.h b/ReactAndroid/src/main/jni/react/Executor.h index 036b6414ea6..6f392a3cd8f 100644 --- a/ReactAndroid/src/main/jni/react/Executor.h +++ b/ReactAndroid/src/main/jni/react/Executor.h @@ -72,6 +72,7 @@ public: virtual void handleMemoryPressureCritical() { handleMemoryPressureModerate(); }; + virtual void destroy() {}; virtual ~JSExecutor() {}; }; diff --git a/ReactAndroid/src/main/jni/react/JSCExecutor.cpp b/ReactAndroid/src/main/jni/react/JSCExecutor.cpp index 56af90b9bfd..01ccc818892 100644 --- a/ReactAndroid/src/main/jni/react/JSCExecutor.cpp +++ b/ReactAndroid/src/main/jni/react/JSCExecutor.cpp @@ -126,17 +126,17 @@ JSCExecutor::JSCExecutor( } JSCExecutor::~JSCExecutor() { - try { - *m_isDestroyed = true; - if (m_messageQueueThread->isOnThread()) { + CHECK(*m_isDestroyed) << "JSCExecutor::destroy() must be called before its destructor!"; +} + +void JSCExecutor::destroy() { + *m_isDestroyed = true; + if (m_messageQueueThread->isOnThread()) { + terminateOnJSVMThread(); + } else { + m_messageQueueThread->runOnQueueSync([this] () { terminateOnJSVMThread(); - } else { - m_messageQueueThread->runOnQueueSync([this] () { - terminateOnJSVMThread(); - }); - } - } catch (...) { - Exceptions::handleUncaughtException(); + }); } } @@ -392,6 +392,7 @@ void JSCExecutor::receiveMessageFromOwner(const std::string& msgString) { void JSCExecutor::terminateOwnedWebWorker(int workerId) { auto worker = m_ownedWorkers.at(workerId).getExecutor(); std::shared_ptr workerMQT = worker->m_messageQueueThread; + worker->destroy(); m_ownedWorkers.erase(workerId); workerMQT->quitSynchronous(); } diff --git a/ReactAndroid/src/main/jni/react/JSCExecutor.h b/ReactAndroid/src/main/jni/react/JSCExecutor.h index 33622014458..a10907ccff0 100644 --- a/ReactAndroid/src/main/jni/react/JSCExecutor.h +++ b/ReactAndroid/src/main/jni/react/JSCExecutor.h @@ -75,6 +75,7 @@ public: virtual void stopProfiler(const std::string &titleString, const std::string &filename) override; virtual void handleMemoryPressureModerate() override; virtual void handleMemoryPressureCritical() override; + virtual void destroy() override; void installNativeHook(const char *name, JSObjectCallAsFunctionCallback callback); diff --git a/ReactAndroid/src/main/jni/react/Platform.cpp b/ReactAndroid/src/main/jni/react/Platform.cpp index 3e1c700b78b..ace85bd9242 100644 --- a/ReactAndroid/src/main/jni/react/Platform.cpp +++ b/ReactAndroid/src/main/jni/react/Platform.cpp @@ -26,8 +26,4 @@ namespace JSLogging { JSCNativeHook nativeHook = nullptr; }; -namespace Exceptions { -HandleUncaughtException handleUncaughtException; -}; - } } diff --git a/ReactAndroid/src/main/jni/react/Platform.h b/ReactAndroid/src/main/jni/react/Platform.h index 5ad664dbfe5..608fbc1a854 100644 --- a/ReactAndroid/src/main/jni/react/Platform.h +++ b/ReactAndroid/src/main/jni/react/Platform.h @@ -46,9 +46,4 @@ namespace JSLogging { extern JSCNativeHook nativeHook; }; -namespace Exceptions { - using HandleUncaughtException = std::function; - extern HandleUncaughtException handleUncaughtException; -}; - } } diff --git a/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp b/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp index fad41e7cc59..d684b5823c0 100644 --- a/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp +++ b/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp @@ -651,6 +651,15 @@ static void create(JNIEnv* env, jobject obj, jobject executor, jobject callback, setCountableForJava(env, obj, std::move(bridge)); } +static void destroy(JNIEnv* env, jobject jbridge) { + auto bridge = extractRefPtr(env, jbridge); + try { + bridge->destroy(); + } catch (...) { + translatePendingCppExceptionToJavaException(); + } +} + static void loadApplicationScript( const RefPtr& bridge, const std::string& script, @@ -865,9 +874,6 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) { return std::unique_ptr( JMessageQueueThread::currentMessageQueueThread().release()); }; - Exceptions::handleUncaughtException = [] () { - translatePendingCppExceptionToJavaException(); - }; PerfLogging::installNativeHooks = addNativePerfLoggingHooks; JSLogging::nativeHook = nativeLoggingHook; @@ -950,6 +956,7 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) { registerNatives("com/facebook/react/bridge/ReactBridge", { makeNativeMethod("initialize", "(Lcom/facebook/react/bridge/JavaScriptExecutor;Lcom/facebook/react/bridge/ReactCallback;Lcom/facebook/react/bridge/queue/MessageQueueThread;)V", bridge::create), + makeNativeMethod("destroy", bridge::destroy), makeNativeMethod( "loadScriptFromAssets", "(Landroid/content/res/AssetManager;Ljava/lang/String;)V", bridge::loadScriptFromAssets),