fix(app, android): stop event emitter losing events to a stale ReactContext#9113
fix(app, android): stop event emitter losing events to a stale ReactContext#9113KAMRONBEK wants to merge 3 commits into
Conversation
…ontext Under the new architecture more than one ReactContext generation can exist during startup (silent destroy+recreate on release-mode startup exceptions, headless-created contexts, TurboModuleManager initializing never-built modules during teardown). The emitter singleton captured its context once, last-writer-wins, and never re-validated, re-attached or cleared it - so it could emit every event of a session into a runtime with no JS listeners. On bridgeless a stale context cannot be detected (hasActiveCatalystInstance delegates to the host) so it must be replaced, not validated: - resolve the context hosting the live JS runtime at emit time via the application's ReactHost/ReactNativeHost, falling back to the attached context (notifee invertase#1127 pattern) - arbitrate attaches against the host's current context and re-attach on every onHostResume via a LifecycleEventListener - hold the attached context weakly and detach it (identity-guarded) from the app module's invalidate, resetting jsReady/listener state; queued events are preserved for the messaging kill-state path - perform attach/detach/ready transitions synchronously under the listener monitor so generation overlap cannot reorder them against a replacement runtime's registrations - expose jsReady/attachedContextHash/currentContextHash via eventsGetListeners for on-device diagnosis Fixes invertase#8374
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Very interesting - thanks for posting this - this area has obviously been a difficult one for the library with all the issues and regressions we've had. I don't hold it against the PR that there's no solid testing for it - it would have been presumptuous to make architecture level decisions like "I've added unit tests using robolectric and mockito" but that's what is really needed here. I've put this through some internal review and found a couple gaps that are provable with JVM / unit testing and I've got some fixes plus the installation of a full unit-testing setup for the repo along with coverage tracking for same via changes to our github actions workflows I'll push a commit with those a moment as follow-ons in this PR and you'll see the extra gaps found plus the reasoning behind them along with all the unit test machinery |
- Add Robolectric + Mockito JVM unit-test deps and `yarn tests:android:unit` - Wire Jacoco unit `*.exec` into merged `jacocoTestReport` / Codecov for native coverage - Update OKF coverage-design, validation checklist, agent-command-policy, and CI Android workflow - Accept AndroidTest-AD-1: Robolectric + Mockito for Android JVM unit tests
- Fail-closed attach, pending/hostLag convergence, and restore-on-pending-cancel - Prefer pending / restored attached over unrelated hostCurrent to avoid silent loss - Flush queued events after pending-only cancel; add Robolectric regressions (LINE/BRANCH 100%)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9113 +/- ##
============================================
+ Coverage 65.81% 66.07% +0.27%
- Complexity 1833 1904 +71
============================================
Files 497 497
Lines 38835 39065 +230
Branches 5803 5816 +13
============================================
+ Hits 25555 25810 +255
+ Misses 11854 11836 -18
+ Partials 1426 1419 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Description
On Android with the New Architecture,
@react-native-firebaseevents (auth_state_changed, firestore snapshot sync events, etc.) can stop being delivered to JS for an entire app session — typically on the first launch after a fresh install, predominantly in release builds — and only a full app restart recovers. This is the long-standing #8374 (and the same failure class documented in #8900 and #8787).The race, untangled
ReactNativeFirebaseEventEmitteris a process-lifetime singleton whoseReactContextis captured once, when the app module'sinitialize()runs, last-writer-wins, and is never re-validated, re-attached, or cleared. That was sound under the old architecture, where modules are created eagerly, in order, exactly once per instance. Under the New Architecture it is not:ReactHostImpl.handleHostExceptionsilently destroys the instance and the next call recreates it — debug shows a redbox instead, which is why debug "works"), a headless/FCM-created context, or a third-party module pinning an old context ([🐛][Android] From a certain point on, all RTDB listeners stop working. #8900) all produce a second generation.TurboModuleManager.invalidate()creates-and-initialize()s modules that were requested but never built, during teardown of the old instance — so the old generation's app module can postattachReactContext(oldContext)after the new generation already attached. Module creation is also lazy (driven by JS access timing), so attach ordering across generations is a pure startup-timing race — which is why release/debug speed flips the outcome and minification is irrelevant.BridgelessReactContext.hasActiveCatalystInstance()delegates to the host's instance state, and itsgetJSModule(RCTDeviceEventEmitter).emit(...)proxy is fire-and-forget. Soemit()on a stale context "succeeds", the event is dequeued as delivered, and it lands in a runtime that has no listeners — exactly the observation in auth().onAuthStateChanged does not always trigger after sign-in on Android in React Native 0.77 #8374 ("the reactContext stored in ReactNativeFirebaseEventEmitter … points to a react context that does not contain the listeners") and the identity-hash logs in [🐛][Android] From a certain point on, all RTDB listeners stop working. #8900 (Module Context: 73152123vsEmitter's Context: 61897238).iOS is immune by construction:
RNFBRCTEventEmitterholds a weak, framework-managed bridge reference that RN reassigns per instance, andRNFBAppModule.invalidatefully resets emitter state. Android had no equivalent of either — this PR brings the Android emitter to parity, using the same design @mikehardy shipped for the identical bug class in notifee (invertase/notifee#1127): never trust a captured context; resolve the current one.The fix (three layers,
packages/app/androidonly — every package's events funnel through this one emitter)emit()now resolves the context that currently hosts the JS runtime —ReactApplication.getReactHost().getCurrentReactContext(), falling back toReactNativeHost(guarded byhasInstance()so nothing is created as a side effect), falling back to the attached field (brownfield hosts that don't implementReactApplicationkeep exactly the old behavior). This is correct in both staleness directions (auth().onAuthStateChanged does not always trigger after sign-in on Android in React Native 0.77 #8374: emitter stale / [🐛][Android] From a certain point on, all RTDB listeners stop working. #8900: module stale) and regardless of which trigger produced the divergent generation. Also moves off deprecatedhasActiveCatalystInstance()tohasActiveReactInstance().attachReactContext()now rejects attaches of a context the host no longer considers current (closing the teardown-time stale-attach window), and the app module registers aLifecycleEventListenerso everyonHostResumere-attaches the live context — which also flushes the queue. Lifecycle events are only ever dispatched to the current context, so a stale generation can never re-assert itself; registering while already resumed immediately firesonHostResume(this is why the community patch's amended "always add the listener" variant worked).WeakReference(the singleton previously pinned a dead context and its module graph for the process lifetime), and the app module'sinvalidate()detaches it — identity-guarded so a dying generation clears only its own state (jsReady, listener registrations) and can never wipe state a replacement runtime has already installed. Queued events are intentionally preserved across detach: messaging's kill-state path relies on queueing until JS registers.All attach/detach/ready transitions now happen synchronously at cause time under the same monitor as listener registration, with only queue-flushing posted to the main looper. This matters: with two generations overlapping, deferring a detach to a (possibly congested) main looper reorders it against the replacement runtime's synchronous
eventsAddListenercalls, and a late-running detach could wipe the new runtime's registrations. Synchronous transitions restore a total order — a replacement runtime always attaches (at module creation) before its JS can register listeners, so the identity guard in detach is sound in every interleaving.Scope/assumption: context resolution goes through the Application's
ReactApplicationhost, i.e. it assumes the standard single-ReactHosttopology (same assumption as notifee's fix). Apps whoseApplicationdoes not implementReactApplicationdegrade to exactly the previous attached-context behavior; exotic multi-ReactHostbrownfield setups were never really supported by a process-wide singleton emitter and rejected attaches are surfaced at warn level for diagnosability.Additionally,
eventsGetListeners()now reportsjsReady,attachedContextHashandcurrentContextHash(and reads under the lock) — the exact diagnostic requested in #8374 ("some diagnostics where the emitter's context object id was logged on-device… would probably show the context object being incorrect"), so field reports can confirm convergence with no debug build needed. Context attach/detach/rejection transitions are logged at debug level (state changes only, never per-event).Related issues
Fixes #8374
Likely fixes #8787 (same first-launch, Android-only, restart-recovers fingerprint on firestore/RTDB listeners)
Diagnostics/mechanism precedent: #8900, invertase/notifee#1127
Investigated and ruled out: firebase/firebase-android-sdk#8064 (auth persistence loss at process start — different layer; in #8374 native state is correct and only the native→JS event crossing fails) and #8981 (deterministic firestore cache-key crash, already fixed in #9097).
Release Summary
fix(app, android): events could be lost for an entire session when the emitter held a stale ReactContext (new architecture); the emitter now resolves the live context at emit time, re-converges on host resume, and resets state on invalidate
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__The race requires destroying/recreating a ReactInstance mid-startup, which the detox harness can't do deterministically; the existing
packages/app/e2e/events.e2e.jssuite (queue-before-ready, queue-before-listener, 100-event burst) covers the normal delivery paths against regression and passes unchanged — includingeventsNotifyReady(false)semantics, which are preserved.Test Plan
packages/app/e2e/events.e2e.jsassertions audited against the change: onlyevents.*keys are asserted; the new diagnostic keys are additive.cd tests/android && ./gradlew :react-native-firebase_app:compileDebugJavaWithJavac→ BUILD SUCCESSFUL. All APIs used (ReactApplication.getReactHost,ReactHost.getCurrentReactContext,hasActiveReactInstance,LifecycleEventListener) verified present since RN 0.74, within this repo's New-Architecture-required support range.google-java-formatclean;yarn tests:jestgreen (1184 tests — no JS touched).TurboModuleManager.invalidate()creates-and-initializes requested-but-never-built modules during old-instance teardown (TurboModuleManager.java:391-425), inside the reload/destroy task chain (ReactHostImpl.java:1485-1567). With this fix: the old generation'sattach(old)is rejected when the host's current context is the new one; if the host is unresolvable at that instant it is accepted but the paireddetach(old)(same teardown, before the next generation exists) clears it. Either way the replacement runtime's listeners andjsReadysurvive, and emit-time resolution delivers into the live runtime regardless of the attached field.