Refactor: JsErrorHandler: Rename JsErrorHandlingFunc -> OnJsError

packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactInstance.cpp

@@ -51,7 +51,7 @@ JReactInstance::JReactInstance(
   jsTimerExecutor->cthis()->setTimerManager(timerManager);
 
   jReactExceptionManager_ = jni::make_global(jReactExceptionManager);
-  auto jsErrorHandlingFunc =
+  auto onJsError =
       [weakJReactExceptionManager = jni::make_weak(jReactExceptionManager)](
           const JsErrorHandler::ParsedError& error) mutable noexcept {
         if (auto jReactExceptionManager =
@@ -66,7 +66,7 @@ JReactInstance::JReactInstance(
       jsRuntimeFactory->cthis()->createJSRuntime(sharedJSMessageQueueThread),
       sharedJSMessageQueueThread,
       timerManager,
-      std::move(jsErrorHandlingFunc),
+      std::move(onJsError),
       jReactHostInspectorTarget
           ? jReactHostInspectorTarget->cthis()->getInspectorTarget()
           : nullptr);

packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp

@@ -88,19 +88,24 @@ parseErrorStack(const jsi::JSError& error, bool isFatal, bool isHermes) {
   };
 }
 
-JsErrorHandler::JsErrorHandler(
-    JsErrorHandler::JsErrorHandlingFunc jsErrorHandlingFunc)
-    : _jsErrorHandlingFunc(std::move(jsErrorHandlingFunc)){
+JsErrorHandler::JsErrorHandler(JsErrorHandler::OnJsError onJsError)
+    : _onJsError(std::move(onJsError)),
+      _hasHandledFatalError(false){
 
       };
 
 JsErrorHandler::~JsErrorHandler() {}
 
-void JsErrorHandler::handleJsError(const jsi::JSError& error, bool isFatal) {
+void JsErrorHandler::handleFatalError(const jsi::JSError& error) {
   // TODO: Current error parsing works and is stable. Can investigate using
   // REGEX_HERMES to get additional Hermes data, though it requires JS setup.
-  ParsedError parsedError = parseErrorStack(error, isFatal, false);
-  _jsErrorHandlingFunc(parsedError);
+  _hasHandledFatalError = true;
+  ParsedError parsedError = parseErrorStack(error, true, false);
+  _onJsError(parsedError);
+}
+
+bool JsErrorHandler::hasHandledFatalError() {
+  return _hasHandledFatalError;
 }
 
 } // namespace facebook::react

packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h

@@ -27,15 +27,17 @@ class JsErrorHandler {
     bool isFatal;
   };
 
-  using JsErrorHandlingFunc = std::function<void(const ParsedError& error)>;
+  using OnJsError = std::function<void(const ParsedError& error)>;
 
-  explicit JsErrorHandler(JsErrorHandlingFunc jsErrorHandlingFunc);
+  explicit JsErrorHandler(OnJsError onJsError);
   ~JsErrorHandler();
 
-  void handleJsError(const jsi::JSError& error, bool isFatal);
+  void handleFatalError(const jsi::JSError& error);
+  bool hasHandledFatalError();
 
  private:
-  JsErrorHandlingFunc _jsErrorHandlingFunc;
+  OnJsError _onJsError;
+  bool _hasHandledFatalError;
 };
 
 } // namespace facebook::react

packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp

@@ -34,22 +34,20 @@ void ReactInstanceIntegrationTest::SetUp() {
   auto timerManager =
       std::make_shared<react::TimerManager>(std::move(mockRegistry));
 
-  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 onJsError = [](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_ =
@@ -77,7 +75,7 @@ void ReactInstanceIntegrationTest::SetUp() {
       std::move(runtime_),
       messageQueueThread,
       timerManager,
-      std::move(jsErrorHandlingFunc),
+      std::move(onJsError),
       hostTargetIfModernCDP == nullptr ? nullptr : hostTargetIfModernCDP.get());
 
   timerManager->setRuntimeExecutor(instance->getBufferedRuntimeExecutor());

packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler.cpp

@@ -13,7 +13,6 @@
 #include <react/featureflags/ReactNativeFeatureFlags.h>
 #include <react/renderer/debug/SystraceSection.h>
 #include <utility>
-#include "ErrorUtils.h"
 
 namespace facebook::react {
 

packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.cpp

@@ -8,10 +8,10 @@
 #include "RuntimeScheduler_Legacy.h"
 #include "SchedulerPriorityUtils.h"
 
+#include <cxxreact/ErrorUtils.h>
 #include <react/renderer/consistency/ScopedShadowTreeRevisionLock.h>
 #include <react/renderer/debug/SystraceSection.h>
 #include <utility>
-#include "ErrorUtils.h"
 
 namespace facebook::react {
 
@@ -139,7 +139,7 @@ void RuntimeScheduler_Legacy::callExpiredTasks(jsi::Runtime& runtime) {
       executeTask(runtime, topPriorityTask, didUserCallbackTimeout);
     }
   } catch (jsi::JSError& error) {
-    handleFatalError(runtime, error);
+    handleJSError(runtime, error, true);
   }
 
   currentPriority_ = previousPriority;
@@ -191,7 +191,7 @@ void RuntimeScheduler_Legacy::startWorkLoop(jsi::Runtime& runtime) {
       executeTask(runtime, topPriorityTask, didUserCallbackTimeout);
     }
   } catch (jsi::JSError& error) {
-    handleFatalError(runtime, error);
+    handleJSError(runtime, error, true);
   }
 
   currentPriority_ = previousPriority;

packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.h

@@ -22,8 +22,7 @@ class RuntimeScheduler_Legacy final : public RuntimeSchedulerBase {
  public:
   explicit RuntimeScheduler_Legacy(
       RuntimeExecutor runtimeExecutor,
-      std::function<RuntimeSchedulerTimePoint()> now =
-          RuntimeSchedulerClock::now);
+      std::function<RuntimeSchedulerTimePoint()> now);
 
   /*
    * Not copyable.

packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp

@@ -14,7 +14,6 @@
 #include <react/renderer/debug/SystraceSection.h>
 #include <react/utils/OnScopeExit.h>
 #include <utility>
-#include "ErrorUtils.h"
 
 namespace facebook::react {
 
@@ -211,7 +210,7 @@ void RuntimeScheduler_Modern::startWorkLoop(
       executeTask(runtime, topPriorityTask, currentTime);
     }
   } catch (jsi::JSError& error) {
-    handleFatalError(runtime, error);
+    handleJSError(runtime, error, true);
   }
 
   currentPriority_ = previousPriority;

packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.h

@@ -23,8 +23,7 @@ class RuntimeScheduler_Modern final : public RuntimeSchedulerBase {
  public:
   explicit RuntimeScheduler_Modern(
       RuntimeExecutor runtimeExecutor,
-      std::function<RuntimeSchedulerTimePoint()> now =
-          RuntimeSchedulerClock::now);
+      std::function<RuntimeSchedulerTimePoint()> now);
 
   /*
    * Not copyable.

packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp

@@ -31,30 +31,29 @@ ReactInstance::ReactInstance(
     std::unique_ptr<JSRuntime> runtime,
     std::shared_ptr<MessageQueueThread> jsMessageQueueThread,
     std::shared_ptr<TimerManager> timerManager,
-    JsErrorHandler::JsErrorHandlingFunc jsErrorHandlingFunc,
+    JsErrorHandler::OnJsError onJsError,
     jsinspector_modern::HostTarget* parentInspectorTarget)
     : runtime_(std::move(runtime)),
       jsMessageQueueThread_(jsMessageQueueThread),
       timerManager_(std::move(timerManager)),
-      jsErrorHandler_(std::move(jsErrorHandlingFunc)),
-      hasFatalJsError_(std::make_shared<bool>(false)),
+      jsErrorHandler_(std::make_shared<JsErrorHandler>(std::move(onJsError))),
       parentInspectorTarget_(parentInspectorTarget) {
   RuntimeExecutor runtimeExecutor = [weakRuntime = std::weak_ptr(runtime_),
                                      weakTimerManager =
                                          std::weak_ptr(timerManager_),
                                      weakJsMessageQueueThread =
                                          std::weak_ptr(jsMessageQueueThread_),
-                                     weakHasFatalJsError = std::weak_ptr(
-                                         hasFatalJsError_)](auto callback) {
-    if (std::shared_ptr<bool> sharedHasFatalJsError =
-            weakHasFatalJsError.lock()) {
-      if (*sharedHasFatalJsError) {
-        LOG(INFO)
-            << "Calling into JS using runtimeExecutor but hasFatalJsError_ is true";
-        return;
-      }
+                                     weakJsErrorHander = std::weak_ptr(
+                                         jsErrorHandler_)](auto callback) {
+    auto jsErrorHandler = weakJsErrorHander.lock();
+    if (weakRuntime.expired() || !jsErrorHandler) {
+      return;
     }
-    if (weakRuntime.expired()) {
+
+    if (jsErrorHandler->hasHandledFatalError()) {
+      LOG(INFO)
+          << "RuntimeExecutor: Detected fatal js error. Dropping work on non-js thread."
+          << std::endl;
       return;
     }
 
@@ -163,6 +162,8 @@ RuntimeExecutor ReactInstance::getBufferedRuntimeExecutor() noexcept {
   };
 }
 
+// TODO(T184010230): Should the RuntimeScheduler returned from this method be
+// buffered?
 std::shared_ptr<RuntimeScheduler>
 ReactInstance::getRuntimeScheduler() noexcept {
   return runtimeScheduler_;
@@ -220,9 +221,7 @@ void ReactInstance::loadScript(
             strongBufferedRuntimeExecuter->flush();
           }
         } catch (jsi::JSError& error) {
-          // Handle uncaught JS errors during loading JS bundle
-          *hasFatalJsError_ = true;
-          this->jsErrorHandler_.handleJsError(error, true);
+          jsErrorHandler_->handleFatalError(error);
         }
       });
 }

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::OnJsError onJsError,
       jsinspector_modern::HostTarget* parentInspectorTarget = nullptr);
 
   RuntimeExecutor getUnbufferedRuntimeExecutor() noexcept;
@@ -80,10 +80,7 @@ class ReactInstance final : private jsinspector_modern::InstanceTargetDelegate {
   std::shared_ptr<TimerManager> timerManager_;
   std::unordered_map<std::string, std::shared_ptr<CallableModule>> modules_;
   std::shared_ptr<RuntimeScheduler> runtimeScheduler_;
-  JsErrorHandler jsErrorHandler_;
-
-  // Whether there are errors caught during bundle loading
-  std::shared_ptr<bool> hasFatalJsError_;
+  std::shared_ptr<JsErrorHandler> jsErrorHandler_;
 
   jsinspector_modern::InstanceTarget* inspectorTarget_{nullptr};
   jsinspector_modern::RuntimeTarget* runtimeInspectorTarget_{nullptr};

packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm

@@ -242,14 +242,14 @@ - (void)_start
   objCTimerRegistryRawPtr->setTimerManager(timerManager);
 
   __weak __typeof(self) weakSelf = self;
-  auto jsErrorHandlingFunc = [=](const JsErrorHandler::ParsedError &error) { [weakSelf _handleJSError:error]; };
+  auto onJsError = [=](const JsErrorHandler::ParsedError &error) { [weakSelf _handleJSError:error]; };
 
   // Create the React Instance
   _reactInstance = std::make_unique<ReactInstance>(
       _jsRuntimeFactory->createJSRuntime(_jsThreadManager.jsMessageThread),
       _jsThreadManager.jsMessageThread,
       timerManager,
-      jsErrorHandlingFunc,
+      onJsError,
       _parentInspectorTarget);
   _valid = true;
 

packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp

@@ -123,15 +123,15 @@ class ReactInstanceTest : public ::testing::Test {
     auto mockRegistry = std::make_unique<MockTimerRegistry>();
     mockRegistry_ = mockRegistry.get();
     timerManager_ = std::make_shared<TimerManager>(std::move(mockRegistry));
-    auto jsErrorHandlingFunc =
-        [](const JsErrorHandler::ParsedError& errorMap) noexcept {
-          // Do nothing
-        };
+    auto onJsError = [](const JsErrorHandler::ParsedError& errorMap) noexcept {
+      // Do nothing
+    };
+
     instance_ = std::make_unique<ReactInstance>(
         std::move(runtime),
         messageQueueThread_,
         timerManager_,
-        std::move(jsErrorHandlingFunc));
+        std::move(onJsError));
     timerManager_->setRuntimeExecutor(instance_->getBufferedRuntimeExecutor());
 
     // Install a C++ error handler