From 22ad30770b3a827893f762cfd7809598dcde2172 Mon Sep 17 00:00:00 2001
From: Moti Zilberman <moti@meta.com>
Date: Sat, 27 Apr 2024 23:59:59 -0700
Subject: [PATCH] Fix use-after-move in console implementation

Summary:
Changelog: [Internal]

In the new CDP backend, calling any `console` method a second time involves a call to a moved-from `std::function`. This shouldn't work, and indeed results in an exception on some platforms, but isn't strictly an error according to the C++ standard: moving from a `std::function` leaves it in an [unspecified state](https://en.cppreference.com/w/cpp/utility/functional/function/function#:~:text=the%20call%20too.-,other,is%20in%20a%20valid%20but%20unspecified%20state%20right%20after%20the%20call.,-5), not necessarily an empty state, so (in particular) it's perfectly legal for the implementation to perform a copy instead of a move and leave the original variable intact.

(See [Compiler Explorer](https://godbolt.org/z/qoo5Mnd68) for proof that libc++ and libstdc++ differ on this - the former performs a copy, while the latter actually performs a move, resulting in a `std::bad_function_call` exception later.)

In the code in question, we're right to want to avoid a copy of the `body` function into the argument of `delegateExecutorSync` - only one copy of this function needs to exist at a time. But the correct way to avoid this copy is to capture `body` by reference, as we can do that repeatedly with no ill effects. (`delegateExecutorSync` is, as its name suggests, synchronous, so there are no lifetime issues.) Doing this also allows us to remove the use of `mutable` so the capturing is by *const* reference.

Differential Revision: D56673529
---
 .../RuntimeTargetConsole.cpp                  | 36 ++++++++-----------
 .../tests/ConsoleApiTest.cpp                  | 10 ++++++
 2 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTargetConsole.cpp b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTargetConsole.cpp
index 0fb848653a45e7..c773907f8a7fdf 100644
--- a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTargetConsole.cpp
+++ b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTargetConsole.cpp
@@ -155,7 +155,7 @@ void RuntimeTarget::installConsoleHandler() {
                  jsi::Runtime& runtime,
                  const jsi::Value& thisVal,
                  const jsi::Value* args,
-                 size_t count) mutable {
+                 size_t count) {
         jsi::Value retVal = innerFn(runtime, thisVal, args, count);
         if (originalConsole) {
           auto val = originalConsole->getProperty(runtime, methodName);
@@ -202,27 +202,21 @@ void RuntimeTarget::installConsoleHandler() {
                           jsi::Runtime& runtime,
                           const jsi::Value& /*thisVal*/,
                           const jsi::Value* args,
-                          size_t count) mutable {
+                          size_t count) {
                         auto timestampMs = getTimestampMs();
-                        delegateExecutorSync(
-                            [&runtime,
-                             args,
-                             count,
-                             body = std::move(body),
-                             state,
-                             timestampMs](auto& runtimeTargetDelegate) {
-                              auto stackTrace =
-                                  runtimeTargetDelegate.captureStackTrace(
-                                      runtime, /* framesToSkip */ 1);
-                              body(
-                                  runtime,
-                                  args,
-                                  count,
-                                  runtimeTargetDelegate,
-                                  *state,
-                                  timestampMs,
-                                  std::move(stackTrace));
-                            });
+                        delegateExecutorSync([&](auto& runtimeTargetDelegate) {
+                          auto stackTrace =
+                              runtimeTargetDelegate.captureStackTrace(
+                                  runtime, /* framesToSkip */ 1);
+                          body(
+                              runtime,
+                              args,
+                              count,
+                              runtimeTargetDelegate,
+                              *state,
+                              timestampMs,
+                              std::move(stackTrace));
+                        });
                         return jsi::Value::undefined();
                       })));
         };
diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/ConsoleApiTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/ConsoleApiTest.cpp
index 98d83362195b1e..1dec6c86d4ca2d 100644
--- a/packages/react-native/ReactCommon/jsinspector-modern/tests/ConsoleApiTest.cpp
+++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/ConsoleApiTest.cpp
@@ -748,6 +748,16 @@ TEST_P(ConsoleApiTest, testConsoleLogStack) {
   )");
 }
 
+TEST_P(ConsoleApiTest, testConsoleLogTwice) {
+  InSequence s;
+  expectConsoleApiCall(
+      AllOf(AtJsonPtr("/type", "log"), AtJsonPtr("/args/0/value", "hello")));
+  eval("console.log('hello');");
+  expectConsoleApiCall(AllOf(
+      AtJsonPtr("/type", "log"), AtJsonPtr("/args/0/value", "hello again")));
+  eval("console.log('hello again');");
+}
+
 static const auto paramValues = testing::Values(
     Params{
         .withConsolePolyfill = true,