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 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);