From 1aad1a3506ef4545bae8806dff6899eff6573010 Mon Sep 17 00:00:00 2001 From: Gijs Weterings Date: Mon, 17 Aug 2026 04:52:50 -0700 Subject: [PATCH 1/2] Fix DOMHighResTimeStamp round-trip truncation in timing primitives (#57975) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: `HighResDuration::fromDOMHighResTimeStamp` and `HighResTimeStamp::fromDOMHighResTimeStamp` converted milliseconds back to nanoseconds with `static_cast(units * 1e6)`, which truncates toward zero. `toDOMHighResTimeStamp()` divides the nanosecond count by 1e6 (one rounding) and multiplying back by 1e6 rounds again, so the product frequently lands a hair below the original integer (e.g. `537648854729249.97`). Truncation then chops off a whole nanosecond, so `fromDOMHighResTimeStamp(toDOMHighResTimeStamp(x)) != x` for roughly 2% of random `now()` values. That is the source of an intermittent `BridgingTest/highResTimeStampTest` failure, which reported: ``` Expected equality of these values: timestamp Which is: 8-byte object <22-02 00-21 FD-E8 01-00> bridging::fromJs( rt, bridging::toJs(rt, timestamp), invoker) Which is: 8-byte object <21-02 00-21 FD-E8 01-00> ``` Round to the nearest nanosecond instead of truncating. Nanosecond values below 2^53 are exactly representable in a double, so the round trip is now exact for every value below 2.25e15 ns (26 days of monotonic clock); beyond that the double's ULP exceeds 0.5 ns and residual error is at most 2 ns, which is a floor of the DOM representation itself. Rounding is also the correct semantic independently of the round trip — truncation gives a systematic downward bias and is asymmetric across zero, which affects the other callers (`RCTHighResTimeStampFromSeconds` for touch timestamps, `RuntimeTargetConsole` `console.timeStamp`, and `PerformanceTracer`). It is implemented without `` so the header stays `constexpr`-safe and C++17/20-portable. `HighResTimeStamp::fromDOMHighResTimeStamp` now delegates to `HighResDuration`'s so there is a single implementation. `highResTimeStampTest` previously asserted on `HighResTimeStamp::now()`, whose magnitude is host-uptime-dependent, so it only tripped the bug on about 2% of runs. It now round-trips five fixed nanosecond values (including the exact value from the failing run), which exercises the bug on every run. No test was skipped, disabled, or loosened. Changelog: [General][Fixed] - Round instead of truncate when converting a `DOMHighResTimeStamp` back to nanoseconds, so `HighResTimeStamp` and `HighResDuration` round trips are exact Differential Revision: D116286868 --- .../react/bridging/tests/BridgingTest.cpp | 37 +++++++++---- .../ReactCommon/react/timing/primitives.h | 20 +++++-- .../react/timing/tests/PrimitivesTest.cpp | 52 +++++++++++++++++++ 3 files changed, 95 insertions(+), 14 deletions(-) diff --git a/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.cpp b/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.cpp index cec7c92657c8..baebdf7cde1a 100644 --- a/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.cpp +++ b/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.cpp @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +#include #include #include @@ -955,17 +956,33 @@ TEST_F(BridgingTest, asyncArrayBufferBridgingTest) { } TEST_F(BridgingTest, highResTimeStampTest) { - HighResTimeStamp timestamp = HighResTimeStamp::now(); - EXPECT_EQ( - timestamp, - bridging::fromJs( - rt, bridging::toJs(rt, timestamp), invoker)); + // Nanosecond counts that are not a whole number of milliseconds, spanning the + // magnitudes a monotonic clock reports (seconds to weeks since boot). Fixed + // values are used instead of `HighResTimeStamp::now()` so that the precision + // of the round trip does not depend on the uptime of the host running this + // test. + for (int64_t nanoseconds : + {int64_t{1}, + int64_t{999'999}, + int64_t{1'000'001}, + int64_t{12'345'678'901}, + int64_t{537'648'854'729'250}}) { + auto timestamp = HighResTimeStamp::fromChronoSteadyClockTimePoint( + std::chrono::steady_clock::time_point( + std::chrono::nanoseconds(nanoseconds))); + EXPECT_EQ( + timestamp, + bridging::fromJs( + rt, bridging::toJs(rt, timestamp), invoker)) + << "timestamp of " << nanoseconds << "ns did not round trip"; - auto duration = HighResDuration::fromNanoseconds(1); - EXPECT_EQ( - duration, - bridging::fromJs( - rt, bridging::toJs(rt, duration), invoker)); + auto duration = HighResDuration::fromNanoseconds(nanoseconds); + EXPECT_EQ( + duration, + bridging::fromJs( + rt, bridging::toJs(rt, duration), invoker)) + << "duration of " << nanoseconds << "ns did not round trip"; + } EXPECT_EQ(1.0, bridging::toJs(rt, HighResDuration::fromNanoseconds(1e6))); EXPECT_EQ( diff --git a/packages/react-native/ReactCommon/react/timing/primitives.h b/packages/react-native/ReactCommon/react/timing/primitives.h index 440e227a3340..968e96f6b31a 100644 --- a/packages/react-native/ReactCommon/react/timing/primitives.h +++ b/packages/react-native/ReactCommon/react/timing/primitives.h @@ -62,8 +62,19 @@ class HighResDuration { // @see https://developer.mozilla.org/en-US/docs/Web/API/DOMHighResTimeStamp static constexpr HighResDuration fromDOMHighResTimeStamp(double units) { - auto nanoseconds = static_cast(units * 1e6); - return fromNanoseconds(nanoseconds); + double nanoseconds = units * 1e6; + // Both the conversion to milliseconds and the multiplication back are + // inexact, so `nanoseconds` often lands just below the original count. + // Rounding to the nearest nanosecond (rather than truncating towards zero) + // is what makes the round trip through a DOMHighResTimeStamp lossless. + auto result = static_cast(nanoseconds); + auto remainder = nanoseconds - static_cast(result); + if (remainder >= 0.5) { + result++; + } else if (remainder <= -0.5) { + result--; + } + return fromNanoseconds(result); } // @see https://developer.mozilla.org/en-US/docs/Web/API/DOMHighResTimeStamp @@ -227,8 +238,9 @@ class HighResTimeStamp { // @see https://developer.mozilla.org/en-US/docs/Web/API/DOMHighResTimeStamp static constexpr HighResTimeStamp fromDOMHighResTimeStamp(double units) { - auto nanoseconds = static_cast(units * 1e6); - return HighResTimeStamp(std::chrono::steady_clock::time_point(std::chrono::nanoseconds(nanoseconds))); + return HighResTimeStamp( + std::chrono::steady_clock::time_point( + static_cast(HighResDuration::fromDOMHighResTimeStamp(units)))); } // @see https://developer.mozilla.org/en-US/docs/Web/API/DOMHighResTimeStamp diff --git a/packages/react-native/ReactCommon/react/timing/tests/PrimitivesTest.cpp b/packages/react-native/ReactCommon/react/timing/tests/PrimitivesTest.cpp index f94a379c036d..f2ed44c09884 100644 --- a/packages/react-native/ReactCommon/react/timing/tests/PrimitivesTest.cpp +++ b/packages/react-native/ReactCommon/react/timing/tests/PrimitivesTest.cpp @@ -30,6 +30,58 @@ TEST(HighResDuration, CorrectlyConvertsToDOMHighResTimeStamp) { HighResDuration::fromMilliseconds(10).toDOMHighResTimeStamp(), 10.0); } +TEST(HighResDuration, CorrectlyConvertsFromDOMHighResTimeStamp) { + EXPECT_EQ( + HighResDuration::fromDOMHighResTimeStamp(0.00001).toNanoseconds(), 10); + EXPECT_EQ( + HighResDuration::fromDOMHighResTimeStamp(1.000001).toNanoseconds(), + 1000001); + EXPECT_EQ( + HighResDuration::fromDOMHighResTimeStamp(-1.000001).toNanoseconds(), + -1000001); + EXPECT_EQ(HighResDuration::fromDOMHighResTimeStamp(0).toNanoseconds(), 0); +} + +TEST(HighResDuration, RoundTripsThroughDOMHighResTimeStamp) { + // A DOMHighResTimeStamp is a double holding milliseconds, so converting back + // to nanoseconds has to round: neither conversion is exact, and truncating + // would drop a nanosecond whenever the result lands just below the original + // value. + for (int64_t nanoseconds : + {int64_t{1}, + int64_t{999'999}, + int64_t{1'000'001}, + int64_t{12'345'678'901}, + int64_t{537'648'854'729'250}}) { + for (int64_t sign : {1, -1}) { + auto duration = HighResDuration::fromNanoseconds(sign * nanoseconds); + EXPECT_EQ( + HighResDuration::fromDOMHighResTimeStamp( + duration.toDOMHighResTimeStamp()), + duration) + << "duration of " << sign * nanoseconds << "ns did not round trip"; + } + } +} + +TEST(HighResTimeStamp, RoundTripsThroughDOMHighResTimeStamp) { + for (int64_t nanoseconds : + {int64_t{1}, + int64_t{999'999}, + int64_t{1'000'001}, + int64_t{12'345'678'901}, + int64_t{537'648'854'729'250}}) { + auto timestamp = HighResTimeStamp::fromChronoSteadyClockTimePoint( + std::chrono::steady_clock::time_point( + std::chrono::nanoseconds(nanoseconds))); + EXPECT_EQ( + HighResTimeStamp::fromDOMHighResTimeStamp( + timestamp.toDOMHighResTimeStamp()), + timestamp) + << "timestamp of " << nanoseconds << "ns did not round trip"; + } +} + TEST(HighResDuration, ComparisonOperators) { auto duration1 = HighResDuration::fromNanoseconds(10); auto duration2 = HighResDuration::fromNanoseconds(20); From 89b30a66a8db44aff1cb624289ffd35fce9ba642 Mon Sep 17 00:00:00 2001 From: Gijs Weterings Date: Mon, 17 Aug 2026 04:52:50 -0700 Subject: [PATCH 2/2] Run the ReactAndroid JNI gtests as instrumentation tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: The `ReactAndroid` JNI gtests in `react/fabric/test` and `react/jni/test` were configured as plain host C++ tests, but they link against the Android-only JNI libraries. The resulting binary is an Android aarch64 ELF that needs `/system/bin/linker64` and so cannot run on a Linux host: ``` qemu-aarch64: Could not open '/system/bin/linker64': No such file or directory Test failed to produce the expected output! ... IO error: No result xml files found. ``` They were not failing their assertions — they were never executing. The runner enumerated case names statically out of the binary without running it, so all 6 cases across the two targets were known and reported as failing, with no stack traces at all. The absence of stack traces was itself the tell: the process never started, so no gtest XML was ever produced. Both targets move to the established pattern for gtests against Android-only native libs: build them as Android instrumentation tests, packaged into an APK and run on a device or emulator. `FabricMountingManagerTest` can drop its deliberate leak as a result. It previously allocated the manager with a no-op deleter, because `~FabricMountingManager()` calls `jni::ThreadScope::WithClassLoader`, which throws without an attached `JavaVM`. Under instrumentation the gtest runs inside a native method registered via fbjni `makeNativeMethod`, so `cachedOrNull()` is non-null and the closure runs inline — destruction is safe. The fixture now holds a default-constructed (null) `jni::global_ref` member and returns a `std::unique_ptr`; releasing a null `global_ref` is a no-op. All four test bodies are byte-identical. `ModuleRegistryBuilderTest.cpp` is a comment-only correction. Changelog: [Internal] Differential Revision: D116286866 --- .../fabric/test/FabricMountingManagerTest.cpp | 38 ++++++++----------- .../jni/test/ModuleRegistryBuilderTest.cpp | 4 +- 2 files changed, 17 insertions(+), 25 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/FabricMountingManagerTest.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/FabricMountingManagerTest.cpp index 6f82841063dc..cb86d39f8220 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/FabricMountingManagerTest.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/FabricMountingManagerTest.cpp @@ -32,32 +32,24 @@ namespace facebook::react { * `allocatedViewsMutex_`; they never call into Java. That is what makes them * unit-testable here. * - * NOTE: `~FabricMountingManager()` calls `jni::ThreadScope::WithClassLoader`, - * which throws `std::runtime_error` when no JavaVM has been attached (see - * `xplat/spectrum/androidLibs/fbjni/cxx/fbjni/detail/Environment.h`). Because - * no JVM is available in this host-side test, the manager instance is - * intentionally never destroyed: each test allocates a single - * `FabricMountingManager` on the heap and wraps it in a `std::shared_ptr` - * with a no-op deleter. The resulting per-test leak is bounded (one ~100-byte - * instance) and does not cross test boundaries. + * `react/fabric:jni` is an Android-only native library, so this target is + * built as a JNI instrumentation test: the gtest binary is packaged into an + * APK and executed on a device/emulator. A JavaVM is therefore attached, which + * `~FabricMountingManager()` requires — it calls + * `jni::ThreadScope::WithClassLoader`, which throws `std::runtime_error` when + * fbjni has not been initialized (see + * `xplat/spectrum/androidLibs/fbjni/cxx/fbjni/detail/Environment.h`). */ class FabricMountingManagerTest : public ::testing::Test { protected: - // Returns a `FabricMountingManager` whose destructor is suppressed. - // See the class-level note above for why this is necessary. - static std::shared_ptr makeManager() { - // `jni::global_ref<>` default-constructs to an empty (null) reference, - // so no JNI calls are performed during construction. The empty - // reference is safe to copy into the manager because the - // surface-registry methods under test never dereference - // `javaUIManager_`. - auto* emptyRef = new jni::global_ref(); - auto* raw = new FabricMountingManager(*emptyRef); - // `emptyRef` is intentionally leaked: resetting it would also engage - // `WithClassLoader`, which requires an attached JavaVM. - return {raw, [](FabricMountingManager* /*unused*/) noexcept { - // No-op deleter: see class-level note. - }}; + // An empty (null) reference: `jni::global_ref<>` default-constructs to null, + // so no JNI call happens when it is created, copied into the manager, or + // released. The surface-registry methods under test never dereference + // `javaUIManager_`. + jni::global_ref emptyUIManager_; + + std::unique_ptr makeManager() { + return std::make_unique(emptyUIManager_); } }; diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/test/ModuleRegistryBuilderTest.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/test/ModuleRegistryBuilderTest.cpp index bd9ac941b05a..7fd7f1478cd1 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/test/ModuleRegistryBuilderTest.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/test/ModuleRegistryBuilderTest.cpp @@ -25,7 +25,7 @@ namespace facebook::react { * exercised by the Robolectric / instrumentation tests that run against a * real JVM. * - * The one branch that can be validated host-side without an attached JavaVM + * The one branch that can be validated without calling into Java at all * is `buildNativeModuleList`'s null-collection guard: when the incoming * `alias_ref>` is a null reference, the function must * short-circuit and return an empty vector rather than dereferencing the @@ -47,7 +47,7 @@ namespace facebook::react { * bring-up paths that legitimately pass no legacy Java modules). Because * the crash would only surface once the process actually reaches this * code with a null collection, catching it here — instead of relying on - * a device-side smoke test — is the earliest signal available. + * an app-level smoke test — is the earliest signal available. * * The `Instance` weak_ptr and `MessageQueueThread` shared_ptr are supplied * as empty on purpose: the guard runs before either is dereferenced, so