Fix Direct Debugging with JSC

[Saadnajmi]

Summary:

Originally with https://github.com/facebook/react-native/commit/5cf8f43ab182781ea82e88077df425c3efbfc21f , we added a call to a new Apple API JSGlobalContextSetInspectable to ensure that our Javascript running with JSC is debuggable. That change was guarded with a __builtin_available(macOS 13.3, iOS 16.4, tvOS 16.4, *) check to make sure it only ran on OS'es where to function existed. Later, in https://github.com/facebook/react-native/commit/3eeee11d7ac4075d0917233d3be4a9469f802d35 we did an extra guard in the way of a macro to check we were compiling against a new enough version of Xcode (so that Xcode knows about the symbol).

Between the runtime check and the compile time check, we should be good right? Wrong! As it turns out, this bit of code still caused crashes on iOS 15 devices (See this Apple Forum Thread). To address this, https://github.com/facebook/react-native/pull/44185 was added which added a new compiler guard (__OSX_AVAILABLE_STARTING(MAC_NA, IPHONE_16_4) was added. Unfortunately, this guard is incorrect: It is basically checking if our minimum iOS deployment target is 16.4 (It's not, as of writing it is iOS 13.4), which effectively means this code is never compiled and one can never direct debug with JSC on iOS 16.4+ 😨!

So what went wrong, and why were the first two guards not good enough? Three main reasons..

Firstly, this is a device only crash, and not reproducible on simulator. This is probably why the crash was not caught earlier. Secondly, It's because system frameworks (like JavascriptCore) are dynamically linked: the linker doesn't look for the symbol till runtime (and crashes when doing so). Thirdly, It's because we are strongly linking the framework, so every symbol must be present and the macros / guard Apple provides with AvailabilityMacros.h don't work.

What we want to do is link JavascriptCore as a weak_framework, more info here: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html

From that link:

One challenge faced by developers is that of taking advantage of new features introduced in new versions of OS X while still supporting older versions of the system. Normally, if an application uses a new feature in a framework, it is unable to run on earlier versions of the framework that do not support that feature. Such applications would either fail to launch or crash when an attempt to use the feature was made. Apple has solved this problem by adding support for weakly-linked symbols.

When a symbol in a framework is defined as weakly linked, the symbol does not have to be present at runtime for a process to continue running. The static linker identifies a weakly linked symbol as such in any code module that references the symbol. The dynamic linker uses this same information at runtime to determine whether a process can continue running. If a weakly linked symbol is not present in the framework, the code module can continue to run as long as it does not reference the symbol. However, if the symbol is present, the code can use it normally.

This seems to be exactly what we want, and the Apple provided method for using new APIs in system frameworks!

Let's update our podspecs so we link JavascriptCore weakly. As a bonus (and admittedly, the original purpose of this PR) let's add macOS support to the JSC_HAS_INSPECTABLE macro (This file JSCRuntime.cpp used to have more explicit macOS support in it's macros, but I had removed it with https://github.com/facebook/react-native/commit/fb30fcaa2f526cc1f7c2d4189ec9c57f9cf9b3c5).

Changelog:

[IOS] [FIXED] - Fix Direct Debugging with JSC

Test Plan:

Tested that RNTester doesn't crash on boot running on an iPad Air 2 running iOS 15.8., and that an iOS 17.2 simulator is debuggable.

Built RN-Tester and RN-Tester-macOS and verified both show up in Safari Web Inspectors' debug menu:

macOS screenshot small bc I got some internal stuff I gotta crop 😅

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	19,411,012	-116,773
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	22,784,339	-116,008
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: eb1b42fa8b2834bebb9eb5b31201e6c20ae575fe Branch: main

[facebook-github-bot]

@ryancat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[Saadnajmi]

Any update for this one?

[Saadnajmi]

Ping on this PR again, any chance someone can take a look? This should be non, risky, and inline with the previous ifdef structure that https://github.com/facebook/react-native/commit/fb30fcaa2f526cc1f7c2d4189ec9c57f9cf9b3c5 had removed

[github-actions[bot]]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[Saadnajmi]

@cipolleschi any chance this could be looked at again?

[cipolleschi]

Is it a problem if we keep this PR hovering for a bit? We are in the middle of some migrations of internal apps and it will be easier to land this after the migration is done.

[Saadnajmi]

it a problem if we keep this PR hovering for a bit? We a

No problem for me. Internally, we're all clear because we use React Native macOS (and have higher minimum Xcode support, etc). For the community, I guess if nobody else has complained, 🤷🏽‍♂️. I'm also focused more on internal stuff atm.