From f78bd0e72a3627be37b6b832fe6b1ab6dcbb45df Mon Sep 17 00:00:00 2001
From: Valentin Shergin <shergin@fb.com>
Date: Tue, 12 May 2020 11:25:24 -0700
Subject: [PATCH] Fabric: Fixed incorrect memory management in Scheduler

Summary:
The previous implementation was incorrect causing a memory leak. In the new approach, we use a shared pointer to optional to be able to construct that ahead of time and then fill with actual value.

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: sammy-SC

Differential Revision: D21516794

fbshipit-source-id: ddcbf8a1ad2b1f629ae4ff2842edeb7bb2a275a6
---
 ReactCommon/fabric/scheduler/Scheduler.cpp | 23 ++++++++++++----------
 ReactCommon/fabric/scheduler/Scheduler.h   | 11 ++++++++++-
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/ReactCommon/fabric/scheduler/Scheduler.cpp b/ReactCommon/fabric/scheduler/Scheduler.cpp
index 9a40225125..cc15c23efc 100644
--- a/ReactCommon/fabric/scheduler/Scheduler.cpp
+++ b/ReactCommon/fabric/scheduler/Scheduler.cpp
@@ -29,13 +29,13 @@ Scheduler::Scheduler(
       schedulerToolbox.contextContainer
           ->at<std::shared_ptr<const ReactNativeConfig>>("ReactNativeConfig");
 
+  // Creating a container for future `EventDispatcher` instance.
+  eventDispatcher_ =
+      std::make_shared<better::optional<EventDispatcher const>>();
+
   auto uiManager = std::make_shared<UIManager>();
   auto eventOwnerBox = std::make_shared<EventBeat::OwnerBox>();
-
-  // A dummy pointer to share a control block (and life-time) with
-  // an actual `owner` later.
-  auto owner = std::make_shared<bool const>(false);
-  eventOwnerBox->owner = owner;
+  eventOwnerBox->owner = eventDispatcher_;
 
   auto eventPipe = [uiManager](
                        jsi::Runtime &runtime,
@@ -52,21 +52,24 @@ Scheduler::Scheduler(
     uiManager->updateState(stateUpdate);
   };
 
-  auto eventDispatcher = std::make_unique<EventDispatcher const>(
+  // Creating an `EventDispatcher` instance inside the already allocated
+  // container (inside the optional).
+  eventDispatcher_->emplace(
       eventPipe,
       statePipe,
       schedulerToolbox.synchronousEventBeatFactory,
       schedulerToolbox.asynchronousEventBeatFactory,
       eventOwnerBox);
 
-  eventDispatcher_ =
-      std::shared_ptr<EventDispatcher const>(owner, eventDispatcher.release());
+  // Casting to `std::shared_ptr<EventDispatcher const>`.
+  auto eventDispatcher =
+      EventDispatcher::Shared{eventDispatcher_, &eventDispatcher_->value()};
 
   componentDescriptorRegistry_ = schedulerToolbox.componentRegistryFactory(
-      eventDispatcher_, schedulerToolbox.contextContainer);
+      eventDispatcher, schedulerToolbox.contextContainer);
 
   rootComponentDescriptor_ = std::make_unique<const RootComponentDescriptor>(
-      ComponentDescriptorParameters{eventDispatcher_, nullptr, nullptr});
+      ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
 
   uiManager->setDelegate(this);
   uiManager->setComponentDescriptorRegistry(componentDescriptorRegistry_);
diff --git a/ReactCommon/fabric/scheduler/Scheduler.h b/ReactCommon/fabric/scheduler/Scheduler.h
index 07799c62f1..c9f664f401 100644
--- a/ReactCommon/fabric/scheduler/Scheduler.h
+++ b/ReactCommon/fabric/scheduler/Scheduler.h
@@ -111,7 +111,16 @@ class Scheduler final : public UIManagerDelegate {
   RuntimeExecutor runtimeExecutor_;
   std::shared_ptr<UIManager> uiManager_;
   std::shared_ptr<const ReactNativeConfig> reactNativeConfig_;
-  EventDispatcher::Shared eventDispatcher_;
+
+  /*
+   * At some point, we have to have an owning shared pointer to something that
+   * will become an `EventDispatcher` a moment later. That's why we have it as a
+   * pointer to an optional: we construct the pointer first, share that with
+   * parts that need to have ownership (and only ownership) of that, and then
+   * fill the optional.
+   */
+  std::shared_ptr<better::optional<EventDispatcher const>> eventDispatcher_;
+
   bool enableNewStateReconciliation_{false};
 };