move page registration logic in to jsinspector

Reviewed By: pakoito

Differential Revision: D6531199

fbshipit-source-id: ed1ae9e2f0c19e7656cd022e438693798320e55a

Author: bnham

React/Inspector/RCTInspector.mm

@@ -49,17 +49,9 @@ @interface RCTInspectorLocalConnection () {
 - (instancetype)initWithConnection:(std::unique_ptr<ILocalConnection>)connection;
 @end
 
-// Only safe to call with Custom JSC. Custom JSC check must occur earlier
-// in the stack
 static IInspector *getInstance()
 {
-  static dispatch_once_t onceToken;
-  static IInspector *s_inspector;
-  dispatch_once(&onceToken, ^{
-    s_inspector = customJSCWrapper()->JSInspectorGetInstance();
-  });
-
-  return s_inspector;
+  return &facebook::react::getInspectorInstance();
 }
 
 @implementation RCTInspector

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

@@ -61,12 +61,8 @@ void JLocalConnection::registerNatives() {
   });
 }
 
-static IInspector* getInspectorInstance() {
-  return JSC_JSInspectorGetInstance(true /*useCustomJSC*/);
-}
-
 jni::global_ref<JInspector::javaobject> JInspector::instance(jni::alias_ref<jclass>) {
-  static auto instance = jni::make_global(newObjectCxxArgs(getInspectorInstance()/*&Inspector::instance()*/));
+  static auto instance = jni::make_global(newObjectCxxArgs(&getInspectorInstance()));
   return instance;
 }
 

ReactCommon/cxxreact/JSCExecutor.cpp

@@ -215,11 +215,12 @@ namespace facebook {
         const std::string ownerId = m_jscConfig.getDefault("OwnerIdentity", "unknown").getString();
         const std::string appId = m_jscConfig.getDefault("AppIdentity", "unknown").getString();
         const std::string deviceId = m_jscConfig.getDefault("DeviceIdentity", "unknown").getString();
-        const std::function<bool()> checkIsInspectedRemote = [&](){
+        auto checkIsInspectedRemote = [ownerId, appId, deviceId]() {
           return isNetworkInspected(ownerId, appId, deviceId);
         };
-        IInspector* pInspector = JSC_JSInspectorGetInstance(true);
-        pInspector->registerGlobalContext(ownerId, checkIsInspectedRemote, m_context);
+
+        auto& globalInspector = facebook::react::getInspectorInstance();
+        JSC_JSGlobalContextEnableDebugger(m_context, globalInspector, ownerId.c_str(), checkIsInspectedRemote);
       }
 
       installNativeHook<&JSCExecutor::nativeFlushQueueImmediate>("nativeFlushQueueImmediate");
@@ -340,8 +341,8 @@ namespace facebook {
       m_nativeModules.reset();
 
       if (canUseInspector(context)) {
-        IInspector* pInspector = JSC_JSInspectorGetInstance(true);
-        pInspector->unregisterGlobalContext(context);
+        auto &globalInspector = facebook::react::getInspectorInstance();
+        JSC_JSGlobalContextDisableDebugger(context, globalInspector);
       }
 
       JSC_JSGlobalContextRelease(context);

ReactCommon/cxxreact/JSCExecutor.h

@@ -117,7 +117,7 @@ class RN_EXPORT JSCExecutor : public JSExecutor, public PrivateDataBase {
   folly::Optional<Object> m_callFunctionReturnResultAndFlushedQueueJS;
 
   void initOnJSVMThread() throw(JSException);
-  bool isNetworkInspected(const std::string &owner, const std::string &app, const std::string &device);
+  static bool isNetworkInspected(const std::string &owner, const std::string &app, const std::string &device);
   // This method is experimental, and may be modified or removed.
   Value callFunctionSyncWithValue(
     const std::string& module, const std::string& method, Value value);

ReactCommon/jschelpers/JSCWrapper.h

@@ -9,6 +9,7 @@
 
 #pragma once
 
+#include <functional>
 #include <string>
 #include <JavaScriptCore/JavaScript.h>
 
@@ -32,7 +33,14 @@ namespace react {
 }
 }
 
-JSC_IMPORT facebook::react::IInspector* JSInspectorGetInstance();
+JSC_IMPORT void JSGlobalContextEnableDebugger(
+    JSGlobalContextRef ctx,
+    facebook::react::IInspector &globalInspector,
+    const char *title,
+    const std::function<bool()> &checkIsInspectedRemote);
+JSC_IMPORT void JSGlobalContextDisableDebugger(
+    JSGlobalContextRef ctx,
+    facebook::react::IInspector &globalInspector);
 
 // This is used to substitute an alternate JSC implementation for
 // testing. These calls must all be ABI compatible with the standard JSC.
@@ -143,7 +151,8 @@ struct JSCWrapper {
   JSC_WRAPPER_METHOD(JSPokeSamplingProfiler);
   JSC_WRAPPER_METHOD(JSStartSamplingProfilingOnMainJSCThread);
 
-  JSC_WRAPPER_METHOD(JSInspectorGetInstance);
+  JSC_WRAPPER_METHOD(JSGlobalContextEnableDebugger);
+  JSC_WRAPPER_METHOD(JSGlobalContextDisableDebugger);
 
   JSC_WRAPPER_METHOD(configureJSCForIOS);
 

ReactCommon/jschelpers/JavaScriptCore.h

@@ -191,9 +191,13 @@ jsc_poison(JSObjectMakeArrayBufferWithBytesNoCopy JSObjectMakeTypedArray
 jsc_poison(JSSamplingProfilerEnabled JSPokeSamplingProfiler
            JSStartSamplingProfilingOnMainJSCThread)
 
-#define JSC_JSInspectorGetInstance(...) __jsc_bool_wrapper(JSInspectorGetInstance, __VA_ARGS__)
-// no need to poison JSInspectorGetInstance because it's not defined for System JSC / standard SDK header
-// jsc_poison(JSInspectorGetInstance)
+#define JSC_JSGlobalContextEnableDebugger(...) __jsc_wrapper(JSGlobalContextEnableDebugger, __VA_ARGS__)
+// no need to poison JSGlobalContextEnableDebugger because it's not defined for System JSC / standard SDK header
+// jsc_poison(JSGlobalContextEnableDebugger)
+
+#define JSC_JSGlobalContextDisableDebugger(...) __jsc_wrapper(JSGlobalContextDisableDebugger, __VA_ARGS__)
+// no need to poison JSGlobalContextDisableDebugger because it's not defined for System JSC / standard SDK header
+// jsc_poison(JSGlobalContextDisableDebugger)
 
 
 #define JSC_configureJSCForIOS(...) __jsc_bool_wrapper(configureJSCForIOS, __VA_ARGS__)

ReactCommon/jschelpers/systemJSCWrapper.cpp

@@ -29,7 +29,8 @@ UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSStringCreateWithUTF8CStringExpectAscii)
 UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSPokeSamplingProfiler)
 UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSStartSamplingProfilingOnMainJSCThread)
 
-UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSInspectorGetInstance)
+UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSGlobalContextEnableDebugger)
+UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSGlobalContextDisableDebugger)
 
 UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(configureJSCForIOS)
 
@@ -129,9 +130,12 @@ const JSCWrapper* systemJSCWrapper() {
         (decltype(&JSStartSamplingProfilingOnMainJSCThread))
         Unimplemented_JSStartSamplingProfilingOnMainJSCThread,
 
-      .JSInspectorGetInstance =
-        (decltype(&JSInspectorGetInstance))
-        Unimplemented_JSInspectorGetInstance,
+      .JSGlobalContextEnableDebugger =
+        (decltype(&JSGlobalContextEnableDebugger))
+        Unimplemented_JSGlobalContextEnableDebugger,
+      .JSGlobalContextDisableDebugger =
+        (decltype(&JSGlobalContextDisableDebugger))
+        Unimplemented_JSGlobalContextDisableDebugger,
 
       .configureJSCForIOS =
         (decltype(&configureJSCForIOS))Unimplemented_configureJSCForIOS,

ReactCommon/jsinspector/BUCK

@@ -20,6 +20,7 @@ rn_xplat_cxx_library(
         "-fexceptions",
         "-std=c++1y",
     ],
+    fbandroid_preferred_linkage = "shared",
     visibility = [
         "PUBLIC",
     ],

ReactCommon/jsinspector/InspectorInterfaces.cpp

@@ -9,6 +9,9 @@
 
 #include "InspectorInterfaces.h"
 
+#include <mutex>
+#include <unordered_map>
+
 namespace facebook {
 namespace react {
 
@@ -19,5 +22,80 @@ IDestructible::~IDestructible() { }
 ILocalConnection::~ILocalConnection() { }
 IRemoteConnection::~IRemoteConnection() { }
 
+namespace {
+
+class InspectorImpl : public IInspector {
+ public:
+  int addPage(const std::string& title, ConnectFunc connectFunc) override;
+  void removePage(int pageId) override;
+
+  std::vector<InspectorPage> getPages() const override;
+  std::unique_ptr<ILocalConnection> connect(
+      int pageId,
+      std::unique_ptr<IRemoteConnection> remote) override;
+
+ private:
+  mutable std::mutex mutex_;
+  int nextPageId_{1};
+  std::unordered_map<int, std::string> titles_;
+  std::unordered_map<int, ConnectFunc> connectFuncs_;
+};
+
+int InspectorImpl::addPage(const std::string& title, ConnectFunc connectFunc) {
+  std::lock_guard<std::mutex> lock(mutex_);
+
+  int pageId = nextPageId_++;
+  titles_[pageId] = title;
+  connectFuncs_[pageId] = std::move(connectFunc);
+
+  return pageId;
+}
+
+void InspectorImpl::removePage(int pageId) {
+  std::lock_guard<std::mutex> lock(mutex_);
+
+  titles_.erase(pageId);
+  connectFuncs_.erase(pageId);
+}
+
+std::vector<InspectorPage> InspectorImpl::getPages() const {
+  std::lock_guard<std::mutex> lock(mutex_);
+
+  std::vector<InspectorPage> inspectorPages;
+  for (auto& it : titles_) {
+    inspectorPages.push_back(InspectorPage{it.first, it.second});
+  }
+
+  return inspectorPages;
+}
+
+std::unique_ptr<ILocalConnection> InspectorImpl::connect(
+    int pageId,
+    std::unique_ptr<IRemoteConnection> remote) {
+  IInspector::ConnectFunc connectFunc;
+
+  {
+    std::lock_guard<std::mutex> lock(mutex_);
+
+    auto it = connectFuncs_.find(pageId);
+    if (it != connectFuncs_.end()) {
+      connectFunc = it->second;
+    }
+  }
+
+  return connectFunc ? connectFunc(std::move(remote)) : nullptr;
+}
+
+} // namespace
+
+IInspector& getInspectorInstance() {
+  static InspectorImpl instance;
+  return instance;
 }
+
+std::unique_ptr<IInspector> makeTestInspectorInstance() {
+  return std::make_unique<InspectorImpl>();
 }
+
+} // namespace react
+} // namespace facebook

ReactCommon/jsinspector/InspectorInterfaces.h

@@ -18,7 +18,7 @@ namespace facebook {
 namespace react {
 
 class IDestructible {
-public:
+ public:
   virtual ~IDestructible() = 0;
 };
 
@@ -27,29 +27,52 @@ struct InspectorPage {
   const std::string title;
 };
 
+/// IRemoteConnection allows the VM to send debugger messages to the client.
 class IRemoteConnection : public IDestructible {
-public:
+ public:
   virtual ~IRemoteConnection() = 0;
   virtual void onMessage(std::string message) = 0;
   virtual void onDisconnect() = 0;
 };
 
+/// ILocalConnection allows the client to send debugger messages to the VM.
 class ILocalConnection : public IDestructible {
-public:
+ public:
   virtual ~ILocalConnection() = 0;
   virtual void sendMessage(std::string message) = 0;
   virtual void disconnect() = 0;
 };
 
-// Note: not destructible!
+/// IInspector tracks debuggable JavaScript targets (pages).
 class IInspector {
-public:
-  virtual void registerGlobalContext(const std::string& title, const std::function<bool()> &checkIsInspectedRemote, void* ctx) = 0;
-  virtual void unregisterGlobalContext(void* ctx) = 0;
+ public:
+  using ConnectFunc = std::function<std::unique_ptr<ILocalConnection>(
+      std::unique_ptr<IRemoteConnection>)>;
 
+  /// addPage is called by the VM to add a page to the list of debuggable pages.
+  virtual int addPage(const std::string& title, ConnectFunc connectFunc) = 0;
+
+  /// removePage is called by the VM to remove a page from the list of
+  /// debuggable pages.
+  virtual void removePage(int pageId) = 0;
+
+  /// getPages is called by the client to list all debuggable pages.
   virtual std::vector<InspectorPage> getPages() const = 0;
-  virtual std::unique_ptr<ILocalConnection> connect(int pageId, std::unique_ptr<IRemoteConnection> remote) = 0;
+
+  /// connect is called by the client to initiate a debugging session on the
+  /// given page.
+  virtual std::unique_ptr<ILocalConnection> connect(
+      int pageId,
+      std::unique_ptr<IRemoteConnection> remote) = 0;
 };
 
-}
-}
+/// getInspectorInstance retrieves the singleton inspector that tracks all
+/// debuggable pages in this process.
+extern IInspector& getInspectorInstance();
+
+/// makeTestInspectorInstance creates an independent inspector instance that
+/// should only be used in tests.
+extern std::unique_ptr<IInspector> makeTestInspectorInstance();
+
+} // namespace react
+} // namespace facebook