From 55ef537ae3f5d0fef291c710ebd8d59495527b75 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Mon, 8 Apr 2024 12:19:49 -0700 Subject: [PATCH 1/6] Noop: ReactInstance: Add todo above getRuntimeScheduler() Summary: getRuntimeScheduler() allows things to schedule work on the js thread by bypassing main bundle buffering. This is unsafe: almost everything should be using the buffered runtime executor, unless it sets up bindings used in the main bundle. I filed a task for the investigation to see if there's any problems. And added it to the code in this diff. Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D55547899 --- .../react-native/ReactCommon/react/runtime/ReactInstance.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp index 3ed3b1342151..01083b78b501 100644 --- a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp +++ b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp @@ -163,6 +163,8 @@ RuntimeExecutor ReactInstance::getBufferedRuntimeExecutor() noexcept { }; } +// TODO(T184010230): Should the RuntimeScheduler returned from this method be +// buffered? std::shared_ptr ReactInstance::getRuntimeScheduler() noexcept { return runtimeScheduler_; From 15d6a0e32d20a40887866b2d6f61f4a78f13d49d Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Mon, 8 Apr 2024 12:19:49 -0700 Subject: [PATCH 2/6] Refactor: RuntimeScheduler: Delete ErrorUtils.h Summary: RuntimeScheduler's ErrorUtils.h is redundant. Let's just remove it. Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D55547905 --- .../renderer/runtimescheduler/ErrorUtils.h | 36 ------------------- .../runtimescheduler/RuntimeScheduler.cpp | 1 - .../RuntimeScheduler_Legacy.cpp | 6 ++-- .../RuntimeScheduler_Modern.cpp | 3 +- 4 files changed, 4 insertions(+), 42 deletions(-) delete mode 100644 packages/react-native/ReactCommon/react/renderer/runtimescheduler/ErrorUtils.h diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/ErrorUtils.h b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/ErrorUtils.h deleted file mode 100644 index c15de49aac6b..000000000000 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/ErrorUtils.h +++ /dev/null @@ -1,36 +0,0 @@ -/* - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#pragma once - -#include - -namespace facebook::react { - -inline static void handleFatalError( - jsi::Runtime& runtime, - const jsi::JSError& error) { - auto reportFatalError = "reportFatalError"; - auto errorUtils = runtime.global().getProperty(runtime, "ErrorUtils"); - if (errorUtils.isUndefined() || !errorUtils.isObject() || - !errorUtils.getObject(runtime).hasProperty(runtime, reportFatalError)) { - // ErrorUtils was not set up. This probably means the bundle didn't - // load properly. - throw jsi::JSError( - runtime, - "ErrorUtils is not set up properly. Something probably went wrong trying to load the JS bundle. Trying to report error " + - error.getMessage(), - error.getStack()); - } - - auto func = errorUtils.asObject(runtime).getPropertyAsFunction( - runtime, reportFatalError); - - func.call(runtime, error.value()); -} - -} // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler.cpp b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler.cpp index ffad447dc366..804e7cf10be9 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler.cpp @@ -13,7 +13,6 @@ #include #include #include -#include "ErrorUtils.h" namespace facebook::react { diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.cpp b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.cpp index 2d13b9b8e645..e16188137413 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.cpp @@ -8,10 +8,10 @@ #include "RuntimeScheduler_Legacy.h" #include "SchedulerPriorityUtils.h" +#include #include #include #include -#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; diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp index c07e2f8c2e9c..8cc6a6deebb0 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp @@ -14,7 +14,6 @@ #include #include #include -#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; From f161d2894b308510a069853e63a379c528c8ce21 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Mon, 8 Apr 2024 12:19:49 -0700 Subject: [PATCH 3/6] Refactor: RuntimeScheduler: Stop defaulting ctor args redundantly Summary: Now, all the defaulting is in RuntimeScheduler.h. Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D55547900 --- .../react/renderer/runtimescheduler/RuntimeScheduler_Legacy.h | 3 +-- .../react/renderer/runtimescheduler/RuntimeScheduler_Modern.h | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.h b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.h index d5f673d39b66..502c0def5ef0 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Legacy.h +++ b/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 now = - RuntimeSchedulerClock::now); + std::function now); /* * Not copyable. diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.h b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.h index 372d8c5da19a..f730509928ba 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.h +++ b/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 now = - RuntimeSchedulerClock::now); + std::function now); /* * Not copyable. From 8dbf114e9c8c546e96de8345e8b663a411f663eb Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Mon, 8 Apr 2024 12:19:49 -0700 Subject: [PATCH 4/6] Refactor: ReactInstance: Store JsErrorHandler inside a shared_ptr Summary: Just makes it easier to pass around JsErrorHandler. We'll need this in D55547897, when we start storing the "has fataled" boolean inside the JsErrorHandler. Changelog: [internal] Reviewed By: cipolleschi Differential Revision: D55547898 --- .../react-native/ReactCommon/react/runtime/ReactInstance.cpp | 5 +++-- .../react-native/ReactCommon/react/runtime/ReactInstance.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp index 01083b78b501..2571fed723b0 100644 --- a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp +++ b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp @@ -36,7 +36,8 @@ ReactInstance::ReactInstance( : runtime_(std::move(runtime)), jsMessageQueueThread_(jsMessageQueueThread), timerManager_(std::move(timerManager)), - jsErrorHandler_(std::move(jsErrorHandlingFunc)), + jsErrorHandler_( + std::make_shared(std::move(jsErrorHandlingFunc))), hasFatalJsError_(std::make_shared(false)), parentInspectorTarget_(parentInspectorTarget) { RuntimeExecutor runtimeExecutor = [weakRuntime = std::weak_ptr(runtime_), @@ -224,7 +225,7 @@ void ReactInstance::loadScript( } catch (jsi::JSError& error) { // Handle uncaught JS errors during loading JS bundle *hasFatalJsError_ = true; - this->jsErrorHandler_.handleJsError(error, true); + jsErrorHandler_->handleJsError(error, true); } }); } diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.h b/packages/react-native/ReactCommon/react/runtime/ReactInstance.h index c785147f72bc..48393cb9a06d 100644 --- a/packages/react-native/ReactCommon/react/runtime/ReactInstance.h +++ b/packages/react-native/ReactCommon/react/runtime/ReactInstance.h @@ -80,7 +80,7 @@ class ReactInstance final : private jsinspector_modern::InstanceTargetDelegate { std::shared_ptr timerManager_; std::unordered_map> modules_; std::shared_ptr runtimeScheduler_; - JsErrorHandler jsErrorHandler_; + std::shared_ptr jsErrorHandler_; // Whether there are errors caught during bundle loading std::shared_ptr hasFatalJsError_; From 71391db7ed398e663006a45c1c2657df9ecc43b0 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Mon, 8 Apr 2024 12:19:49 -0700 Subject: [PATCH 5/6] Refactor: ReactInstance: Pull "fatal error" bool into JsErrorHandler Summary: I think we should try to centralize all things js error handling related inside JsErrorHandler. So, I moved this bool into JsErrorHandler. This makes ReactInstance easier to understand: it removes one member variable from ReactInstance. Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D55547897 --- .../jserrorhandler/JsErrorHandler.cpp | 10 +++++++- .../jserrorhandler/JsErrorHandler.h | 2 ++ .../react/runtime/ReactInstance.cpp | 23 ++++++++----------- .../ReactCommon/react/runtime/ReactInstance.h | 3 --- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp index 9b474c1e1391..1c00b15dc951 100644 --- a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp +++ b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp @@ -90,7 +90,8 @@ parseErrorStack(const jsi::JSError& error, bool isFatal, bool isHermes) { JsErrorHandler::JsErrorHandler( JsErrorHandler::JsErrorHandlingFunc jsErrorHandlingFunc) - : _jsErrorHandlingFunc(std::move(jsErrorHandlingFunc)){ + : _jsErrorHandlingFunc(std::move(jsErrorHandlingFunc)), + _hasHandledFatalError(false){ }; @@ -99,8 +100,15 @@ 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. + if (isFatal) { + _hasHandledFatalError = true; + } ParsedError parsedError = parseErrorStack(error, isFatal, false); _jsErrorHandlingFunc(parsedError); } +bool JsErrorHandler::hasHandledFatalError() { + return _hasHandledFatalError; +} + } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h index 69f714d48536..d7e16eb9e6b8 100644 --- a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h +++ b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h @@ -33,9 +33,11 @@ class JsErrorHandler { ~JsErrorHandler(); void handleJsError(const jsi::JSError& error, bool isFatal); + bool hasHandledFatalError(); private: JsErrorHandlingFunc _jsErrorHandlingFunc; + bool _hasHandledFatalError; }; } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp index 2571fed723b0..1984ab3c3f12 100644 --- a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp +++ b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp @@ -38,24 +38,23 @@ ReactInstance::ReactInstance( timerManager_(std::move(timerManager)), jsErrorHandler_( std::make_shared(std::move(jsErrorHandlingFunc))), - hasFatalJsError_(std::make_shared(false)), 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 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; } @@ -223,8 +222,6 @@ void ReactInstance::loadScript( strongBufferedRuntimeExecuter->flush(); } } catch (jsi::JSError& error) { - // Handle uncaught JS errors during loading JS bundle - *hasFatalJsError_ = true; jsErrorHandler_->handleJsError(error, true); } }); diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.h b/packages/react-native/ReactCommon/react/runtime/ReactInstance.h index 48393cb9a06d..d44b539bd7eb 100644 --- a/packages/react-native/ReactCommon/react/runtime/ReactInstance.h +++ b/packages/react-native/ReactCommon/react/runtime/ReactInstance.h @@ -82,9 +82,6 @@ class ReactInstance final : private jsinspector_modern::InstanceTargetDelegate { std::shared_ptr runtimeScheduler_; std::shared_ptr jsErrorHandler_; - // Whether there are errors caught during bundle loading - std::shared_ptr hasFatalJsError_; - jsinspector_modern::InstanceTarget* inspectorTarget_{nullptr}; jsinspector_modern::RuntimeTarget* runtimeInspectorTarget_{nullptr}; jsinspector_modern::HostTarget* parentInspectorTarget_{nullptr}; From 98e88311a822febd81680f549e95ab13d880032d Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Mon, 8 Apr 2024 12:19:49 -0700 Subject: [PATCH 6/6] Refactor: JsErrorHandler: Rename handleJsError to handleFatalError (#43957) Summary: Right now, JsErrorHandler is only used to handle fatal exceptions. So, let's just scope handleJsError down to handleFatalError. Changelog: [General][Breaking] - JsErrorHandler: Rename handleJsError to handleFatalError Reviewed By: cortinico Differential Revision: D55547901 --- .../ReactCommon/jserrorhandler/JsErrorHandler.cpp | 8 +++----- .../ReactCommon/jserrorhandler/JsErrorHandler.h | 2 +- .../ReactCommon/react/runtime/ReactInstance.cpp | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp index 1c00b15dc951..d70a4463761e 100644 --- a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp +++ b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp @@ -97,13 +97,11 @@ JsErrorHandler::JsErrorHandler( 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. - if (isFatal) { - _hasHandledFatalError = true; - } - ParsedError parsedError = parseErrorStack(error, isFatal, false); + _hasHandledFatalError = true; + ParsedError parsedError = parseErrorStack(error, true, false); _jsErrorHandlingFunc(parsedError); } diff --git a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h index d7e16eb9e6b8..53a03367989f 100644 --- a/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h +++ b/packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h @@ -32,7 +32,7 @@ class JsErrorHandler { explicit JsErrorHandler(JsErrorHandlingFunc jsErrorHandlingFunc); ~JsErrorHandler(); - void handleJsError(const jsi::JSError& error, bool isFatal); + void handleFatalError(const jsi::JSError& error); bool hasHandledFatalError(); private: diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp index 1984ab3c3f12..440e9f5de576 100644 --- a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp +++ b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp @@ -222,7 +222,7 @@ void ReactInstance::loadScript( strongBufferedRuntimeExecuter->flush(); } } catch (jsi::JSError& error) { - jsErrorHandler_->handleJsError(error, true); + jsErrorHandler_->handleFatalError(error); } }); }