Revert "firestore: minor refactor of listener registration of "versionchange" indexedb events (#9087) (#9168)

* Revert "firestore: minor refactor of listener registration of "versionchange" indexedb events (#9087)"

This reverts commit f73e08b.

* Create hungry-icons-dream.md

Author: MarkDuckworth

.changeset/hungry-icons-dream.md

@@ -0,0 +1,5 @@
+---
+"@firebase/firestore": patch
+---
+
+Fixed a regression where the SDK did not re-connect to IndexedDb after disconnect (#9087)

packages/firestore/src/core/firestore_client.ts

@@ -231,23 +231,11 @@ export async function setOfflineComponentProvider(
     }
   });
 
-  offlineComponentProvider.persistence.setDatabaseDeletedListener(() => {
-    logWarn('Terminating Firestore due to IndexedDb database deletion');
-    client
-      .terminate()
-      .then(() => {
-        logDebug(
-          'Terminating Firestore due to IndexedDb database deletion ' +
-            'completed successfully'
-        );
-      })
-      .catch(error => {
-        logWarn(
-          'Terminating Firestore due to IndexedDb database deletion failed',
-          error
-        );
-      });
-  });
+  // When a user calls clearPersistence() in one client, all other clients
+  // need to be terminated to allow the delete to succeed.
+  offlineComponentProvider.persistence.setDatabaseDeletedListener(() =>
+    client.terminate()
+  );
 
   client._offlineComponents = offlineComponentProvider;
 }

packages/firestore/src/local/indexeddb_persistence.ts

@@ -58,11 +58,7 @@ import { IndexedDbTargetCache } from './indexeddb_target_cache';
 import { getStore, IndexedDbTransaction } from './indexeddb_transaction';
 import { LocalSerializer } from './local_serializer';
 import { LruParams } from './lru_garbage_collector';
-import {
-  DatabaseDeletedListener,
-  Persistence,
-  PrimaryStateListener
-} from './persistence';
+import { Persistence, PrimaryStateListener } from './persistence';
 import { PersistencePromise } from './persistence_promise';
 import {
   PersistenceTransaction,
@@ -328,25 +324,20 @@ export class IndexedDbPersistence implements Persistence {
   }
 
   /**
-   * Registers a listener that gets called when the underlying database receives
-   * an event indicating that it either has been deleted or is pending deletion
-   * and must be closed.
-   *
-   * For example, this callback will be called in the case that multi-tab
-   * IndexedDB persistence is in use and another tab calls
-   * clearIndexedDbPersistence(). In that case, this Firestore instance must
-   * close its IndexedDB connection in order to allow the deletion initiated by
-   * the other tab to proceed.
-   *
-   * This method may only be called once; subsequent invocations will result in
-   * an exception, refusing to supersede the previously-registered listener.
+   * Registers a listener that gets called when the database receives a
+   * version change event indicating that it has deleted.
    *
    * PORTING NOTE: This is only used for Web multi-tab.
    */
   setDatabaseDeletedListener(
-    databaseDeletedListener: DatabaseDeletedListener
+    databaseDeletedListener: () => Promise<void>
   ): void {
-    this.simpleDb.setDatabaseDeletedListener(databaseDeletedListener);
+    this.simpleDb.setVersionChangeListener(async event => {
+      // Check if an attempt is made to delete IndexedDB.
+      if (event.newVersion === null) {
+        await databaseDeletedListener();
+      }
+    });
   }
 
   /**

packages/firestore/src/local/persistence.ts

@@ -98,8 +98,6 @@ export interface ReferenceDelegate {
   ): PersistencePromise<void>;
 }
 
-export type DatabaseDeletedListener = () => void;
-
 /**
  * Persistence is the lowest-level shared interface to persistent storage in
  * Firestore.
@@ -153,23 +151,13 @@ export interface Persistence {
   shutdown(): Promise<void>;
 
   /**
-   * Registers a listener that gets called when the underlying database receives
-   * an event indicating that it either has been deleted or is pending deletion
-   * and must be closed.
-   *
-   * For example, this callback will be called in the case that multi-tab
-   * IndexedDB persistence is in use and another tab calls
-   * clearIndexedDbPersistence(). In that case, this Firestore instance must
-   * close its IndexedDB connection in order to allow the deletion initiated by
-   * the other tab to proceed.
-   *
-   * This method may only be called once; subsequent invocations will result in
-   * an exception, refusing to supersede the previously-registered listener.
+   * Registers a listener that gets called when the database receives a
+   * version change event indicating that it has deleted.
    *
    * PORTING NOTE: This is only used for Web multi-tab.
    */
   setDatabaseDeletedListener(
-    databaseDeletedListener: DatabaseDeletedListener
+    databaseDeletedListener: () => Promise<void>
   ): void;
 
   /**

packages/firestore/src/local/simple_db.ts

@@ -19,10 +19,9 @@ import { getGlobal, getUA, isIndexedDBAvailable } from '@firebase/util';
 
 import { debugAssert } from '../util/assert';
 import { Code, FirestoreError } from '../util/error';
-import { logDebug, logError, logWarn } from '../util/log';
+import { logDebug, logError } from '../util/log';
 import { Deferred } from '../util/promise';
 
-import { DatabaseDeletedListener } from './persistence';
 import { PersistencePromise } from './persistence_promise';
 
 // References to `indexedDB` are guarded by SimpleDb.isAvailable() and getGlobal()
@@ -159,7 +158,8 @@ export class SimpleDbTransaction {
  */
 export class SimpleDb {
   private db?: IDBDatabase;
-  private databaseDeletedListener?: DatabaseDeletedListener;
+  private lastClosedDbVersion: number | null = null;
+  private versionchangelistener?: (event: IDBVersionChangeEvent) => void;
 
   /** Deletes the specified database. */
   static delete(name: string): Promise<void> {
@@ -365,35 +365,22 @@ export class SimpleDb {
       });
     }
 
-    this.db.addEventListener(
-      'versionchange',
-      event => {
-        // Notify the listener if another tab attempted to delete the IndexedDb
-        // database, such as by calling clearIndexedDbPersistence().
-        if (event.newVersion === null) {
-          logWarn(
-            `Received "versionchange" event with newVersion===null; ` +
-              'notifying the registered DatabaseDeletedListener, if any'
-          );
-          this.databaseDeletedListener?.();
-        }
-      },
-      { passive: true }
-    );
+    if (this.versionchangelistener) {
+      this.db.onversionchange = event => this.versionchangelistener!(event);
+    }
 
     return this.db;
   }
 
-  setDatabaseDeletedListener(
-    databaseDeletedListener: DatabaseDeletedListener
+  setVersionChangeListener(
+    versionChangeListener: (event: IDBVersionChangeEvent) => void
   ): void {
-    if (this.databaseDeletedListener) {
-      throw new Error(
-        'setDatabaseDeletedListener() may only be called once, ' +
-          'and it has already been called'
-      );
+    this.versionchangelistener = versionChangeListener;
+    if (this.db) {
+      this.db.onversionchange = (event: IDBVersionChangeEvent) => {
+        return versionChangeListener(event);
+      };
     }
-    this.databaseDeletedListener = databaseDeletedListener;
   }
 
   async runTransaction<T>(

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -365,14 +365,8 @@ abstract class TestRunner {
     this.eventManager.onLastRemoteStoreUnlisten =
       triggerRemoteStoreUnlisten.bind(null, this.syncEngine);
 
-    this.persistence.setDatabaseDeletedListener(() => {
-      this.shutdown().catch(error => {
-        console.warn(
-          'WARNING: this.shutdown() failed in callback ' +
-            'specified to persistence.setDatabaseDeletedListener',
-          error
-        );
-      });
+    await this.persistence.setDatabaseDeletedListener(async () => {
+      await this.shutdown();
     });
 
     this.started = true;