Skip to content

Upgrade to WebKitGTK 2.36.3 - #169

Merged
Kudo merged 3 commits into
mainfrom
webkit-gtk-2.36.3
Jul 12, 2022
Merged

Upgrade to WebKitGTK 2.36.3#169
Kudo merged 3 commits into
mainfrom
webkit-gtk-2.36.3

Conversation

@Kudo

@KudoKudo commented Jul 10, 2022

Copy link
Copy Markdown
Member

Why

upgrade to latest WebKitGTK 2.36.3 (svn r294992). mostly from community requests and hopefully to fix the crashes on samsung android 12 devices.

How

  • upgrade NDK to 23.2.8568313. we need newer versions because the newer webkit needs std::filesystem that doesn't support on NDK r21. also, because the NDK r23 libc++_shared.so has std::filesystem symbols that older version doesn't have. to prevent developers use pickFirst to pick wrong libc++_shared.so, i turned to use link static c++ runtime instead.
  • [BREAKING CHANGE] remove non Intl variant, because newer JSC doesn't have a build flag to turn it off.
  • [BREAKING CHANGE] Bump minimum supported Android SDK to 21
  • remove outdated patches for older jsc or ndk
  • remove some platform specific cflags, let ndk to do it.

size changes:

-r250230 Intlr294992 Intl
armeabi-v7a14 MB17 MB
arm64-v8a18 MB19 MB
x8617 MB20 MB
x86_6419 MB21 MB

Test Plan

  • ci passed.
  • launch test on arm64 devices

- Keeps only Intl variant
- Upgrade NDK to r23
- Set minimal Android SDK to 21
- Remove unused patch
- Update CI
@Kudo
Kudoforce-pushed the webkit-gtk-2.36.3 branch from c21c5b2 to 81ff9a8CompareJuly 11, 2022 06:34
@KudoKudo changed the title WebKitGTK 2.36.3Upgrade to WebKitGTK 2.36.3Jul 11, 2022
@Kudo
Kudo marked this pull request as ready for review July 11, 2022 12:23
@Kudo
Kudo requested a review from wkozyra95July 11, 2022 15:18
"cmake;3.18.1" \
"ndk;23.2.8568313"
# move out builtin icu headers from ndk and prevent icu build errors
mv "${ANDROID_HOME}/ndk/23.2.8568313/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/unicode" "${ANDROID_HOME}/ndk/23.2.8568313/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/unicode2"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this necessary, can you clarify what was the error? Does it mean that building jsc will not work locally? And if it works locally, then it should be possible to make it work on ci without this patch.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from NDK r22, the icu headers are builtin inside NDK. when building icu, the includes such as #include "unicode/utypes.h" will access to NDK headers first and lead to build errors.

i also need to move out the unicode/ folder from NDK when building locally.

-latomic \
-lm \
-lc++_shared \
-static-libstdc++ \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why static?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upgrade NDK to 23.2.8568313. we need newer versions because the newer webkit needs std::filesystem that doesn't support on NDK r21. also, because the NDK r23 libc++_shared.so has std::filesystem symbols that older version doesn't have. to prevent developers use pickFirst to pick wrong libc++_shared.so, i turned to use link static c++ runtime instead.

left it on pr description. feel free to let me know if that's not clear.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not sound like a safe change. e.g. if we have jsc with statically linked stl and e.g. expo-gl or react-native build with shared stl then we are including stl twice and potentially different versions of it

https://developer.android.com/ndk/guides/cpp-support#sr

In this situation, the STL, including and global data and static constructors, will be present in both libraries. The runtime behavior of this application is undefined, and in practice crashes are very common. Other possible issues include:

Memory allocated in one library, and freed in the other, causing memory leakage or heap corruption.
Exceptions raised in libfoo.so going uncaught in libbar.so, causing your app to crash.
Buffering of std::cout not working properly.

I just wanted to mention my concern on that, I have only a minimal idea of how all this works, so if you think it's a safe change I'm fine with it

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting! i would say thankfully the libjsc.so is really independency. the exceptions, memory allocations are not across other libraries. the JSCRuntime.cpp implementation in react-native only relies on C interfaces, so it might be safer in this case.

otherwise, TBH i don't have a better solution toward the inconsistent libc++_shared.so issues.

@Kudo
Kudo merged commit 6b1ec06 into mainJul 12, 2022
@Kudo
Kudo deleted the webkit-gtk-2.36.3 branch July 12, 2022 17:01
@KudoKudo mentioned this pull request Jul 13, 2022
@aweffr

aweffr commented Aug 20, 2022

Copy link
Copy Markdown

I have test jsc-android@294992.0.0 on react native 0.69.4 but failed to start

the error log is:

2022-08-20 18:17:31.624 30482-30482/com.dongdongapp D/SoLoader: libjscexecutor.so not found on /data/data/com.dongdongapp/lib-main
2022-08-20 18:17:31.624 30482-30482/com.dongdongapp D/SoLoader: libjscexecutor.so found on /data/app/com.dongdongapp-Mweej1BEjjpm6zwMqcyWbQ==/lib/arm64
2022-08-20 18:17:31.624 30482-30482/com.dongdongapp D/SoLoader: Not resolving dependencies for libjscexecutor.so
2022-08-20 18:17:31.630 30482-30482/com.dongdongapp W/System.err: java.lang.UnsatisfiedLinkError: dlopen failed: cannot locate symbol "_Unwind_Resume" referenced by "/data/app/com.dongdongapp-Mweej1BEjjpm6zwMqcyWbQ==/lib/arm64/libjscexecutor.so"...
2022-08-20 18:17:31.630 30482-30482/com.dongdongapp W/System.err: at java.lang.Runtime.load0(Runtime.java:938)
2022-08-20 18:17:31.630 30482-30482/com.dongdongapp W/System.err: at java.lang.System.load(System.java:1632)

if i use 250230 as default, it works, the log is:

2022-08-20 18:23:26.739 31472-31472/com.dongdongapp D/SoLoader: libjscexecutor.so not found on /data/data/com.dongdongapp/lib-main
2022-08-20 18:23:26.739 31472-31472/com.dongdongapp D/SoLoader: libjscexecutor.so found on /data/app/com.dongdongapp-efMygP2ImvdqkCDQPP5tdQ==/lib/arm64
2022-08-20 18:23:26.739 31472-31472/com.dongdongapp D/SoLoader: Not resolving dependencies for libjscexecutor.so
2022-08-20 18:23:26.776 31472-31472/com.dongdongapp D/JavaScriptCore.Version: 250230.2.1
2022-08-20 18:23:26.889 31472-31472/com.dongdongapp D/NetworkSecurityConfig: No Network Security Config specified, using platform default

@Kudo

Kudo commented Aug 21, 2022

Copy link
Copy Markdown
MemberAuthor

hi @aweffr, do you mind to create a github repository of minimal reproducible example for me? i tested jsc-android@294992.0.0 + react-native@0.69.4 on android 12 emulator and it works for me.

@khoanguyen-yang

khoanguyen-yang commented Nov 30, 2022

Copy link
Copy Markdown

Hi @Kudo,

Is there any way that I can use non Intl for this JSC version? Enabling Intl significantly increases the app bundle size though

@Kudo

Kudo commented Dec 6, 2022

Copy link
Copy Markdown
MemberAuthor

@khoanguyen-yang unfortunately no. as the pr described the new jsc has no flag to turn off Intl now.

@cortinico

cortinico commented Dec 6, 2022

Copy link
Copy Markdown
Member

@khoanguyen-yang unfortunately no. as the pr described the new jsc has no flag to turn off Intl now.

@Kudo do we need to update the RN template and remove the non-intl flavor there?

@Kudo

Kudo commented Dec 6, 2022

Copy link
Copy Markdown
MemberAuthor

@cortinico as react/react-native#35504 (comment) mentioned, we don't have to update RN template in the mean time.

Kudo added a commit that referenced this pull request Feb 3, 2023
Kudo added a commit that referenced this pull request Feb 4, 2023
Kudo added a commit that referenced this pull request Feb 4, 2023
Kudo added a commit that referenced this pull request Feb 4, 2023
Kudo added a commit that referenced this pull request Feb 4, 2023
Kudo added a commit that referenced this pull request Feb 4, 2023
Kudo added a commit that referenced this pull request Feb 5, 2023
# Why
fixes#178
# How
- manually pick necessary changes for ndk r23 from #169. it's ndk r23c which is newer than react-native 0.71's r23b. i cannot build jsc on top on r23b successfully. hopefully r23b <-> r23c is good for abi safety.
- fix build errors for webkitgtk 2.26.1 + ndk r23.
- unlike #169, this pr is still using shared c++ runtime.
- update test project to 0.71 and build from source code because we need a patch to link libunwind in libjscexecutor.so.
# Test Plan
ci passed
@Hesowcharov

Copy link
Copy Markdown

I have test jsc-android@294992.0.0 on react native 0.69.4 but failed to start

the error log is:

2022-08-20 18:17:31.624 30482-30482/com.dongdongapp D/SoLoader: libjscexecutor.so not found on /data/data/com.dongdongapp/lib-main
2022-08-20 18:17:31.624 30482-30482/com.dongdongapp D/SoLoader: libjscexecutor.so found on /data/app/com.dongdongapp-Mweej1BEjjpm6zwMqcyWbQ==/lib/arm64
2022-08-20 18:17:31.624 30482-30482/com.dongdongapp D/SoLoader: Not resolving dependencies for libjscexecutor.so
2022-08-20 18:17:31.630 30482-30482/com.dongdongapp W/System.err: java.lang.UnsatisfiedLinkError: dlopen failed: cannot locate symbol "_Unwind_Resume" referenced by "/data/app/com.dongdongapp-Mweej1BEjjpm6zwMqcyWbQ==/lib/arm64/libjscexecutor.so"...
2022-08-20 18:17:31.630 30482-30482/com.dongdongapp W/System.err: at java.lang.Runtime.load0(Runtime.java:938)
2022-08-20 18:17:31.630 30482-30482/com.dongdongapp W/System.err: at java.lang.System.load(System.java:1632)

if i use 250230 as default, it works, the log is:

2022-08-20 18:23:26.739 31472-31472/com.dongdongapp D/SoLoader: libjscexecutor.so not found on /data/data/com.dongdongapp/lib-main
2022-08-20 18:23:26.739 31472-31472/com.dongdongapp D/SoLoader: libjscexecutor.so found on /data/app/com.dongdongapp-efMygP2ImvdqkCDQPP5tdQ==/lib/arm64
2022-08-20 18:23:26.739 31472-31472/com.dongdongapp D/SoLoader: Not resolving dependencies for libjscexecutor.so
2022-08-20 18:23:26.776 31472-31472/com.dongdongapp D/JavaScriptCore.Version: 250230.2.1
2022-08-20 18:23:26.889 31472-31472/com.dongdongapp D/NetworkSecurityConfig: No Network Security Config specified, using platform default

The same issue. Did you find the solution ?

@Kudo

Kudo commented Jan 10, 2024

Copy link
Copy Markdown
MemberAuthor

@Hesowcharov jsc-android@294992.0.0 is not compatible with older react-native since android ndk is incompatible.

@Hesowcharov

Copy link
Copy Markdown

@Kudo Thank you for your reply! So, after rn 0.69 (i used 0.69.12) it's recommended to use the default jsc (from facebook) ? I'm just wondering that it should be working since the example app (in this repo) works with rn 0.69 🤔

@Hesowcharov

Copy link
Copy Markdown

for someone who will try to migrate to RN 0.69-0.70, move straight to 0.71.x (my version is 0.71.15) => there jsc-android@294992.0.0 works!

 JavaScriptCore.Version myapp D 294992.0.0
SoLoader myapp D libjscexecutor.so found on /data/app/myapp-blah-blah/base.apk!/lib/arm64-v8a

Kudo added a commit to Kudo/jsc-android-buildscripts that referenced this pull request Nov 16, 2024
…mmunity#179)
# Why
fixesreact-native-community#178
# How
- manually pick necessary changes for ndk r23 from react-native-community#169. it's ndk r23c which is newer than react-native 0.71's r23b. i cannot build jsc on top on r23b successfully. hopefully r23b <-> r23c is good for abi safety.
- fix build errors for webkitgtk 2.26.1 + ndk r23.
- unlike react-native-community#169, this pr is still using shared c++ runtime.
- update test project to 0.71 and build from source code because we need a patch to link libunwind in libjscexecutor.so.
# Test Plan
ci passed
Kudo added a commit that referenced this pull request Nov 16, 2024
# Why
fixes#178
# How
- manually pick necessary changes for ndk r23 from #169. it's ndk r23c which is newer than react-native 0.71's r23b. i cannot build jsc on top on r23b successfully. hopefully r23b <-> r23c is good for abi safety.
- fix build errors for webkitgtk 2.26.1 + ndk r23.
- unlike #169, this pr is still using shared c++ runtime.
- update test project to 0.71 and build from source code because we need a patch to link libunwind in libjscexecutor.so.
# Test Plan
ci passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@Kudo@aweffr@khoanguyen-yang@cortinico@Hesowcharov@wkozyra95