Clear event handler on deallocation

Summary:
It's just bad that we give a naked pointer of the connection manager to other instances. If the connection manager gets deallocated, the instances keeping a pointer to it are doomed to crash.

This change creates a wrapper on top of the pointer that can be freely shared. On deallocation, the shared wrapper gets invalidated.

Reviewed By: timur-valiev

Differential Revision: D30398466

fbshipit-source-id: 8f228e7fbaebc0ea28921409de071b58bbb69f1e

xplat/Flipper/FlipperConnectionManagerImpl.cpp

@@ -36,34 +36,52 @@ using namespace folly;
 namespace facebook {
 namespace flipper {
 
-class ConnectionEvents {
+class FlipperConnectionManagerWrapper {
  public:
-  ConnectionEvents(FlipperConnectionManagerImpl* impl) : impl_(impl) {}
+  FlipperConnectionManagerWrapper(FlipperConnectionManagerImpl* impl)
-  void operator()(const SocketEvent event) {
+      : impl_(impl) {}
-    switch (event) {
+  FlipperConnectionManagerImpl* get_impl() {
-      case SocketEvent::OPEN:
+    return impl_;
-        impl_->isOpen_ = true;
-        if (impl_->connectionIsTrusted_) {
-          impl_->callbacks_->onConnected();
-        }
-        break;
-      case SocketEvent::CLOSE:
-      case SocketEvent::ERROR:
-        if (!impl_->isOpen_)
-          return;
-        impl_->isOpen_ = false;
-        if (impl_->connectionIsTrusted_) {
-          impl_->connectionIsTrusted_ = false;
-          impl_->callbacks_->onDisconnected();
-        }
-        impl_->reconnect();
-        break;
-    }
   }
 
  private:
   FlipperConnectionManagerImpl* impl_;
 };
+class ConnectionEvents {
+ public:
+  ConnectionEvents(std::weak_ptr<FlipperConnectionManagerWrapper> impl)
+      : impl_(impl) {}
+  void operator()(const SocketEvent event) {
+    if (auto w = impl_.lock()) {
+      FlipperConnectionManagerImpl* impl = w->get_impl();
+      if (impl == nullptr) {
+        return;
+      }
+      switch (event) {
+        case SocketEvent::OPEN:
+          impl->isOpen_ = true;
+          if (impl->connectionIsTrusted_) {
+            impl->callbacks_->onConnected();
+          }
+          break;
+        case SocketEvent::CLOSE:
+        case SocketEvent::ERROR:
+          if (!impl->isOpen_)
+            return;
+          impl->isOpen_ = false;
+          if (impl->connectionIsTrusted_) {
+            impl->connectionIsTrusted_ = false;
+            impl->callbacks_->onDisconnected();
+          }
+          impl->reconnect();
+          break;
+      }
+    }
+  }
+
+ private:
+  std::weak_ptr<FlipperConnectionManagerWrapper> impl_;
+};
 
 FlipperConnectionManagerImpl::FlipperConnectionManagerImpl(
     FlipperInitConfig config,
@@ -75,7 +93,8 @@ FlipperConnectionManagerImpl::FlipperConnectionManagerImpl(
       securePort(config.securePort),
       flipperEventBase_(config.callbackWorker),
       connectionEventBase_(config.connectionWorker),
-      contextStore_(contextStore) {
+      contextStore_(contextStore),
+      implWrapper_(std::make_shared<FlipperConnectionManagerWrapper>(this)) {
   CHECK_THROW(config.callbackWorker, std::invalid_argument);
   CHECK_THROW(config.connectionWorker, std::invalid_argument);
 }
@@ -188,7 +207,7 @@ bool FlipperConnectionManagerImpl::connectAndExchangeCertificate() {
 
   auto newClient = std::make_unique<FlipperRSocket>(
       endpoint, std::move(payload), connectionEventBase_);
-  newClient->setEventHandler(ConnectionEvents(this));
+  newClient->setEventHandler(ConnectionEvents(implWrapper_));
 
   auto connectingInsecurely = flipperState_->start("Connect insecurely");
   connectionIsTrusted_ = false;
@@ -234,7 +253,7 @@ bool FlipperConnectionManagerImpl::connectSecurely() {
 
   auto newClient = std::make_unique<FlipperRSocket>(
       endpoint, std::move(payload), connectionEventBase_, contextStore_.get());
-  newClient->setEventHandler(ConnectionEvents(this));
+  newClient->setEventHandler(ConnectionEvents(implWrapper_));
 
   auto connectingSecurely = flipperState_->start("Connect securely");
   connectionIsTrusted_ = true;

xplat/Flipper/FlipperConnectionManagerImpl.h

@@ -21,7 +21,7 @@ namespace flipper {
 class ConnectionEvents;
 class ConnectionContextStore;
 class FlipperRSocketResponder;
-
+class FlipperConnectionManagerWrapper;
 class FlipperConnectionManagerImpl : public FlipperConnectionManager {
   friend ConnectionEvents;
 
@@ -70,6 +70,7 @@ class FlipperConnectionManagerImpl : public FlipperConnectionManager {
   bool connectionIsTrusted_;
   int failedConnectionAttempts_ = 0;
   std::shared_ptr<ConnectionContextStore> contextStore_;
+  std::shared_ptr<FlipperConnectionManagerWrapper> implWrapper_;
 
   void startSync();
   bool connectAndExchangeCertificate();

xplat/Flipper/FlipperRSocket.cpp

@@ -35,11 +35,11 @@ rsocket::Payload toRSocketPayload(folly::dynamic data);
 
 class RSocketEvents : public rsocket::RSocketConnectionEvents {
  private:
-  const SocketEventHandler& handler_;
+  const SocketEventHandler handler_;
 
  public:
-  RSocketEvents(const SocketEventHandler& eventHandler)
+  RSocketEvents(const SocketEventHandler eventHandler)
-      : handler_(eventHandler) {}
+      : handler_(std::move(eventHandler)) {}
 
   void onConnected() {
     handler_(SocketEvent::OPEN);