Fix Direct Debugging with JSC

[Saadnajmi]

Summary:

Originally with 5cf8f43 , 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 3eeee11 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, #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 fb30fca).

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: eb1b42f
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 fb30fca had removed

[github-actions]

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]

Suggested change

	#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 130300
	#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 13300

If MacOS follows the same semantics of iOS

[Saadnajmi]

In AvailabilityVersions.h:

#define __IPHONE_16_4                                   160400
#define __IPHONE_16_5                                   160500
#define __IPHONE_16_6                                   160600
#define __IPHONE_16_7                                   160700
#define __IPHONE_17_0                                   170000
#define __IPHONE_17_1                                   170100
#define __IPHONE_17_2                                   170200
#define __IPHONE_17_3                                   170300
#define __IPHONE_17_4                                   170400

[cipolleschi]

uhm... so, why the check #if __IPHONE_OS_VERSION_MAX_ALLOWED >= 160400 failed on devices < 16.4? 😱

[Saadnajmi]

I'll do some testing with an iOS 15 simulator and try to update. Luckily Xcode has a way to see post macro processed files (though I forget the command at this time)

[Saadnajmi]

I did some testing, and as far as I can tell. the existing code is right.
With local testing, I added two printf statements to the offending block and tested on an iOS 15.5 and iOS 17.2 Simulator + RNTester with USE_HERMES=0 pod install:

//#if (__OSX_AVAILABLE_STARTING(MAC_13_3, IPHONE_16_4))
  printf("Inside JSC_HAS_INSPECTABLE hit");
  if (__builtin_available(macOS 13.3, iOS 16.4, tvOS 16.4, *)) {
    printf("Inside __builtin_available hit");
    JSGlobalContextSetInspectable(ctx_, true);
  }
//#endif
#endif

With the outer __OSX_AVAILABLE_STARTING macro commented out, iOS 17.2 will hit both printf statements, while iOS 15.5 will only hit the outer statement (expected). However, with the extra __OSX_AVAILABLE_STARTING commented in, we hit neither printf statement on both iOS 15.5 and iOS 17.2! I can confirm this breaks JSC direct debugging on iOS 16.4+ as well 😨 .

Looking even deeper at Availability.h, I suspect the original crash repoprted the apple forum had their app build setting misconfigured. I see this in comments around the __OSX_AVAILABLE_STARTING macro:

For these macros to function properly, a program must specify the OS version range
it is targeting. The min OS version is specified as an option to the compiler:
-mmacosx-version-min=10.x when building for Mac OS X, and -miphoneos-version-min=y.z
when building for the iPhone. The upper bound for the OS version is rarely needed,
but it can be set on the command line via: -D__MAC_OS_X_VERSION_MAX_ALLOWED=10x0 for
Mac OS X and __IPHONE_OS_VERSION_MAX_ALLOWED = y0z00 for iOS.

[Saadnajmi]

Really? There was an internal app using JSC? I guess you could maybe compile and link this file without using JSC.. Now I don't know 😵‍💫

[cipolleschi]

yes, unfortunately some teams are still on JSC... 🤷

[Saadnajmi]

I think that just leaves the current state being that Direct Debugging is broken on JSC for iOS 16.4+, and fix crashes iOS 16.4-...
I will need to investigate more it seems, but short term priority for be right now is React Native macOS 0.74 sooo no timeline guarantees :/

[Saadnajmi]

(Unless of course my testing was wrong and the patch for the new macro is actually fine.. maybe I'll try outside of RNTester

[Saadnajmi]

Update:

• The crash only happens on device, hence we didn't catch it in simulator (Thankfully I kept my old iPad)
• System frameworks are dynamically linked, so the symbol lookup happens at runtime. That makes sense why it's a crash and not a failure to compile.
• It can be fixed by linking JavascriptCore as a weak_framework instead of a framework. Doing so means it will explicitly link all the sumbols in the framework weakly, and seems to be the intended way to deal with new APIs. We don't use weak frameworks internally much, so it's a bit of a blind spot to me. https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html

I updated this PR so that we weakly link JavascriptCore. I also totally changed the title and description because the scope is now very different :D

[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.

[react-native-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.

[react-native-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.

[liamjones]

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.

Hi @cipolleschi, are the team in a position where this could be landed now? (Asking because the stale bot just kicked in 😄)

[cipolleschi]

@liamjones is there a reason why you are still using JSC instead of Hermes? With Hermes, you can use the React Native DevTools that are much more powerful than the JSC direct debugging.

[liamjones]

@cipolleschi It's down to dependency/upgrade complexity + resource constraint issues here.

I'd love to get to Hermes (and the new architecture) but, alongside feature development & bug fixing of our own code, we struggle to even keep up to date with RN upgrades currently due to their frequency (I'm trying to upgrade from 0.73 to 0.74+ atm).

I have tried to look at the Hermes switchover in the past but;

• Originally we couldn't switch to Hermes because of Realm's low-level binding to JSC and it didn't yet have Hermes bindings.
• The next time I looked I think we had other modules that didn't work on Hermes.
• Next, we were blocked by the lack of Proxy & support/performance issues with Intl API in Hermes.
• The last time I had a chance to look at it, as soon as I tried enabling Hermes the apps crashed on launch and I didn't have time to investigate in depth (our app is big; over 300k LOC TypeScript, including tests, and our team is small).

[facebook-github-bot]

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

[facebook-github-bot]

@cipolleschi merged this pull request in b04d17a.

[react-native-bot]

This pull request was successfully merged by @Saadnajmi in b04d17a

^{When will my fix make it into a release? | How to file a pick request?}

[Saadnajmi]

Heh. Didn't think this would land. Thanks!