diff --git a/packages/firestore/android/src/main/java/io/invertase/firebase/firestore/UniversalFirebaseFirestoreCommon.java b/packages/firestore/android/src/main/java/io/invertase/firebase/firestore/UniversalFirebaseFirestoreCommon.java index b241b9344c..7d9c673b5c 100644 --- a/packages/firestore/android/src/main/java/io/invertase/firebase/firestore/UniversalFirebaseFirestoreCommon.java +++ b/packages/firestore/android/src/main/java/io/invertase/firebase/firestore/UniversalFirebaseFirestoreCommon.java @@ -26,10 +26,14 @@ import io.invertase.firebase.app.ReactNativeFirebaseVersion; import io.invertase.firebase.common.UniversalFirebasePreferences; import java.lang.ref.WeakReference; -import java.util.WeakHashMap; +import java.util.concurrent.ConcurrentHashMap; public class UniversalFirebaseFirestoreCommon { - static WeakHashMap> instanceCache = new WeakHashMap<>(); + // Values are already wrapped in WeakReference, so a WeakHashMap (weak keys) is not needed here + // and is actively harmful: firestoreKey is a freshly concatenated String with no other strong + // reference, so it would be eligible for GC almost immediately, evicting the cache entry. + static ConcurrentHashMap> instanceCache = + new ConcurrentHashMap<>(); static String createFirestoreKey(String appName, String databaseId) { return appName + ":" + databaseId; @@ -50,7 +54,7 @@ static FirebaseFirestore getFirestoreForApp(String appName, String databaseId) { setFirestoreSettings(instance, firestoreKey); - instanceCache.put(appName, new WeakReference(instance)); + instanceCache.put(firestoreKey, new WeakReference(instance)); return instance; } diff --git a/packages/firestore/e2e/issues.e2e.js b/packages/firestore/e2e/issues.e2e.js index a01b6c4374..4a78f7f0d7 100644 --- a/packages/firestore/e2e/issues.e2e.js +++ b/packages/firestore/e2e/issues.e2e.js @@ -124,6 +124,77 @@ describe('firestore()', function () { }); }); + describe('issue 8981 - android firestore instance cache key mismatch', function () { + // A real, already-provisioned second database in the test project - see + // packages/firestore/e2e/SecondDatabase/*.e2e.js. + const SECOND_DATABASE_ID = 'second-rnfb'; + + it('does not throw when switching databases on the same app and reapplying settings', async function () { + if (Platform.other) { + // Not supported on web lite sdk - no persistent native instance cache to break + return; + } + + const { initializeApp, deleteApp } = modular; + const { getFirestore, doc, setDoc, getDoc } = firestoreModular; + + // A dedicated, dynamically created app guarantees Firestore instances that have + // never been started before this test runs. + const appName = `firestoreIssue8981${FirebaseHelpers.id}`; + const app = await initializeApp(FirebaseHelpers.app.config(), appName); + + try { + // Same app, two different database IDs. + // + // Regression test for https://github.com/invertase/react-native-firebase/issues/8981: + // on Android, `UniversalFirebaseFirestoreCommon.getFirestoreForApp()` read the + // instance cache keyed by `appName:databaseId` but wrote it back keyed by + // `appName` alone. Both databases therefore collided on a single cache slot + // that could never actually be hit on lookup (which always includes the + // `:databaseId` suffix) - switching from one database to another and back + // never protects either instance's settings from being re-derived. + const defaultDb = getFirestore(app); + const secondDb = getFirestore(app, SECOND_DATABASE_ID); + + // Start the default database with an initial settings value. + await defaultDb.settings({ cacheSizeBytes: 1048576 }); + await setDoc(doc(defaultDb, `${COLLECTION}/issue8981/default`), { value: 1 }); + + // Switch to the second database under the *same* app and start it too - this + // is the "different db ID hitting the same app" half of the bug: both native + // calls are keyed by the same broken, appName-only cache slot. + await secondDb.settings({ cacheSizeBytes: 2097152 }); + await setDoc(doc(secondDb, `${COLLECTION}/issue8981/second`), { value: 2 }); + + // Switch back to the default database and reapply *different* settings to an + // already-started instance. Android's `FirebaseFirestore.setFirestoreSettings()` + // only throws when the newly-derived settings actually differ from what's + // already active (identical settings passed repeatedly are an explicit no-op + // in the native SDK) - so this value change is what deterministically triggers + // `IllegalStateException: FirebaseFirestore has already been started...` before + // the fix, once the cache-miss-on-every-call bug re-applies it natively. + await defaultDb.settings({ cacheSizeBytes: 5242880 }); + await setDoc(doc(defaultDb, `${COLLECTION}/issue8981/default`), { value: 3 }); + + const [snap1, snap2] = await Promise.all([ + getDoc(doc(defaultDb, `${COLLECTION}/issue8981/default`)), + getDoc(doc(secondDb, `${COLLECTION}/issue8981/second`)), + ]); + snap1.data().value.should.equal(3); + snap2.data().value.should.equal(2); + } catch (e) { + // Fail explicitly with the regression context rather than letting the native + // `IllegalStateException` (surfaced here as a rejected promise) bubble up with a + // generic message - see https://github.com/invertase/react-native-firebase/issues/8981. + fail( + `Regression in issue 8981: switching database on the same app and reapplying settings should not throw, but got: ${e}`, + ); + } finally { + await deleteApp(app); + } + }); + }); + describe('number type consistency', function () { before(async function () { // FIXME: