From db1043dfbf15b8d2e39eeb50b315384aa39e4606 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Fri, 31 May 2024 05:20:27 -0700 Subject: [PATCH] Continue running microtasks when parent task throws Summary: Changelog: [General][Fixed] Fixed LogBox not showing correctly on the New Architecture We found an incorrect behavior in the event loop, where an error in a task would prevent its microtasks from running. This isn't spec compliant and should be fixed. This caused LogBox to not work correctly, as error reporting is implemented via microtasks that would never execute. Reviewed By: sammy-SC Differential Revision: D58010521 fbshipit-source-id: 7901c5d6e83fb63af148e12ad6c32be490a3999d --- .../RuntimeScheduler_Modern.cpp | 36 +++++++-------- .../react/renderer/runtimescheduler/Task.cpp | 16 ++++--- .../tests/RuntimeSchedulerTest.cpp | 46 +++++++++++++++++++ 3 files changed, 73 insertions(+), 25 deletions(-) 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 036135c9103..9ebda05e428 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp @@ -204,21 +204,17 @@ void RuntimeScheduler_Modern::startWorkLoop( auto previousPriority = currentPriority_; - try { - while (syncTaskRequests_ == 0) { - auto currentTime = now_(); - auto topPriorityTask = selectTask(currentTime, onlyExpired); + while (syncTaskRequests_ == 0) { + auto currentTime = now_(); + auto topPriorityTask = selectTask(currentTime, onlyExpired); - if (!topPriorityTask) { - // No pending work to do. - // Events will restart the loop when necessary. - break; - } - - executeTask(runtime, *topPriorityTask, currentTime); + if (!topPriorityTask) { + // No pending work to do. + // Events will restart the loop when necessary. + break; } - } catch (jsi::JSError& error) { - handleJSError(runtime, error, true); + + executeTask(runtime, *topPriorityTask, currentTime); } currentPriority_ = previousPriority; @@ -310,12 +306,16 @@ void RuntimeScheduler_Modern::executeMacrotask( bool didUserCallbackTimeout) const { SystraceSection s("RuntimeScheduler::executeMacrotask"); - auto result = task.execute(runtime, didUserCallbackTimeout); + try { + auto result = task.execute(runtime, didUserCallbackTimeout); - if (result.isObject() && result.getObject(runtime).isFunction(runtime)) { - // If the task returned a continuation callback, we re-assign it to the task - // and keep the task in the queue. - task.callback = result.getObject(runtime).getFunction(runtime); + if (result.isObject() && result.getObject(runtime).isFunction(runtime)) { + // If the task returned a continuation callback, we re-assign it to the + // task and keep the task in the queue. + task.callback = result.getObject(runtime).getFunction(runtime); + } + } catch (jsi::JSError& error) { + handleJSError(runtime, error, true); } } diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/Task.cpp b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/Task.cpp index 24a0357ac83..6816e38fa55 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/Task.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/Task.cpp @@ -32,20 +32,22 @@ jsi::Value Task::execute(jsi::Runtime& runtime, bool didUserCallbackTimeout) { return result; } - auto& cbVal = callback.value(); + // We get the value of the callback and reset it immediately to avoid it being + // called more than once (including when the callback throws). + auto originalCallback = std::move(*callback); + callback.reset(); - if (cbVal.index() == 0) { + if (originalCallback.index() == 0) { // Callback in JavaScript is expecting a single bool parameter. // React team plans to remove it in the future when a scheduler bug on web // is resolved. - result = - std::get(cbVal).call(runtime, {didUserCallbackTimeout}); + result = std::get(originalCallback) + .call(runtime, {didUserCallbackTimeout}); } else { // Calling a raw callback - std::get(cbVal)(runtime); + std::get(originalCallback)(runtime); } - // Destroying callback to prevent calling it twice. - callback.reset(); + return result; } diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp index 4803da7c735..7ec0e2d38ea 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp @@ -1055,6 +1055,52 @@ TEST_P(RuntimeSchedulerTest, modernTwoThreadsRequestAccessToTheRuntime) { EXPECT_EQ(stubQueue_->size(), 0); } +TEST_P(RuntimeSchedulerTest, errorInTaskShouldNotStopMicrotasks) { + // Only for modern runtime scheduler + if (!GetParam()) { + return; + } + + auto microtaskRan = false; + auto taskRan = false; + + auto callback = createHostFunctionFromLambda([&](bool /* unused */) { + taskRan = true; + + auto microtaskCallback = jsi::Function::createFromHostFunction( + *runtime_, + jsi::PropNameID::forUtf8(*runtime_, "microtask1"), + 3, + [&](jsi::Runtime& /*unused*/, + const jsi::Value& /*unused*/, + const jsi::Value* /*arguments*/, + size_t /*unused*/) -> jsi::Value { + microtaskRan = true; + return jsi::Value::undefined(); + }); + + runtime_->queueMicrotask(microtaskCallback); + + throw jsi::JSError(*runtime_, "Test error"); + + return jsi::Value::undefined(); + }); + + runtimeScheduler_->scheduleTask( + SchedulerPriority::NormalPriority, std::move(callback)); + + EXPECT_EQ(taskRan, false); + EXPECT_EQ(microtaskRan, false); + EXPECT_EQ(stubQueue_->size(), 1); + + stubQueue_->tick(); + + EXPECT_EQ(taskRan, 1); + EXPECT_EQ(microtaskRan, 1); + EXPECT_EQ(stubQueue_->size(), 0); + EXPECT_EQ(stubErrorUtils_->getReportFatalCallCount(), 1); +} + INSTANTIATE_TEST_SUITE_P( UseModernRuntimeScheduler, RuntimeSchedulerTest,