Use an atomic bool to track bridge destruction

Summary: We're seeing intermittent crashes in ~Bridge() where m_destroyed isn't set. This could be because the value of m_destroyed is cached for the destructing thread and doesn't see that the value got updated. Using an atomic boolean should fix this.

Reviewed By: mhorowitz

Differential Revision: D3126701

fb-gh-sync-id: 5887edef748cc05971765943de80187ab7fd8ede
fbshipit-source-id: 5887edef748cc05971765943de80187ab7fd8ede

ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java

@@ -406,6 +406,12 @@ public class CatalystInstanceImpl implements CatalystInstance {
     }
   }
 
+  @Override
+  protected void finalize() throws Throwable {
+    Assertions.assertCondition(mDestroyed, "Bridge was not destroyed before finalizer!");
+    super.finalize();
+  }
+
   private class NativeModulesReactCallback implements ReactCallback {
 
     @Override

ReactAndroid/src/main/jni/react/Bridge.cpp

@@ -19,7 +19,7 @@ Bridge::Bridge(
     std::unique_ptr<ExecutorTokenFactory> executorTokenFactory,
     std::unique_ptr<BridgeCallback> callback) :
   m_callback(std::move(callback)),
-  m_destroyed(std::make_shared<bool>(false)),
+  m_destroyed(std::make_shared<std::atomic_bool>(false)),
   m_executorTokenFactory(std::move(executorTokenFactory)) {
   std::unique_ptr<JSExecutor> mainExecutor = jsExecutorFactory->createJSExecutor(this);
   // cached to avoid locked map lookup in the common case
@@ -31,7 +31,7 @@ Bridge::Bridge(
 
 // This must be called on the same thread on which the constructor was called.
 Bridge::~Bridge() {
-  CHECK(*m_destroyed) << "Bridge::destroy() must be called before deallocating the Bridge!";
+  CHECK(m_destroyed->load(std::memory_order_acquire)) << "Bridge::destroy() must be called before deallocating the Bridge!";
 }
 
 void Bridge::loadApplicationScript(const std::string& script, const std::string& sourceURL) {
@@ -216,14 +216,14 @@ ExecutorToken Bridge::getTokenForExecutor(JSExecutor& executor) {
 }
 
 void Bridge::destroy() {
-  *m_destroyed = true;
+  m_destroyed->store(true, std::memory_order_release);
   m_mainExecutor = nullptr;
   std::unique_ptr<JSExecutor> mainExecutor = unregisterExecutor(*m_mainExecutorToken);
   mainExecutor->destroy();
 }
 
 void Bridge::runOnExecutorQueue(ExecutorToken executorToken, std::function<void(JSExecutor*)> task) {
-  if (*m_destroyed) {
+  if (m_destroyed->load(std::memory_order_acquire)) {
     return;
   }
 
@@ -233,9 +233,9 @@ void Bridge::runOnExecutorQueue(ExecutorToken executorToken, std::function<void(
     return;
   }
 
-  std::shared_ptr<bool> isDestroyed = m_destroyed;
+  std::shared_ptr<std::atomic_bool> isDestroyed = m_destroyed;
   executorMessageQueueThread->runOnQueue([this, isDestroyed, executorToken, task=std::move(task)] {
-    if (*isDestroyed) {
+    if (isDestroyed->load(std::memory_order_acquire)) {
       return;
     }
 

ReactAndroid/src/main/jni/react/Bridge.h

@@ -143,7 +143,7 @@ private:
   // This is used to avoid a race condition where a proxyCallback gets queued after ~Bridge(),
   // on the same thread. In that case, the callback will try to run the task on m_callback which
   // will have been destroyed within ~Bridge(), thus causing a SIGSEGV.
-  std::shared_ptr<bool> m_destroyed;
+  std::shared_ptr<std::atomic_bool> m_destroyed;
   JSExecutor* m_mainExecutor;
   std::unique_ptr<ExecutorToken> m_mainExecutorToken;
   std::unique_ptr<ExecutorTokenFactory> m_executorTokenFactory;