Conversation
…crash (#48302) # Why Split out of #48209 at @Ubax's request — that PR fixed two separate issues. This one is the icon type mismatch crash; the `xcasset` asset-catalog fix is in #48301. `NativeTabsView.ios.tsx` converted `icon` with the *normal* icon color and `selectedIcon` with the *selected* icon color: ```tsx const iosIcon = convertOptionsIconToScreensPropsIcon( shared.icon, standardAppearance?.stacked?.normal?.tabBarItemIconColor ); const iosSelectedIcon = convertOptionsIconToScreensPropsIcon( shared.selectedIcon ?? shared.icon, standardAppearance?.stacked?.selected?.tabBarItemIconColor ); ``` That color is what decides between `imageSource` and `templateSource`, so setting only one of `iconColor` / `selectedIconColor` made the two icons resolve to different types, and react-native-screens throws on that: ``` Error: [RNScreens] icon and selectedIcon must be same type. ``` That is a hard crash (red screen) for a plausible config — for example `<NativeTabs iconColor={{ selected: 'red' }}>` with any `src` icon. # How Derive the icon color from both states (`normal ?? selected`) and pass the same value to both conversions. The color is only used as a "should this image be a template" signal, so merging is the right granularity — the rendering mode is a property of the tab bar item, not of one state. This only changes behavior in the case that previously threw. # Test Plan Added to `native-tabs/__tests__/options.e2e.test.ios.tsx`: a table asserting `icon.type === selectedIcon.type` across all four `iconColor` / `selectedIconColor` combinations, verified against the real `Tabs.Screen`, not a mock. Red before the change, green after: ``` # on main ✕ icon and selectedIcon have the same type when only selectedIconColor is set ✓ icon and selectedIcon have the same type when only iconColor is set ✓ icon and selectedIcon have the same type when both icon colors are set ✓ icon and selectedIcon have the same type when no icon color is set Tests: 1 failed, 63 passed, 64 total # with the fix Tests: 64 passed, 64 total ``` ``` $ et check-packages expo-router 🏁 All checks passed. ``` # Checklist - [x] Added a `CHANGELOG.md` entry - [x] Built, type-checked, linted and tested via `et check-packages expo-router` - [x] Documentation — n/a, no public API change --------- Co-authored-by: Aman Mittal <amandeepmittal@live.com>
…dy evaluation (#48426) Fix modifier application rebuilding a fresh `AnyViewModifier` every body evaluation, which defeated AttributeGraph subtree pruning during scroll. # Why `AnyViewModifier` stores a closure, so building it here means a fresh heap box every body evaluation. AttributeGraph compares modifier values byte-wise to decide whether a subtree can be skipped, and a fresh box always compares as changed so nothing is ever pruned during scroll. This leads to a heavy load on the main thread and lags in scrolling on older devices. # How AttributeGraph decides whether a subtree can be skipped by byte-comparing the modifier value against the previous frame's. StableViewModifier storing representation is three pointers: the params dictionary's heap buffer and two context objects all of which stay identical across body evaluations until JS sends a new modifier array. Equal bytes -> subtree pruned. # Test Plan The change was tested on a production tvOS application by sample tool with a large number of complex elements in the scroll (many swift-ui components and modifiers), in both regular stacks and lazy stacks. ``` sample <pid> 10 -file scroll.txt ``` Without this change: main thead busy 50%, AG::Subgraph::update calls > 500 With this change: main thead busy 20% , AG::Subgraph::update calls < 200 # Checklist - [x] I added a `changelog.md` entry and rebuilt the package sources according to [this short guide](https://github.com/expo/expo/blob/main/CONTRIBUTING.md#-before-submitting) - [x] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin). - [x] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) --------- Co-authored-by: nishan (o^▽^o) <nishanbende@gmail.com>
…49480) Co-authored-by: Kudo Chien <kudo@expo.dev>
…against concurrent access (#49554) # Why Fixes #49549. Previously reported as #47702, #48202 and #48992, each auto-closed for lacking a reproduction. `NotificationCenterManager` is a process-wide singleton whose `delegates` and `pendingResponses` were unsynchronized stored properties. They are mutated by `addDelegate`, `removeDelegate` and the `pendingResponses.append` in `userNotificationCenter(_:didReceive:withCompletionHandler:)`, and read by every `for delegate in delegates` callback — from whichever thread happens to be creating or destroying an app context. App contexts are not singletons. `PushTokenModule`, `HandlerModule` and `EmitterModule` each call `addDelegate` from `OnCreate` and `removeDelegate` from `OnDestroy`, so an incoming context registering its modules overlaps a departing context removing its own. That overlap is routine: every `expo-dev-client` reload, and `Updates.reloadAsync()` in a release build. Concurrent `Array.append` then corrupts the buffer and the process dies with `SIGSEGV` inside module registration. # How Both arrays move into a `NotificationDelegateRegistry` that keeps them behind `Mutex`, the backport in `ExpoModulesCore` that `expo-app-metrics` and `expo-app-intents` already use (`Synchronization.Mutex` is iOS 18+, this package targets 16.4). Readers take a snapshot instead of holding the lock across a delegate callback. That matters: `addDelegate` calls `didReceive` on the delegate it just added, and a delegate is free to add or remove delegates from there — holding the lock across that would deadlock. `add` therefore returns the pending responses rather than exposing them separately. Extracting the registry is what makes the synchronization testable. The manager is a singleton whose `init` calls `install()`, which reaches `UNUserNotificationCenter.current()` and crashes in a unit test bundle — I hit exactly that (`Crash: xctest at NotificationCenterManager.install()`) with a first version of these tests written against `.shared`. The registry has no such dependency. `chainedDelegate` is left alone. It is written only from `install()` and is a separate concern from the reported crash. # Test Plan ## The reporter's standalone repro Their harness needs no Expo project, simulator or Xcode project. I re-ran it with the storage and the bodies of `addDelegate`, `removeDelegate` and `didRegister` copied verbatim from `main` — including `removeDelegate`'s current `removeAll { $0 === delegate }`, not the older `firstIndex` form quoted in the issue. | build | ThreadSanitizer | 12 unsanitized runs | | --- | --- | --- | | `main` | race reported in `addDelegate` | **10–12 of 12 died with SIGSEGV** | | this PR | **0 warnings** | **0 of 12** | TSan on `main`'s logic: ``` WARNING: ThreadSanitizer: Swift access race #0 NotificationCenterManager.addDelegate(_:) #1 PushTokenModule.onCreate() Previous read: #0 NotificationCenterManager.didRegister(_:) SUMMARY: ThreadSanitizer: Swift access race in NotificationCenterManager.addDelegate(_:)+0x10c ``` ## Unit tests Five new tests in `ios/Tests/NotificationDelegateRegistryTests.swift`. `et native-unit-tests -p ios --packages expo-notifications`: ``` NotificationDelegateRegistryTests ✓ keeps every delegate added from several threads at once ✓ removes every delegate removed from several threads at once ✓ keeps the survivors when adds and removes interleave across threads ✓ add returns the responses the new delegate still has to be offered ✓ removing a delegate that was never added leaves the others in place BackgroundEventTransformerTests (existing, 5 tests) ✓ ** TEST SUCCEEDED ** ``` Mutation check — reverting the registry to plain unsynchronized arrays and keeping everything else: ``` ✕ keeps every delegate added from several threads at once (0.000s) ✕ removes every delegate removed from several threads at once (0.000s) ✕ keeps the survivors when adds and removes interleave across threads (0.000s) ✓ add returns the responses the new delegate still has to be offered ✓ removing a delegate that was never added leaves the others in place ** TEST FAILED ** Crash: xctest at closure #2 in NotificationDelegateRegistryTests. `removes every delegate removed from several threads at once`() ``` Exactly the three concurrency tests fail, and the two single-threaded ones still pass, so the tests are measuring the synchronization rather than passing by construction. ## One thing I had to add to run any of this `test_spec` gains `'OTHER_LDFLAGS' => '$(inherited) -lc++'`. Without it the test target fails to link: ``` Undefined symbol: std::runtime_error::what() const Undefined symbol: std::logic_error::logic_error(char const*) ... in libExpoModulesCore.a[3](EventEmitter-....o) ExpoNotifications-Unit-Tests: clang: error: linker command failed with exit code 1 ``` This is not caused by anything here — it is the same flag the test specs of `expo-maps`, `expo-camera`, `expo-audio`, `expo-updates`, `expo-image-picker`, `expo-observe`, `expo-app-metrics` and `expo-dev-launcher` already carry. Without it no test can run for this package at all, so the existing `BackgroundEventTransformerTests` could not have been running locally either. Happy to split it out if you would rather have it separately. `Podfile.lock` carries the resulting `ExpoNotifications` checksum change, the same way the RN 0.87 upgrade PRs updated it. ## Not covered I did not reproduce the crash in a running app on a device or simulator — the reporter's own trace does that, from a real build under Maestro relaunch cycles. Everything above is TSan output, crash counts and test results from this machine. `et check-packages expo-notifications` reports `expo-notifications#lint` failing, but that reproduces unchanged on `main` (`Rule 'globals' not found in plugin 'react'`, an oxlint config/version mismatch). This PR touches no JavaScript or TypeScript. # Checklist - [x] Added a `CHANGELOG.md` entry. - [x] Added unit tests that fail without the fix. - [x] Conforms to the [documentation writing style guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md). --------- Co-authored-by: Vojtech Novak <vonovak@gmail.com>
# Why Fixes #37842. Any Jest test that imports `expo-video`, directly or transitively, fails before a single test body runs: ``` TypeError: Cannot read properties of undefined (reading 'prototype') > 9 | const replace = NativeVideoModule.VideoPlayer.prototype.replace; | ^ at Object.prototype (src/VideoPlayer.tsx:9:47) ``` `VideoPlayer.tsx` patches `NativeVideoModule.VideoPlayer.prototype.replace` at module load. Under the `jest-expo` preset, `requireNativeModule` is swapped for `requireMockModule`, which looks for `packages/<pkg>/mocks/<ModuleName>` and otherwise falls back to the generated table in `jest-expo/src/preset/moduleMocks/expoModules.js`. `expo-video` had no `mocks/` directory, and the generated table only describes flat functions and constants — it has no way to express a SharedObject **class** — so `ExpoVideo.VideoPlayer` was `undefined`. There are two independent defects behind the single symptom, and fixing only the first still leaves a consuming app broken. # How **1. `packages/expo-video/mocks/ExpoVideo.ts` (new).** A hand-written mock covering exactly the six members `NativeVideoModule.ts` declares on `ExpoVideoModule` — `VideoPlayer`, `VideoThumbnail`, `isPictureInPictureSupported`, `setVideoCacheSizeAsync`, `clearVideoCacheAsync`, `getCurrentVideoCacheSize` — following the conventions in `packages/expo-file-system/mocks/FileSystem.ts`. `VideoPlayer` is a real `class` extending the `SharedObject` polyfill rather than an object factory. That matters in two places: `VideoPlayer.tsx` reads `.prototype.replace`/`.prototype.replaceAsync`, and `VideoView.tsx:103` narrows with `player instanceof NativeVideoModule.VideoPlayer`. An arrow function or plain factory has no `.prototype` and is not constructible, so it would not survive either. The player keeps a little in-memory state (playing flag, current time, current source) so tests can assert on `play()`/`pause()`/`seekBy()`/`replace()` instead of inert stubs. `VideoThumbnail` spells out a `release()` override that only forwards to `SharedRef`. This is deliberate: `requireMockModule` decides whether an export is a class by counting own properties on its prototype, so a class carrying nothing but instance fields is mistaken for a plain function and wrapped in `jest.fn()` — which makes it throw `Cannot call a class as a function` on construction. `FileSystemUploadTask` spells `release` out for the same reason. **2. `packages/jest-expo/src/preset/resolveExistingFile.js` (new), used by `attemptLookup`.** The mock alone fixes tests that import `expo-video` from source, but *not* a consuming app, which resolves the published `build/` output. `attemptLookup` walks the stack to find the file that called `requireNativeModule`, and skips any frame whose file does not exist on disk. Frames from a built package are source-mapped back to the original TypeScript path — `build/NativeVideoModule.js.map` carries `"sources": ["NativeVideoModule.ts"]`, so the frame reads `build/NativeVideoModule.ts`, which is never emitted. The frame was skipped, the lookup returned `null`, and the mock was never found. `expo-file-system` is immune only by luck: its call site lives in `ExpoFileSystem.js`, and `attemptLookup`'s first check is `fileName.includes(moduleName)` — `'ExpoFileSystem.js'.includes('FileSystem')` short-circuits before the existence check. `expo-video`'s call site is `NativeVideoModule.ts`, which does not contain `ExpoVideo`, so it depends on the fallback path that was broken. The fix resolves a frame to the sibling `.js` when the source-mapped path is absent. It is extracted into its own module so it can be unit-tested without executing the setup file's mocking side effects. **3. Supporting changes.** `expo-video` gains a `test` script, the `expo-module-scripts` jest preset, and a `jest-expo` devDependency (matching `expo-file-system`), and its `tsconfig.json` now type-checks `mocks/` alongside `src/` so the mock cannot silently drift from the module it stands in for. No `package.json` `files`/`.npmignore` change is needed — `mocks/` already ships (verified with `npm pack`, below). # Test Plan **New tests.** `packages/expo-video/src/__tests__/VideoPlayer-test.native.ts` (8 tests × iOS/Android) covers module load, the exact member list the native module declares, `instanceof`, both prototype-patched `replace`/`replaceAsync` wrappers including the deprecation warning, playback state, and thumbnail construction. `packages/jest-expo/tests/__tests__/resolveExistingFile-test.js` covers the frame resolution. ``` $ cd packages/expo-video && pnpm test PASS Android src/__tests__/VideoPlayer-test.native.ts PASS iOS src/__tests__/VideoPlayer-test.native.ts Test Suites: 2 passed, 2 total Tests: 16 passed, 16 total $ cd packages/jest-expo && pnpm test Test Suites: 29 passed, 29 total Tests: 113 passed, 113 total ``` **Counterfactual.** With `mocks/ExpoVideo.ts` removed, the new test fails with the original error, at the original location: ``` ● Test suite failed to run TypeError: Cannot read properties of undefined (reading 'prototype') > 9 | const replace = NativeVideoModule.VideoPlayer.prototype.replace; at Object.prototype (src/VideoPlayer.tsx:9:47) ``` Restoring it returns 16/16. **The user-visible path** — a consuming app resolving the built package, which is what the issue actually reports. Run from `apps/test-suite` with `preset: 'jest-expo'`, importing by package name so it resolves `packages/expo-video/build/index.js`: ```js import { useVideoPlayer, VideoView, createVideoPlayer, isPictureInPictureSupported } from 'expo-video'; it('a consuming app can import expo-video and drive a player', () => { expect(typeof useVideoPlayer).toBe('function'); expect(VideoView).toBeDefined(); expect(isPictureInPictureSupported()).toBe(false); const player = createVideoPlayer('https://example.com/v.mp4'); player.replace('https://example.com/w.mp4', true); player.play(); expect(player.playing).toBe(true); }); ``` With the mock but *without* the `attemptLookup` fix this still failed at `build/VideoPlayer.tsx:9:47`; with both it passes. (Scratch file, not committed.) **No regressions.** `expo-sqlite` 10 suites / 190 passed and `expo-file-system` 10 suites / 218 passed are unchanged, confirming the `attemptLookup` change does not disturb packages that already resolved their mocks. **Packaging.** `npm pack` on `expo-video` contains `package/mocks/ExpoVideo.ts`. `et check-packages expo-video jest-expo` passes (build, typecheck, lint, format, test). # Checklist - [x] Added a `CHANGELOG.md` entry to `expo-video` and `jest-expo` - [x] Builds, type-checks, lints, and tests via `et check-packages expo-video jest-expo` - [ ] Not applicable — no native or config-plugin change, so `npx expo prebuild` / EAS Build are unaffected # Credit @behenate proposed a mock for this in #39707, which was closed unreviewed in a sweep of PRs older than six months rather than on its merits. This PR is independent but takes the same direction. Two notes on the differences, both from re-testing against current `main`: - #39707 defined `VideoPlayer` as an arrow function returning an object literal. On today's `main` that would not fix the crash — an arrow function has no `.prototype`, so line 9 would still throw, and `createVideoPlayer`'s `new NativeVideoModule.VideoPlayer(...)` plus `VideoView`'s `instanceof` check both need a real constructor. - #39707 also removed `ExpoVideo` from the generated mock table. I left that alone: `ExpoAsset`, `ExpoLocation`, `ExpoAgeRange`, and `ExpoAppIntegrity` all ship a hand-written `mocks/` directory *and* keep their generated entry, so removing it is not the established convention, and `attemptLookup` takes priority over the table regardless. Happy to add it if you'd prefer the consistency. Co-authored-by: Wojciech Dróżdż <behenate@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )