Skip to content

Commit

Permalink
Change jsinspector back to a shared library in the CMake build (faceb…
Browse files Browse the repository at this point in the history
…ook#42144)

Summary:
Pull Request resolved: facebook#42144

D51895785 changed several CMake libraries from shared to static, including `jsinspector`. This happens to be semantically incorrect in the case of `jsinspector`, as the library contains singletons which can be inadvertently duplicated due to static linking. As a result, different parts of the code can end up accessing different instances of a supposed singleton, leading to bugs.

Here we revert the change to `jsinspector` (only) and add an explanatory comment to signpost this for future readers.

## More context & general principle

While nothing is broken today, allowing static libraries to contain global state is brittle and breaks in surprising ways:

* The upcoming diff D52231237 introduces a new dependency on `jsinspector` which builds cleanly, but causes debugging to stop working because of the duplicated singleton.
* The only reason debugging currently works in the CMake build of Bridgeless is by a happy accident: the shared library `hermesinstancejni` depends on `reactnativejni` through a chain of three other libraries unrelated to debugging, and as a result, can access `reactnativejni`'s copy of `jsinspector` (see graph).

 {F1237835169}

It seems that the safest rule of thumb, given the way React Native is currently structured, is that **singletons should live in their own shared libraries** so no call site can cause them to be duplicated through static linking. (It's reasonable to revisit this guidance if we manage to consolidate React Native into one monolithic shared library, eliminating the footgun at the source.)

Changelog:
[Internal] [Changed] - Change jsinspector back to a shared library in the CMake build.

Reviewed By: cortinico, NickGerleman

Differential Revision: D52541488

fbshipit-source-id: 502210add0b734a9bbc470bdf38fb70a41e149a9
  • Loading branch information
motiz88 authored and facebook-github-bot committed Jan 4, 2024
1 parent 5aa425c commit 9ba3bc9
Showing 1 changed file with 4 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ add_compile_options(
-std=c++20)

file(GLOB jsinspector_SRC CONFIGURE_DEPENDS *.cpp)
add_library(jsinspector STATIC ${jsinspector_SRC})
# jsinspector contains singletons that hold app-global state (InspectorFlags, InspectorImpl).
# Placing it in a shared library makes the singletons safe to use from arbitrary shared libraries
# (even ones that don't depend on one another).
add_library(jsinspector SHARED ${jsinspector_SRC})

target_include_directories(jsinspector PUBLIC ${REACT_COMMON_DIR})

Expand Down

0 comments on commit 9ba3bc9

Please sign in to comment.