Explicit shutdown of TimerManager to ensure this is done on the main thread (#54625)

Summary:
Pull Request resolved: #54625

# Changelog
[Internal] -

It makes sense to explicitly shut down TimerManager instance to make sure that this is done from the place and thread it's intended to - otherwise, since lifetime of TimerManager instance is managed by a shared pointer, this can lead to situations, whereas some other thread retains the shared pointer and calls the shut down at some later stage, which can lead to all kinds of unintended consequences.

Reviewed By: Nurtau

Differential Revision: D87631467

fbshipit-source-id: 206198ffd019b1d3edb4db557e67b22ca107757e

Author: rshest

packages/react-native/ReactCommon/react/runtime/PlatformTimerRegistry.h

@@ -25,6 +25,8 @@ class PlatformTimerRegistry {
   virtual void createRecurringTimer(uint32_t timerID, double delayMS) = 0;
 
   virtual ~PlatformTimerRegistry() noexcept = default;
+
+  virtual void quit() {}
 };
 
 using TimerManagerDelegate = PlatformTimerRegistry;

packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp

@@ -162,6 +162,11 @@ ReactInstance::ReactInstance(
         runtimeScheduler->scheduleWork(std::move(callback));
       });
 }
+ReactInstance::~ReactInstance() noexcept {
+  if (timerManager_ != nullptr) {
+    timerManager_->quit();
+  }
+}
 
 void ReactInstance::unregisterFromInspector() {
   if (inspectorTarget_ != nullptr) {
@@ -184,8 +189,8 @@ RuntimeExecutor ReactInstance::getUnbufferedRuntimeExecutor() noexcept {
 
 // This BufferedRuntimeExecutor ensures that the main JS bundle finished
 // execution before any JS queued into it from C++ are executed. Use
-// getUnbufferedRuntimeExecutor() instead if you do not need the main JS bundle
-// to have finished. e.g. setting global variables into JS runtime.
+// getUnbufferedRuntimeExecutor() instead if you do not need the main JS
+// bundle to have finished. e.g. setting global variables into JS runtime.
 RuntimeExecutor ReactInstance::getBufferedRuntimeExecutor() noexcept {
   return [weakBufferedRuntimeExecutor_ =
               std::weak_ptr<BufferedRuntimeExecutor>(bufferedRuntimeExecutor_)](
@@ -219,8 +224,8 @@ std::string simpleBasename(const std::string& path) {
  * Load the JS bundle and flush buffered JS calls, future JS calls won't be
  * buffered after calling this.
  * Note that this method is asynchronous. However, a completion callback
- * isn't needed because all calls into JS should be dispatched to the JSThread,
- * preferably via the runtimeExecutor_.
+ * isn't needed because all calls into JS should be dispatched to the
+ * JSThread, preferably via the runtimeExecutor_.
  */
 void ReactInstance::loadScript(
     std::unique_ptr<const JSBigString> script,

packages/react-native/ReactCommon/react/runtime/ReactInstance.h

@@ -31,6 +31,8 @@ class ReactInstance final : private jsinspector_modern::InstanceTargetDelegate {
       JsErrorHandler::OnJsError onJsError,
       jsinspector_modern::HostTarget *parentInspectorTarget = nullptr);
 
+  ~ReactInstance() noexcept;
+
   RuntimeExecutor getUnbufferedRuntimeExecutor() noexcept;
 
   RuntimeExecutor getBufferedRuntimeExecutor() noexcept;

packages/react-native/ReactCommon/react/runtime/TimerManager.cpp

@@ -56,6 +56,18 @@ TimerManager::TimerManager(
     std::unique_ptr<PlatformTimerRegistry> platformTimerRegistry) noexcept
     : platformTimerRegistry_(std::move(platformTimerRegistry)) {}
 
+TimerManager::~TimerManager() noexcept {
+  quit();
+}
+
+void TimerManager::quit() {
+  if (platformTimerRegistry_ == nullptr) {
+    return;
+  }
+  platformTimerRegistry_->quit();
+  platformTimerRegistry_ = nullptr;
+}
+
 void TimerManager::setRuntimeExecutor(
     RuntimeExecutor runtimeExecutor) noexcept {
   runtimeExecutor_ = std::move(runtimeExecutor);

packages/react-native/ReactCommon/react/runtime/TimerManager.h

@@ -46,13 +46,16 @@ struct TimerCallback {
 class TimerManager {
  public:
   explicit TimerManager(std::unique_ptr<PlatformTimerRegistry> platformTimerRegistry) noexcept;
+  ~TimerManager() noexcept;
 
   void setRuntimeExecutor(RuntimeExecutor runtimeExecutor) noexcept;
 
   void callTimer(TimerHandle handle);
 
   void attachGlobals(jsi::Runtime &runtime);
 
+  void quit();
+
  private:
   TimerHandle createTimer(
       jsi::Function &&callback,

packages/react-native/ReactCxxPlatform/react/runtime/platform/cxx/react/runtime/PlatformTimerRegistryImpl.cpp

@@ -12,10 +12,8 @@
 
 namespace facebook::react {
 
-PlatformTimerRegistryImpl::~PlatformTimerRegistryImpl() noexcept {
-  LOG(INFO)
-      << "PlatformTimerRegistryImpl::~PlatformTimerRegistryImpl() was called (address: "
-      << this << ").";
+void PlatformTimerRegistryImpl::quit() {
+  LOG(INFO) << "Shutting down PlatformTimerRegistryImpl...";
   taskDispatchThread_.quit();
   std::lock_guard<std::mutex> guard(timersMutex_);
   timers_.clear();

packages/react-native/ReactCxxPlatform/react/runtime/platform/cxx/react/runtime/PlatformTimerRegistryImpl.h

@@ -23,7 +23,7 @@ class PlatformTimerRegistryImpl : public PlatformTimerRegistry {
   PlatformTimerRegistryImpl &operator=(const PlatformTimerRegistryImpl &) = delete;
   PlatformTimerRegistryImpl(PlatformTimerRegistryImpl &&) noexcept = delete;
   PlatformTimerRegistryImpl &operator=(PlatformTimerRegistryImpl &&) noexcept = delete;
-  ~PlatformTimerRegistryImpl() noexcept override;
+  ~PlatformTimerRegistryImpl() noexcept = default;
 
   void createTimer(uint32_t timerId, double delayMs) override;
 
@@ -33,6 +33,8 @@ class PlatformTimerRegistryImpl : public PlatformTimerRegistry {
 
   void setTimerManager(std::weak_ptr<TimerManager> timerManager);
 
+  void quit() override;
+
  private:
   struct Timer {
     uint32_t timerId{0};