[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: [Internal] Reviewed By: fkgozali Differential Revision: D55265170
RSNara · Apr 7, 2024 · f2be3ad · f2be3ad
1 parent 5927165
commit f2be3ad
Show file tree

Hide file tree

Showing 13 changed files with 183 additions and 90 deletions.
diff --git a/packages/react-native/ReactAndroid/api/ReactAndroid.api b/packages/react-native/ReactAndroid/api/ReactAndroid.api
@@ -2830,6 +2830,20 @@ public abstract interface class com/facebook/react/interfaces/TaskInterface {
 	public abstract fun waitForCompletion (JLjava/util/concurrent/TimeUnit;)Z
 }
 
+public abstract interface class com/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedError {
+	public abstract fun getExceptionId ()I
+	public abstract fun getFrames ()Ljava/util/List;
+	public abstract fun getMessage ()Ljava/lang/String;
+	public abstract fun isFatal ()Z
+}
+
+public abstract interface class com/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedError$StackFrame {
+	public abstract fun getColumnNumber ()I
+	public abstract fun getFileName ()Ljava/lang/String;
+	public abstract fun getLineNumber ()I
+	public abstract fun getMethodName ()Ljava/lang/String;
+}
+
 public abstract interface class com/facebook/react/interfaces/fabric/ReactSurface {
 	public abstract fun clear ()V
 	public abstract fun detach ()V

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(std::move(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/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp
@@ -34,12 +34,22 @@ void ReactInstanceIntegrationTest::SetUp() {
   auto timerManager =
       std::make_shared<react::TimerManager>(std::move(mockRegistry));
 
-  auto jsErrorHandlingFunc = [](react::MapBuffer error) noexcept {
-    LOG(INFO) << "Error: \nFile: " << error.getString(react::kFrameFileName)
-              << "\nLine: " << error.getInt(react::kFrameLineNumber)
-              << "\nColumn: " << error.getInt(react::kFrameColumnNumber)
-              << "\nMethod: " << error.getString(react::kFrameMethodName);
-  };
+  auto jsErrorHandlingFunc =
+      [](const JsErrorHandler::ParsedError& errorMap) noexcept {
+        LOG(INFO) << "[jsErrorHandlingFunc called]";
+        LOG(INFO) << "message: " << errorMap.message;
+        LOG(INFO) << "exceptionId: " << std::to_string(errorMap.exceptionId);
+        LOG(INFO) << "isFatal: "
+                  << std::to_string(static_cast<int>(errorMap.isFatal));
+        auto frames = errorMap.frames;
+        for (const auto& mapBuffer : frames) {
+          LOG(INFO) << "[Frame]" << std::endl
+                    << "\tfile: " << mapBuffer.fileName;
+          LOG(INFO) << "\tmethodName: " << mapBuffer.methodName;
+          LOG(INFO) << "\tlineNumber: " << std::to_string(mapBuffer.lineNumber);
+          LOG(INFO) << "\tcolumn: " << std::to_string(mapBuffer.columnNumber);
+        }
+      };
 
   auto jsRuntimeFactory = std::make_unique<react::HermesInstance>();
   std::unique_ptr<react::JSRuntime> runtime_ =

diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactNativeMocks.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactNativeMocks.cpp
@@ -6,6 +6,7 @@
  */
 
 #include "ReactNativeMocks.h"
+#include <glog/logging.h>
 
 namespace facebook::react::jsinspector_modern {
 

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_),