Skip to content

Commit

Permalink
Fix Direct Debugging with JSC (facebook#39549)
Browse files Browse the repository at this point in the history
Summary:
Originally with facebook@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 facebook@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](https://forums.developer.apple.com/forums/thread/749534)). To address this, facebook#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 facebook@fb30fca).

## Changelog:

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

Pull Request resolved: facebook#39549

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:

<img width="1316" alt="Screenshot 2023-09-19 at 10 48 43 PM" src="https://github.com/facebook/react-native/assets/6722175/c642e6e0-36af-4c9f-845a-7e491489f419">

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

<img width="347" alt="Screenshot 2023-09-19 at 10 53 46 PM" src="https://github.com/facebook/react-native/assets/6722175/1e802c88-02b8-49e1-8fd2-d91726ca1e93">

Reviewed By: huntie

Differential Revision: D67338150

Pulled By: cipolleschi

fbshipit-source-id: 620c3b3cc1e37e54de7fa4dc9956a02c8f3c09f8
  • Loading branch information
Saadnajmi authored and facebook-github-bot committed Dec 18, 2024
1 parent 9f12fce commit b04d17a
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 5 deletions.
2 changes: 1 addition & 1 deletion packages/react-native/React-Core.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ Pod::Spec.new do |s|
s.resource_bundle = { "RCTI18nStrings" => ["React/I18n/strings/*.lproj"]}
s.compiler_flags = folly_compiler_flags + ' ' + boost_compiler_flags + ' ' + use_hermes_flag
s.header_dir = "React"
s.framework = "JavaScriptCore"
s.weak_framework = "JavaScriptCore"
s.pod_target_xcconfig = {
"HEADER_SEARCH_PATHS" => header_search_paths,
"DEFINES_MODULE" => "YES",
Expand Down
3 changes: 2 additions & 1 deletion packages/react-native/React/React-RCTFabric.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ Pod::Spec.new do |s|
s.compiler_flags = folly_compiler_flags + ' ' + boost_compiler_flags + new_arch_flags
s.header_dir = header_dir
s.module_name = module_name
s.framework = ["JavaScriptCore", "MobileCoreServices"]
s.weak_framework = "JavaScriptCore"
s.framework = "MobileCoreServices"
s.pod_target_xcconfig = {
"HEADER_SEARCH_PATHS" => header_search_paths,
"OTHER_CFLAGS" => "$(inherited) " + folly_compiler_flags + new_arch_flags,
Expand Down
2 changes: 0 additions & 2 deletions packages/react-native/ReactCommon/jsc/JSCRuntime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -377,13 +377,11 @@ JSCRuntime::JSCRuntime(JSGlobalContextRef ctx)
{
#ifndef NDEBUG
#ifdef _JSC_HAS_INSPECTABLE
#if (__OSX_AVAILABLE_STARTING(MAC_NA, IPHONE_16_4))
if (__builtin_available(macOS 13.3, iOS 16.4, tvOS 16.4, *)) {
JSGlobalContextSetInspectable(ctx_, true);
}
#endif
#endif
#endif
}

JSCRuntime::~JSCRuntime() {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-native/ReactCommon/jsc/React-jsc.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ Pod::Spec.new do |s|
s.source = source
s.source_files = "JSCRuntime.{cpp,h}"
s.exclude_files = "**/test/*"
s.framework = "JavaScriptCore"
s.weak_framework = "JavaScriptCore"

s.dependency "React-jsi", version

Expand Down

0 comments on commit b04d17a

Please sign in to comment.