[skip ci] Remove mapbuffer from early js error handling (facebook#43951)

Summary: ## Context The **early js** error reporting pipeline catches javascript exceptions. After errors are caught, the pipeline parses the exception, and puts the data into into a map buffer. 🤨 ## Problems We don't need to use a mapbuffer here: The structure of this exception data is known and never changes. (A map buffer is a type-unsafe bag of key/value pairs). Instead, we could just use lower-level type-safe language primitives: regular C++ struct, and java class w/ fbjni. ## Changes Migrate the **early js** error handling infra to C++ structs/fbjni. ## Impact Now, there is no mapbuffer usage on iOS. We could re-introduce it when there is a need. Changelog: [Breaking][General] - Remove mapbuffer from early js error handling Reviewed By: fkgozali Differential Revision: D55265170
RSNara · Apr 7, 2024 · 390fad6 · 390fad6
1 parent 5927165
commit 390fad6
Show file tree

Hide file tree

Showing 10 changed files with 152 additions and 84 deletions.
diff --git a/...d/src/main/java/com/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler.kt b/...d/src/main/java/com/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler.kt
@@ -9,10 +9,42 @@ package com.facebook.react.interfaces.exceptionmanager
 
 import com.facebook.proguard.annotations.DoNotStripAny
 import com.facebook.react.common.annotations.UnstableReactNativeAPI
-import com.facebook.react.common.mapbuffer.ReadableMapBuffer
+import java.util.ArrayList
 
 @DoNotStripAny
 @UnstableReactNativeAPI
 public fun interface ReactJsExceptionHandler {
-  public fun reportJsException(errorMap: ReadableMapBuffer?)
+  @DoNotStripAny
+  public interface ParsedError {
+    @DoNotStripAny
+    public interface StackFrame {
+      public val fileName: String
+      public val methodName: String
+      public val lineNumber: Int
+      public val columnNumber: Int
+    }
+
+    public val frames: List<StackFrame>
+    public val message: String
+    public val exceptionId: Int
+    public val isFatal: Boolean
+  }
+
+  @DoNotStripAny
+  private data class ParsedStackFrameImpl(
+      override val fileName: String,
+      override val methodName: String,
+      override val lineNumber: Int,
+      override val columnNumber: Int,
+  ) : ParsedError.StackFrame
+
+  @DoNotStripAny
+  private data class ParsedErrorImpl(
+      override val frames: ArrayList<ParsedStackFrameImpl>,
+      override val message: String,
+      override val exceptionId: Int,
+      override val isFatal: Boolean,
+  ) : ParsedError
+
+  public fun reportJsException(errorMap: ParsedError)
 }
diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactExceptionManager.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactExceptionManager.cpp
@@ -9,17 +9,57 @@
 #include <fbjni/fbjni.h>
 #include <glog/logging.h>
 #include <jni.h>
-#include <iostream>
 
 namespace facebook::react {
 
+namespace {
+class ParsedError : public facebook::jni::JavaClass<ParsedError> {
+ public:
+  static auto constexpr kJavaDescriptor =
+      "Lcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedError;";
+};
+
+class ParsedStackFrameImpl
+    : public facebook::jni::JavaClass<ParsedStackFrameImpl> {
+ public:
+  static auto constexpr kJavaDescriptor =
+      "Lcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedStackFrameImpl;";
+
+  static facebook::jni::local_ref<ParsedStackFrameImpl> create(
+      const JsErrorHandler::ParsedError::StackFrame& frame) {
+    return newInstance(
+        frame.fileName, frame.methodName, frame.lineNumber, frame.columnNumber);
+  }
+};
+
+class ParsedErrorImpl
+    : public facebook::jni::JavaClass<ParsedErrorImpl, ParsedError> {
+ public:
+  static auto constexpr kJavaDescriptor =
+      "Lcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedErrorImpl;";
+
+  static facebook::jni::local_ref<ParsedErrorImpl> create(
+      const JsErrorHandler::ParsedError& error) {
+    auto stackFrames =
+        facebook::jni::JArrayList<ParsedStackFrameImpl>::create();
+    for (const auto& frame : error.frames) {
+      stackFrames->add(ParsedStackFrameImpl::create(frame));
+    }
+
+    return newInstance(
+        stackFrames, error.message, error.exceptionId, error.isFatal);
+  }
+};
+
+} // namespace
+
 void JReactExceptionManager::reportJsException(
-    const JReadableMapBuffer::javaobject errorMapBuffer) {
+    const JsErrorHandler::ParsedError& error) {
   static const auto method =
-      javaClassStatic()->getMethod<void(JReadableMapBuffer::javaobject)>(
+      javaClassStatic()->getMethod<void(jni::alias_ref<ParsedError>)>(
           "reportJsException");
   if (self() != nullptr) {
-    method(self(), errorMapBuffer);
+    method(self(), ParsedErrorImpl::create(error));
   }
 }
 

diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactExceptionManager.h b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactExceptionManager.h
@@ -9,7 +9,7 @@
 
 #include <fbjni/fbjni.h>
 #include <jni.h>
-#include <react/common/mapbuffer/JReadableMapBuffer.h>
+#include <jserrorhandler/JsErrorHandler.h>
 
 namespace facebook::react {
 
@@ -19,7 +19,7 @@ class JReactExceptionManager
   static auto constexpr kJavaDescriptor =
       "Lcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler;";
 
-  void reportJsException(const JReadableMapBuffer::javaobject errorMapBuffer);
+  void reportJsException(const JsErrorHandler::ParsedError& error);
 };
 
 } // namespace facebook::react
diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactInstance.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactInstance.cpp
@@ -21,7 +21,6 @@
 #include <react/common/mapbuffer/JReadableMapBuffer.h>
 #include <react/jni/JRuntimeExecutor.h>
 #include <react/jni/JSLogging.h>
-#include <react/renderer/mapbuffer/MapBuffer.h>
 #include <react/runtime/BridgelessJSCallInvoker.h>
 #include <react/runtime/BridgelessNativeMethodCallInvoker.h>
 #include "JavaTimerRegistry.h"
@@ -52,13 +51,14 @@ JReactInstance::JReactInstance(
   jsTimerExecutor->cthis()->setTimerManager(timerManager);
 
   jReactExceptionManager_ = jni::make_global(jReactExceptionManager);
-  auto jsErrorHandlingFunc = [this](MapBuffer errorMap) noexcept {
-    if (jReactExceptionManager_ != nullptr) {
-      auto jErrorMap =
-          JReadableMapBuffer::createWithContents(std::move(errorMap));
-      jReactExceptionManager_->reportJsException(jErrorMap.get());
-    }
-  };
+  auto jsErrorHandlingFunc =
+      [weakJReactExceptionManager = jni::make_weak(jReactExceptionManager)](
+          const JsErrorHandler::ParsedError& error) mutable noexcept {
+        if (auto jReactExceptionManager =
+                weakJReactExceptionManager.lockLocal()) {
+          jReactExceptionManager->reportJsException(error);
+        }
+      };
 
   jBindingsInstaller_ = jni::make_global(jBindingsInstaller);
 

diff --git a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp
@@ -6,17 +6,14 @@
  */
 
 #include "JsErrorHandler.h"
-#include <react/renderer/mapbuffer/MapBufferBuilder.h>
 #include <regex>
 #include <sstream>
 #include <string>
 #include <vector>
 
 namespace facebook::react {
 
-using facebook::react::JSErrorHandlerKey;
-
-static MapBuffer
+static JsErrorHandler::ParsedError
 parseErrorStack(const jsi::JSError& error, bool isFatal, bool isHermes) {
   /**
    * This parses the different stack traces and puts them into one format
@@ -43,63 +40,67 @@ parseErrorStack(const jsi::JSError& error, bool isFatal, bool isHermes) {
   std::string line;
   std::stringstream strStream(error.getStack());
 
-  auto errorObj = MapBufferBuilder();
-  std::vector<MapBuffer> frames;
+  std::vector<JsErrorHandler::ParsedError::StackFrame> frames;
 
   while (std::getline(strStream, line, '\n')) {
-    auto frame = MapBufferBuilder();
     auto searchResults = std::smatch{};
 
     if (isHermes) {
       if (std::regex_search(line, searchResults, REGEX_HERMES)) {
         std::string str2 = std::string(searchResults[2]);
         if (str2.compare("native")) {
-          frame.putString(kFrameFileName, std::string(searchResults[4]));
-          frame.putString(kFrameMethodName, std::string(searchResults[1]));
-          frame.putInt(kFrameLineNumber, std::stoi(searchResults[5]));
-          frame.putInt(kFrameColumnNumber, std::stoi(searchResults[6]));
-          frames.push_back(frame.build());
+          frames.push_back({
+              .fileName = std::string(searchResults[4]),
+              .methodName = std::string(searchResults[1]),
+              .lineNumber = std::stoi(searchResults[5]),
+              .columnNumber = std::stoi(searchResults[6]),
+          });
         }
       }
     } else {
       if (std::regex_search(line, searchResults, REGEX_GECKO)) {
-        frame.putString(kFrameFileName, std::string(searchResults[3]));
-        frame.putString(kFrameMethodName, std::string(searchResults[1]));
-        frame.putInt(kFrameLineNumber, std::stoi(searchResults[4]));
-        frame.putInt(kFrameColumnNumber, std::stoi(searchResults[5]));
+        frames.push_back({
+            .fileName = std::string(searchResults[3]),
+            .methodName = std::string(searchResults[1]),
+            .lineNumber = std::stoi(searchResults[4]),
+            .columnNumber = std::stoi(searchResults[5]),
+        });
       } else if (
           std::regex_search(line, searchResults, REGEX_CHROME) ||
           std::regex_search(line, searchResults, REGEX_NODE)) {
-        frame.putString(kFrameFileName, std::string(searchResults[2]));
-        frame.putString(kFrameMethodName, std::string(searchResults[1]));
-        frame.putInt(kFrameLineNumber, std::stoi(searchResults[3]));
-        frame.putInt(kFrameColumnNumber, std::stoi(searchResults[4]));
+        frames.push_back({
+            .fileName = std::string(searchResults[2]),
+            .methodName = std::string(searchResults[1]),
+            .lineNumber = std::stoi(searchResults[3]),
+            .columnNumber = std::stoi(searchResults[4]),
+        });
       } else {
         continue;
       }
-      frames.push_back(frame.build());
     }
   }
-  errorObj.putMapBufferList(kAllStackFrames, std::move(frames));
-  errorObj.putString(kErrorMessage, "EarlyJsError: " + error.getMessage());
-  // TODO: If needed, can increment exceptionId by 1 each time
-  errorObj.putInt(kExceptionId, 0);
-  errorObj.putBool(kIsFatal, isFatal);
-  return errorObj.build();
+
+  return {
+      .frames = std::move(frames),
+      .message = "EarlyJsError: " + error.getMessage(),
+      .exceptionId = 0,
+      .isFatal = isFatal,
+  };
 }
 
 JsErrorHandler::JsErrorHandler(
-    JsErrorHandler::JsErrorHandlingFunc jsErrorHandlingFunc) {
-  this->_jsErrorHandlingFunc = jsErrorHandlingFunc;
-};
+    JsErrorHandler::JsErrorHandlingFunc jsErrorHandlingFunc)
+    : _jsErrorHandlingFunc(jsErrorHandlingFunc){
+
+      };
 
 JsErrorHandler::~JsErrorHandler() {}
 
 void JsErrorHandler::handleJsError(const jsi::JSError& error, bool isFatal) {
   // TODO: Current error parsing works and is stable. Can investigate using
   // REGEX_HERMES to get additional Hermes data, though it requires JS setup.
-  MapBuffer errorMap = parseErrorStack(error, isFatal, false);
-  _jsErrorHandlingFunc(std::move(errorMap));
+  ParsedError parsedError = parseErrorStack(error, isFatal, false);
+  _jsErrorHandlingFunc(parsedError);
 }
 
 } // namespace facebook::react
diff --git a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h
@@ -8,26 +8,28 @@
 #pragma once
 
 #include <jsi/jsi.h>
-#include <react/renderer/mapbuffer/MapBuffer.h>
 
 namespace facebook::react {
 
-enum JSErrorHandlerKey : uint16_t {
-  kFrameFileName = 0,
-  kFrameMethodName = 1,
-  kFrameLineNumber = 2,
-  kFrameColumnNumber = 3,
-  kAllStackFrames = 4,
-  kErrorMessage = 5,
-  kExceptionId = 6,
-  kIsFatal = 7
-};
-
 class JsErrorHandler {
  public:
-  using JsErrorHandlingFunc = std::function<void(MapBuffer errorMap)>;
-
-  JsErrorHandler(JsErrorHandlingFunc jsErrorHandlingFunc);
+  struct ParsedError {
+    struct StackFrame {
+      std::string fileName;
+      std::string methodName;
+      int lineNumber;
+      int columnNumber;
+    };
+
+    std::vector<StackFrame> frames;
+    std::string message;
+    int exceptionId;
+    bool isFatal;
+  };
+
+  using JsErrorHandlingFunc = std::function<void(const ParsedError& error)>;
+
+  explicit JsErrorHandler(JsErrorHandlingFunc jsErrorHandlingFunc);
   ~JsErrorHandler();
 
   void handleJsError(const jsi::JSError& error, bool isFatal);

diff --git a/packages/react-native/ReactCommon/jserrorhandler/React-jserrorhandler.podspec b/packages/react-native/ReactCommon/jserrorhandler/React-jserrorhandler.podspec
@@ -48,6 +48,5 @@ Pod::Spec.new do |s|
   s.dependency folly_dep_name, folly_version
   s.dependency "React-jsi"
   add_dependency(s, "React-debug")
-  add_dependency(s, "React-Mapbuffer")
 
 end
diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp
@@ -23,7 +23,6 @@
 #include <react/utils/jsi-utils.h>
 #include <iostream>
 #include <memory>
-#include <tuple>
 #include <utility>
 
 namespace facebook::react {
@@ -37,7 +36,7 @@ ReactInstance::ReactInstance(
     : runtime_(std::move(runtime)),
       jsMessageQueueThread_(jsMessageQueueThread),
       timerManager_(std::move(timerManager)),
-      jsErrorHandler_(jsErrorHandlingFunc),
+      jsErrorHandler_(std::move(jsErrorHandlingFunc)),
       hasFatalJsError_(std::make_shared<bool>(false)),
       parentInspectorTarget_(parentInspectorTarget) {
   RuntimeExecutor runtimeExecutor = [weakRuntime = std::weak_ptr(runtime_),

diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.h b/packages/react-native/ReactCommon/react/runtime/ReactInstance.h
@@ -34,7 +34,7 @@ class ReactInstance final : private jsinspector_modern::InstanceTargetDelegate {
       std::unique_ptr<JSRuntime> runtime,
       std::shared_ptr<MessageQueueThread> jsMessageQueueThread,
       std::shared_ptr<TimerManager> timerManager,
-      JsErrorHandler::JsErrorHandlingFunc JsErrorHandlingFunc,
+      JsErrorHandler::JsErrorHandlingFunc jsErrorHandlingFunc,
       jsinspector_modern::HostTarget* parentInspectorTarget = nullptr);
 
   RuntimeExecutor getUnbufferedRuntimeExecutor() noexcept;

diff --git a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm
@@ -245,7 +245,7 @@ - (void)_start
   objCTimerRegistryRawPtr->setTimerManager(timerManager);
 
   __weak __typeof(self) weakSelf = self;
-  auto jsErrorHandlingFunc = [=](MapBuffer errorMap) { [weakSelf _handleJSErrorMap:std::move(errorMap)]; };
+  auto jsErrorHandlingFunc = [=](const JsErrorHandler::ParsedError &error) { [weakSelf _handleJSError:error]; };
 
   // Create the React Instance
   _reactInstance = std::make_unique<ReactInstance>(
@@ -496,28 +496,23 @@ - (void)_notifyEventDispatcherObserversOfEvent_DEPRECATED:(NSNotification *)noti
   }
 }
 
-- (void)_handleJSErrorMap:(facebook::react::MapBuffer)errorMap
+- (void)_handleJSError:(const JsErrorHandler::ParsedError &)error
 {
-  NSString *message = [NSString stringWithCString:errorMap.getString(JSErrorHandlerKey::kErrorMessage).c_str()
-                                         encoding:[NSString defaultCStringEncoding]];
-  std::vector<facebook::react::MapBuffer> frames = errorMap.getMapBufferList(JSErrorHandlerKey::kAllStackFrames);
+  NSString *message = [NSString stringWithCString:error.message.c_str() encoding:[NSString defaultCStringEncoding]];
   NSMutableArray<NSDictionary<NSString *, id> *> *stack = [NSMutableArray new];
-  for (const facebook::react::MapBuffer &mapBuffer : frames) {
-    NSDictionary *frame = @{
-      @"file" : [NSString stringWithCString:mapBuffer.getString(JSErrorHandlerKey::kFrameFileName).c_str()
-                                   encoding:[NSString defaultCStringEncoding]],
-      @"methodName" : [NSString stringWithCString:mapBuffer.getString(JSErrorHandlerKey::kFrameMethodName).c_str()
-                                         encoding:[NSString defaultCStringEncoding]],
-      @"lineNumber" : [NSNumber numberWithInt:mapBuffer.getInt(JSErrorHandlerKey::kFrameLineNumber)],
-      @"column" : [NSNumber numberWithInt:mapBuffer.getInt(JSErrorHandlerKey::kFrameColumnNumber)],
-    };
-    [stack addObject:frame];
+  for (const JsErrorHandler::ParsedError::StackFrame &frame : error.frames) {
+    [stack addObject:@{
+      @"file" : [NSString stringWithCString:frame.fileName.c_str() encoding:[NSString defaultCStringEncoding]],
+      @"methodName" : [NSString stringWithCString:frame.methodName.c_str() encoding:[NSString defaultCStringEncoding]],
+      @"lineNumber" : [NSNumber numberWithInt:frame.lineNumber],
+      @"column" : [NSNumber numberWithInt:frame.columnNumber],
+    }];
   }
   [_delegate instance:self
       didReceiveJSErrorStack:stack
                      message:message
-                 exceptionId:errorMap.getInt(JSErrorHandlerKey::kExceptionId)
-                     isFatal:errorMap.getBool(JSErrorHandlerKey::kIsFatal)];
+                 exceptionId:error.exceptionId
+                     isFatal:error.isFatal];
 }
 
 - (void)didReceiveReloadCommand