diff --git a/android/src/main/java/cn/reactnative/modules/update/StateSerialRunner.java b/android/src/main/java/cn/reactnative/modules/update/StateSerialRunner.java new file mode 100644 index 00000000..f0e12c19 --- /dev/null +++ b/android/src/main/java/cn/reactnative/modules/update/StateSerialRunner.java @@ -0,0 +1,64 @@ +package cn.reactnative.modules.update; + +import android.util.Log; +import androidx.annotation.Nullable; +import com.facebook.react.bridge.Promise; + +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadFactory; + +/** + * Runs state-persistence operations (switchVersion / markSuccess / setUuid / + * setLocalHashInfo) on a dedicated single background thread. + * + * These operations only read/modify SharedPreferences via a synchronous + * commit(); they were previously dispatched to the UI thread purely to + * serialize them. markSuccess in particular runs on every cold start, so doing + * its blocking disk write on the main thread caused jank/ANR on low-end + * devices. A single-thread executor preserves the same serialization guarantee + * while keeping the disk I/O off the UI thread. + * + * Note: reload/restart operations must still run on the UI thread and therefore + * keep using {@link UiThreadRunner}. + */ +final class StateSerialRunner { + interface Operation { + void run() throws Throwable; + } + + // Single worker thread -> operations stay serialized in submission order, + // matching the previous UI-thread behavior. The thread is named so it is + // identifiable in thread dumps / ANR traces when diagnosing persistence. + private static final Executor EXECUTOR = Executors.newSingleThreadExecutor( + new ThreadFactory() { + @Override + public Thread newThread(Runnable r) { + return new Thread(r, "pushy-state-serial"); + } + }); + + private StateSerialRunner() { + } + + static void run( + @Nullable final Promise promise, + final String operationName, + final Operation operation + ) { + EXECUTOR.execute(new Runnable() { + @Override + public void run() { + try { + operation.run(); + } catch (Throwable error) { + if (promise != null) { + promise.reject(operationName + " failed", error); + } else { + Log.e(UpdateContext.TAG, operationName + " failed", error); + } + } + } + }); + } +} diff --git a/android/src/main/java/cn/reactnative/modules/update/UpdateModuleImpl.java b/android/src/main/java/cn/reactnative/modules/update/UpdateModuleImpl.java index 5aab3323..5a3a9b8a 100644 --- a/android/src/main/java/cn/reactnative/modules/update/UpdateModuleImpl.java +++ b/android/src/main/java/cn/reactnative/modules/update/UpdateModuleImpl.java @@ -149,7 +149,7 @@ public static void setNeedUpdate( final Promise promise ) { final String hash = options.getString("hash"); - UiThreadRunner.run(promise, "switchVersionLater", new UiThreadRunner.Operation() { + StateSerialRunner.run(promise, "switchVersionLater", new StateSerialRunner.Operation() { @Override public void run() { setNeedUpdateInternal(updateContext, hash); @@ -160,7 +160,7 @@ public void run() { public static void setNeedUpdate(final UpdateContext updateContext, final ReadableMap options) { final String hash = options.getString("hash"); - UiThreadRunner.run(null, "switchVersionLater", new UiThreadRunner.Operation() { + StateSerialRunner.run(null, "switchVersionLater", new StateSerialRunner.Operation() { @Override public void run() { setNeedUpdateInternal(updateContext, hash); @@ -173,7 +173,7 @@ private static void markSuccessInternal(UpdateContext updateContext) { } public static void markSuccess(final UpdateContext updateContext, final Promise promise) { - UiThreadRunner.run(promise, "markSuccess", new UiThreadRunner.Operation() { + StateSerialRunner.run(promise, "markSuccess", new StateSerialRunner.Operation() { @Override public void run() { markSuccessInternal(updateContext); @@ -183,7 +183,7 @@ public void run() { } public static void markSuccess(final UpdateContext updateContext) { - UiThreadRunner.run(null, "markSuccess", new UiThreadRunner.Operation() { + StateSerialRunner.run(null, "markSuccess", new StateSerialRunner.Operation() { @Override public void run() { markSuccessInternal(updateContext); @@ -200,7 +200,7 @@ public static void setUuid( final String uuid, final Promise promise ) { - UiThreadRunner.run(promise, "setUuid", new UiThreadRunner.Operation() { + StateSerialRunner.run(promise, "setUuid", new StateSerialRunner.Operation() { @Override public void run() { setUuidInternal(updateContext, uuid); @@ -210,7 +210,7 @@ public void run() { } public static void setUuid(final UpdateContext updateContext, final String uuid) { - UiThreadRunner.run(null, "setUuid", new UiThreadRunner.Operation() { + StateSerialRunner.run(null, "setUuid", new StateSerialRunner.Operation() { @Override public void run() { setUuidInternal(updateContext, uuid); @@ -235,7 +235,7 @@ public static void setLocalHashInfo( final String info, final Promise promise ) { - UiThreadRunner.run(promise, "setLocalHashInfo", new UiThreadRunner.Operation() { + StateSerialRunner.run(promise, "setLocalHashInfo", new StateSerialRunner.Operation() { @Override public void run() { setLocalHashInfoInternal(updateContext, hash, info); @@ -249,7 +249,7 @@ public static void setLocalHashInfo( final String hash, final String info ) { - UiThreadRunner.run(null, "setLocalHashInfo", new UiThreadRunner.Operation() { + StateSerialRunner.run(null, "setLocalHashInfo", new StateSerialRunner.Operation() { @Override public void run() { setLocalHashInfoInternal(updateContext, hash, info); diff --git a/cpp/patch_core/tests/patch_core_test.cpp b/cpp/patch_core/tests/patch_core_test.cpp index 26b44fa4..2ed6f228 100644 --- a/cpp/patch_core/tests/patch_core_test.cpp +++ b/cpp/patch_core/tests/patch_core_test.cpp @@ -554,15 +554,22 @@ int main() { {"StateCoreSwitchToSameVersion", TestStateCoreSwitchToSameVersion}, }; + int failures = 0; for (const auto& test : tests) { try { test.second(); std::fprintf(stdout, "[PASS] %s\n", test.first.c_str()); } catch (const std::exception& error) { std::fprintf(stderr, "[FAIL] %s: %s\n", test.first.c_str(), error.what()); - return 1; + ++failures; } } - return 0; + std::fprintf( + stdout, + "\n%zu tests, %d passed, %d failed\n", + tests.size(), + static_cast(tests.size()) - failures, + failures); + return failures == 0 ? 0 : 1; } diff --git a/scripts/test-patch-core.sh b/scripts/test-patch-core.sh index 2f8702b9..bc928ec0 100755 --- a/scripts/test-patch-core.sh +++ b/scripts/test-patch-core.sh @@ -6,6 +6,15 @@ BUILD_DIR="$ROOT_DIR/.tmp/patch-core-tests" mkdir -p "$BUILD_DIR" +# Opt-in AddressSanitizer + UndefinedBehaviorSanitizer build. The patch core +# processes untrusted (downloaded) patch data, so running the tests under +# sanitizers catches memory/UB regressions. Enable with: SANITIZE=1 npm run test:patch-core +SANITIZE_FLAGS="" +if [ "${SANITIZE:-0}" = "1" ]; then + SANITIZE_FLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer -g" + echo "Building patch core tests with AddressSanitizer + UBSan" +fi + COMMON_INCLUDES=" -I$ROOT_DIR/cpp/patch_core -I$ROOT_DIR/android/jni @@ -14,16 +23,17 @@ COMMON_INCLUDES=" -I$ROOT_DIR/android/jni/lzma/C " -cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/hpatch.c" -o "$BUILD_DIR/hpatch.o" -cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/HDiffPatch/libHDiffPatch/HPatch/patch.c" -o "$BUILD_DIR/patch.o" -cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/HDiffPatch/file_for_patch.c" -o "$BUILD_DIR/file_for_patch.o" -cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/lzma/C/LzmaDec.c" -o "$BUILD_DIR/LzmaDec.o" -cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/lzma/C/Lzma2Dec.c" -o "$BUILD_DIR/Lzma2Dec.o" +cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/hpatch.c" -o "$BUILD_DIR/hpatch.o" +cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/HDiffPatch/libHDiffPatch/HPatch/patch.c" -o "$BUILD_DIR/patch.o" +cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/HDiffPatch/file_for_patch.c" -o "$BUILD_DIR/file_for_patch.o" +cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/lzma/C/LzmaDec.c" -o "$BUILD_DIR/LzmaDec.o" +cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/lzma/C/Lzma2Dec.c" -o "$BUILD_DIR/Lzma2Dec.o" c++ \ -std=c++17 \ -Wall \ -Wextra \ + $SANITIZE_FLAGS \ $COMMON_INCLUDES \ "$ROOT_DIR/cpp/patch_core/tests/patch_core_test.cpp" \ "$ROOT_DIR/cpp/patch_core/archive_patch_core.cpp" \ diff --git a/src/__tests__/client.test.ts b/src/__tests__/client.test.ts index 215b37e0..4edd952e 100644 --- a/src/__tests__/client.test.ts +++ b/src/__tests__/client.test.ts @@ -2,6 +2,13 @@ import { afterEach, describe, expect, mock, test } from 'bun:test'; const importFreshClient = (cacheKey: string) => import(`../client?${cacheKey}`); +const originalDev = (globalThis as any).__DEV__; + +afterEach(() => { + mock.restore(); + (globalThis as any).__DEV__ = originalDev; +}); + const createJsonResponse = (payload: unknown) => ({ ok: true, @@ -97,6 +104,63 @@ const setupClientMocks = ({ })); }; +const setupAndroidApkMocks = ( + downloadAndInstallApk: ReturnType, +) => { + (globalThis as any).__DEV__ = false; + + mock.module('react-native', () => ({ + Platform: { + OS: 'android', + Version: 30, + }, + DeviceEventEmitter: { + addListener: mock(() => ({ remove: mock(() => {}) })), + }, + NativeEventEmitter: class { + addListener = mock(() => ({ remove: mock(() => {}) })); + removeAllListeners = mock(() => {}); + }, + })); + + mock.module('../core', () => ({ + PushyModule: { + markSuccess: mock(() => {}), + reloadUpdate: mock(() => Promise.resolve()), + setNeedUpdate: mock(() => Promise.resolve()), + downloadPatchFromPpk: mock(() => Promise.resolve()), + downloadPatchFromPackage: mock(() => Promise.resolve()), + downloadFullUpdate: mock(() => Promise.resolve()), + downloadAndInstallApk, + restartApp: mock(() => Promise.resolve()), + }, + buildTime: '2023-01-01', + cInfo: { rnu: '10.0.0', rn: '0.73.0', os: 'android', uuid: 'uuid' }, + currentVersion: 'hash', + currentVersionInfo: {}, + isFirstTime: false, + isRolledBack: false, + packageVersion: '1.0.0', + pushyNativeEventEmitter: { + addListener: mock(() => ({ remove: mock(() => {}) })), + }, + rolledBackVersion: '', + setLocalHashInfo: mock(() => {}), + })); + + mock.module('../permissions', () => ({ + PermissionsAndroid: { + request: mock(() => Promise.resolve('granted')), + PERMISSIONS: { WRITE_EXTERNAL_STORAGE: 'WRITE_EXTERNAL_STORAGE' }, + RESULTS: { GRANTED: 'granted' }, + }, + })); + + mock.module('../i18n', () => ({ + default: { t: (key: string) => key, setLocale: mock(() => {}) }, + })); +}; + describe('Pushy server config', () => { test('uses preset main endpoints directly as configured endpoints', async () => { setupClientMocks(); @@ -601,6 +665,45 @@ describe('downloadUpdate fallback chain', () => { 'error_full_patch_failed', ); }); + + test('deduplicates concurrent downloads of the same hash (JS-8)', async () => { + // A slow native download keeps the first call in-flight while the second + // starts, so the dedup must reuse the same promise instead of triggering a + // second native download. + let resolveDownload: (() => void) | undefined; + const downloadFullUpdate = mock( + () => + new Promise(resolve => { + resolveDownload = resolve; + }), + ); + setupDownloadMocks({ downloadFullUpdate }); + const { Pushy, sharedState } = await importFreshClient('dl-concurrent-dedup'); + sharedState.downloadedHash = undefined; + // Only a full URL so the full strategy is the one exercised. + const fullOnlyInfo = { + update: true as const, + hash: 'new-hash', + full: 'full.ppk', + paths: ['https://cdn.example.com'], + name: 'v2.0', + description: 'test update', + }; + // No onDownloadProgress passed — old code only deduped when a progress + // handler was registered, so this exercises the new promise-table dedup. + const client = new Pushy({ appKey: 'demo-app' }); + + const first = client.downloadUpdate(fullOnlyInfo); + const second = client.downloadUpdate(fullOnlyInfo); + // Let both calls reach the (single) native download, then let it finish. + await new Promise(r => setTimeout(r, 10)); + resolveDownload?.(); + const [firstHash, secondHash] = await Promise.all([first, second]); + + expect(firstHash).toBe('new-hash'); + expect(secondHash).toBe('new-hash'); + expect(downloadFullUpdate).toHaveBeenCalledTimes(1); + }); }); describe('Cresc class', () => { @@ -681,3 +784,31 @@ describe('Cresc class', () => { ]); }); }); + +describe('downloadAndInstallApk apkStatus (JS-3)', () => { + test('resets apkStatus after a failed download so retry is possible', async () => { + // First download fails, second succeeds. The old code unconditionally set + // apkStatus='downloaded' after a caught failure, permanently blocking retry. + let callCount = 0; + const downloadAndInstallApk = mock(() => { + callCount++; + if (callCount === 1) { + return Promise.reject(Error('download fail')); + } + return Promise.resolve(); + }); + setupAndroidApkMocks(downloadAndInstallApk); + const { Pushy, sharedState } = await importFreshClient('apk-retry'); + sharedState.apkStatus = null; + const client = new Pushy({ appKey: 'demo-app' }); + + await client.downloadAndInstallApk('https://example.com/app.apk'); + // Failure must leave apkStatus reset (null), not stuck at 'downloaded'. + expect(sharedState.apkStatus).toBe(null); + + // A retry should now proceed and reach the native module a second time. + await client.downloadAndInstallApk('https://example.com/app.apk'); + expect(downloadAndInstallApk).toHaveBeenCalledTimes(2); + expect(sharedState.apkStatus).toBe('downloaded'); + }); +});