Test on Android using "release" configuration #6769

kraenhansen · 2024-06-28T09:05:12Z

What, How & Why?

As part of #6737 I've experienced the Android test app crashing with out of memory exceptions (most likely caused by microsoft/react-native-test-app#2109). As per conversations with the other members of our SDK team, it seems valuable for use to bundle and run our Android app using a "release" configuration, which I suspect will also serve as a workaround for the out-of-memory issue.

kraenhansen · 2024-06-28T09:07:32Z

.github/workflows/pr-realm-js.yml

+          keytool -genkey -v -keyalg RSA -keysize 2048 -validity 10000 -dname cn=Realm -keystore ${{ env.STORE_PATH }} -storepass ${{ env.STORE_PASS }} -alias ${{ env.KEY_ALIAS }} -keypass ${{ env.KEY_PASS }}
+          jq --arg storeFile ${{ env.STORE_PATH }} --arg storePassword ${{ env.STORE_PASS }} --arg keyAlias ${{ env.KEY_ALIAS }} --arg keyPassword ${{ env.KEY_PASS }} '. +{android: {signingConfigs: { release: { $storeFile, $storePassword, $keyAlias, $keyPassword } }}}' app.json > patched-app.json
+          mv patched-app.json app.json


This generates a temporary keystore and key to sign the release APK and patch the RNTA app.json to pick it up when building the app.

kraenhansen · 2024-06-28T09:09:12Z

.github/workflows/pr-realm-js.yml

-    env:
-      SPAWN_LOGCAT: true
-    runs-on: macos-latest-large
+    runs-on: ubuntu-latest


I'm also switching to ubuntu, as that allows hardware acceleration and is supposedly faster (see #6444 and realm/realm-dart@2d93a4f3 for context).

Scrap that. This results in a clang++ crash (likely caused by the runner lacking disk space)

clang++: error: unable to execute command: Bus error (core dumped)
clang++: error: linker command failed due to signal (use -v to see invocation)
ninja: build stopped: subcommand failed.

kraenhansen · 2024-06-28T14:38:39Z

This is currently crashing:

Sync Errors
06-28 10:19:24.844 3482 3513 E libc++abi: terminating due to uncaught exception of type facebook::jsi::JSError: expected 'Person' to equal 't'
06-28 10:19:24.844 3482 3513 E libc++abi:
06-28 10:19:24.844 3482 3513 E libc++abi: AssertionError: expected 'Person' to equal 't'
06-28 10:19:24.844 3482 3513 E libc++abi: at proxy trap (native)
06-28 10:19:24.844 3482 3513 E libc++abi: at onError (main.android.jsbundle:1345:9930)
06-28 10:19:24.844 3482 3513 E libc++abi: at anonymous (main.android.jsbundle:635:800)

I believe we've seen this in the past when the C++ stdlib ABI mismatch :thinking_face: Basically the block of native code that catch the JSError and "translates" in into an Error object for the JS runtime is unable to catch the exception. I suspect this has to do with the arguments provide to cmake when we build the prebuild or the binding itself:

realm-js/packages/realm/src/scripts/build/android.ts

Lines 94 to 130 in f8aefa2

    
           "-G", 
        
           "Ninja", 
        
           "-S", 
        
           REALM_CORE_PATH, 
        
           "-B", 
        
           buildPath, 
        
           "--toolchain", 
        
           toolchainPath, 
        
           "-D", 
        
           "CMAKE_SYSTEM_NAME=Android", 
        
           "-D", 
        
           `CPACK_SYSTEM_NAME=Android-${architecture}`, 
        
           "-D", 
        
           `CMAKE_INSTALL_PREFIX=${installPath}`, 
        
           "-D", 
        
           `CMAKE_BUILD_TYPE=${configuration}`, 
        
           "-D", 
        
           "CMAKE_MAKE_PROGRAM=ninja", 
        
           "-D", 
        
           "CMAKE_C_COMPILER_LAUNCHER=ccache", 
        
           "-D", 
        
           "CMAKE_CXX_COMPILER_LAUNCHER=ccache", 
        
           "-D", 
        
           `ANDROID_NDK=${ndkPath}`, 
        
           "-D", 
        
           `ANDROID_ABI=${architecture}`, 
        
           "-D", 
        
           "ANDROID_TOOLCHAIN=clang", 
        
           "-D", 
        
           `ANDROID_NATIVE_API_LEVEL=${ANDROID_API_LEVEL}`, 
        
           "-D", 
        
           "ANDROID_STL=c++_shared", 
        
           // Realm specific variables below 
        
           "-D", 
        
           `REALM_VERSION=${REALM_CORE_VERSION}`, 
        
           "-D", 
        
           "REALM_BUILD_LIB_ONLY=ON",

realm-js/packages/realm/binding/android/build.gradle

Lines 58 to 60 in f8aefa2

    
           arguments "-DANDROID_STL=c++_shared", 
        
                     "-DCMAKE_CXX_VISIBILITY_PRESET=hidden", 
        
                     "-DREACT_NATIVE_ROOT_DIR=${REACT_NATIVE_ROOT_DIR}"

We might need to pass -frtti or -fexceptions as it's done here.

kraenhansen · 2024-06-28T15:01:40Z

I ran it with -frtti and -fexpections but got the same crash.

Another theory of mine:

We're failing an expect that normally passes, possibly because this PR produce a minified bundle that might change names (in this case the "Person" class might now be called "t").

The assertion is written in a callback that we're not wrapping like we do it for the object listeners:

realm-js/packages/realm/src/ObjectListeners.ts

Lines 60 to 71 in f8aefa2

    
           try { 
        
             callback(this.object as RealmObject<T> & T, { 
        
               deleted: changes.isDeleted, 
        
               changedProperties: changes.changedColumns.map(this.properties.getName), 
        
             }); 
        
           } catch (err) { 
        
             // Scheduling a throw on the event loop, 
        
             // since throwing synchronously here would result in an abort in the calling C++ 
        
             setImmediate(() => { 
        
               throw err; 
        
             }); 
        
           }

☝️ this explicitly mentions the app aborting if we don't wrap the callback in a try catch and rethrow the error on the event-loop. It's most likely related to the callback passed through sync.onError here: https://github.com/realm/realm-js/blob/main/integration-tests/tests/src/tests/sync/flexible.ts#L488

kraenhansen · 2024-07-02T06:04:06Z

.github/workflows/pr-realm-js.yml

+          # Enabling new architecture and bridgeless
+          ORG_GRADLE_PROJECT_newArchEnabled: true
+          ORG_GRADLE_PROJECT_bridgelessEnabled: true


Our native module supports this only on Android at the moment, while #6737 will bring this to iOS too (and refactor the Android implementation too).

kraenhansen · 2024-07-02T06:05:07Z

.github/workflows/pr-realm-js.yml

+            # Setup port forwarding to Mocha Remote
+            adb reverse tcp:8090 tcp:8090
+            # Uninstall the app if already in the snapshot (unlikely but could result in a signature mismatch failure)
+            adb uninstall com.microsoft.reacttestapp || true


Uninstalling here should fix the issues we're having with the Android test app being unable to install on other branches: https://github.com/realm/realm-js/actions/runs/9680928508/job/26757061658?pr=6743#step:16:526

Unable to install /Users/runner/work/realm-js/realm-js/integration-tests/environments/react-native-test-app/android/app/build/outputs/apk/debug/app-debug.apk
[runner] com.android.ddmlib.InstallException: INSTALL_FAILED_UPDATE_INCOMPATIBLE: Package com.microsoft.reacttestapp signatures do not match previously installed version; ignoring!

kraenhansen · 2024-07-02T06:06:00Z

integration-tests/environments/react-native-test-app/package.json

@@ -130,7 +131,7 @@
  },
  "dependencies": {
    "base-64": "^1.0.0",
-    "mocha-remote-react-native": "^1.12.2",
+    "mocha-remote-react-native": "^1.12.3",


This new version avoids transforming errors when the bundle isn't shipped from Metro.

kraenhansen · 2024-07-02T06:07:19Z

.github/workflows/pr-realm-js.yml

+        working-directory: integration-tests/environments/react-native-test-app
+        run: |
+          mkdir dist
+          npx react-native bundle --entry-file index.js --platform android --dev false --minify false --bundle-output dist/main.android.jsbundle --assets-dest dist/res


We use --minify false to avoid test failures from error messages mismatching due to class names being renamed in the bundle.

integration-tests/tests/src/tests/sync/flexible.ts

Co-authored-by: LJ <[email protected]>

kneth

My comment is not blocking the merge

kneth · 2024-07-03T08:45:52Z

packages/realm/binding/android/build.gradle

@@ -59,7 +59,7 @@ android {
                          "-DCMAKE_CXX_VISIBILITY_PRESET=hidden",
                          "-DREACT_NATIVE_ROOT_DIR=${REACT_NATIVE_ROOT_DIR}"
                targets 'realm-js-android-binding'
-                cppFlags '-std=c++20'
+                cppFlags '-O2 -std=c++20 -frtti -fexceptions -Wall -fstack-protector-all'


Should we use -Os or -Oz instead?

It doesn't seem to make too much of a difference:

-O2 -std=c++20 -frtti -fexceptions -Wall -fstack-protector-all

❯ du -h ../../../packages/realm/binding/android/src/main/jniLibs/*/librealm.so 12M ../../../packages/realm/binding/android/src/main/jniLibs/arm64-v8a/librealm.so 6.9M ../../../packages/realm/binding/android/src/main/jniLibs/armeabi-v7a/librealm.so 12M ../../../packages/realm/binding/android/src/main/jniLibs/x86/librealm.so 13M ../../../packages/realm/binding/android/src/main/jniLibs/x86_64/librealm.so

-Os -std=c++20 -frtti -fexceptions -Wall

❯ du -h ../../../packages/realm/binding/android/src/main/jniLibs/*/librealm.so 12M ../../../packages/realm/binding/android/src/main/jniLibs/arm64-v8a/librealm.so 6.8M ../../../packages/realm/binding/android/src/main/jniLibs/armeabi-v7a/librealm.so 12M ../../../packages/realm/binding/android/src/main/jniLibs/x86/librealm.so 13M ../../../packages/realm/binding/android/src/main/jniLibs/x86_64/librealm.so

-Oz -std=c++20 -frtti -fexceptions -Wall

❯ du -h ../../../packages/realm/binding/android/src/main/jniLibs/*/librealm.so 12M ../../../packages/realm/binding/android/src/main/jniLibs/arm64-v8a/librealm.so 6.8M ../../../packages/realm/binding/android/src/main/jniLibs/armeabi-v7a/librealm.so 12M ../../../packages/realm/binding/android/src/main/jniLibs/x86/librealm.so 13M ../../../packages/realm/binding/android/src/main/jniLibs/x86_64/librealm.so

We could probably leave out the stack-protector-all flag, as it does come with a runtime cost and we're taking user input data through JSI values which I think does provide some protection to these type of issues. I'd rather that we opt on the safe side and do some investigations to see if we could reasonably do without it, if we run a project to squeeze out some performance later on.

kraenhansen added the T-Internal label Jun 28, 2024

kraenhansen self-assigned this Jun 28, 2024

cla-bot bot added the cla: yes label Jun 28, 2024

kraenhansen commented Jun 28, 2024

View reviewed changes

kraenhansen added no-changelog no-jira-ticket Skip checking the PR title for Jira reference labels Jun 28, 2024

kraenhansen added 5 commits June 28, 2024 11:29

Fixing react-native dependency

98a7f65

Upgrading RNTA

e1c63e1

Adding an NPM script to attach logcat

3f90b25

Bundling and running Android app in release mode in CI

e450978

Fixed the package lock

98483ae

kraenhansen force-pushed the kh/test-with-android-release branch from 0c5b8fc to 98483ae Compare June 28, 2024 09:32

Uninstalling Android app

ef7320b

Update build.gradle to pass additional cpp flags

4b0e69a

kraenhansen mentioned this pull request Jun 28, 2024

Test on iOS using "release" configuration #6771

Draft

kraenhansen added 3 commits June 28, 2024 21:15

Using strings instead of class-names which might change when minified

897f29c

Upgrade mocha-remote to fix error reporting in release build

31a565a

Disabling minification to stabilize error messages

ed77194

kraenhansen commented Jul 2, 2024

View reviewed changes

kraenhansen requested review from elle-j, kneth and gagik July 2, 2024 06:07

elle-j approved these changes Jul 3, 2024

View reviewed changes

Apply suggestions from code review

382261d

Co-authored-by: LJ <[email protected]>

kneth approved these changes Jul 3, 2024

View reviewed changes

kraenhansen merged commit 7b6a79b into main Jul 5, 2024
35 checks passed

kraenhansen deleted the kh/test-with-android-release branch July 5, 2024 05:46

kraenhansen mentioned this pull request Jul 5, 2024

RJS-2636: Add progress notifications and tests #6743

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test on Android using "release" configuration #6769

Test on Android using "release" configuration #6769

kraenhansen commented Jun 28, 2024

kraenhansen Jun 28, 2024

kraenhansen Jun 28, 2024

kraenhansen Jun 28, 2024 •

edited

Loading

kraenhansen commented Jun 28, 2024

kraenhansen commented Jun 28, 2024 •

edited

Loading

kraenhansen Jul 2, 2024

kraenhansen Jul 2, 2024

kraenhansen Jul 2, 2024 •

edited

Loading

kraenhansen Jul 2, 2024

kneth left a comment

kneth Jul 3, 2024

kraenhansen Jul 5, 2024

kraenhansen Jul 5, 2024

Test on Android using "release" configuration #6769

Test on Android using "release" configuration #6769

Conversation

kraenhansen commented Jun 28, 2024

What, How & Why?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

kraenhansen commented Jun 28, 2024

kraenhansen commented Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kneth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

-O2 -std=c++20 -frtti -fexceptions -Wall -fstack-protector-all

-Os -std=c++20 -frtti -fexceptions -Wall

-Oz -std=c++20 -frtti -fexceptions -Wall

Choose a reason for hiding this comment

kraenhansen Jun 28, 2024 •

edited

Loading

kraenhansen commented Jun 28, 2024 •

edited

Loading

kraenhansen Jul 2, 2024 •

edited

Loading

`-O2 -std=c++20 -frtti -fexceptions -Wall -fstack-protector-all`

`-Os -std=c++20 -frtti -fexceptions -Wall`

`-Oz -std=c++20 -frtti -fexceptions -Wall`