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

Author: dconeybe

.changeset/long-pets-sell.md

@@ -0,0 +1,5 @@
+---
+'@firebase/firestore': patch
+---
+
+Internal listener registration change for IndexedDB "versionchange" events.

packages/firestore/src/core/firestore_client.ts

@@ -230,11 +230,23 @@ export async function setOfflineComponentProvider(
     }
   });
 
-  // 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()
-  );
+  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
+        );
+      });
+  });
 
   client._offlineComponents = offlineComponentProvider;
 }

packages/firestore/src/local/indexeddb_persistence.ts

@@ -58,7 +58,11 @@ import { IndexedDbTargetCache } from './indexeddb_target_cache';
 import { getStore, IndexedDbTransaction } from './indexeddb_transaction';
 import { LocalSerializer } from './local_serializer';
 import { LruParams } from './lru_garbage_collector';
-import { Persistence, PrimaryStateListener } from './persistence';
+import {
+  DatabaseDeletedListener,
+  Persistence,
+  PrimaryStateListener
+} from './persistence';
 import { PersistencePromise } from './persistence_promise';
 import {
   PersistenceTransaction,
@@ -324,20 +328,25 @@ export class IndexedDbPersistence implements Persistence {
   }
 
   /**
-   * Registers a listener that gets called when the database receives a
-   * version change event indicating that it has deleted.
+   * 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.
    *
    * PORTING NOTE: This is only used for Web multi-tab.
    */
   setDatabaseDeletedListener(
-    databaseDeletedListener: () => Promise<void>
+    databaseDeletedListener: DatabaseDeletedListener
   ): void {
-    this.simpleDb.setVersionChangeListener(async event => {
-      // Check if an attempt is made to delete IndexedDB.
-      if (event.newVersion === null) {
-        await databaseDeletedListener();
-      }
-    });
+    this.simpleDb.setDatabaseDeletedListener(databaseDeletedListener);
   }
 
   /**

packages/firestore/src/local/persistence.ts

@@ -98,6 +98,8 @@ export interface ReferenceDelegate {
   ): PersistencePromise<void>;
 }
 
+export type DatabaseDeletedListener = () => void;
+
 /**
  * Persistence is the lowest-level shared interface to persistent storage in
  * Firestore.
@@ -151,13 +153,23 @@ export interface Persistence {
   shutdown(): Promise<void>;
 
   /**
-   * Registers a listener that gets called when the database receives a
-   * version change event indicating that it has deleted.
+   * 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.
    *
    * PORTING NOTE: This is only used for Web multi-tab.
    */
   setDatabaseDeletedListener(
-    databaseDeletedListener: () => Promise<void>
+    databaseDeletedListener: DatabaseDeletedListener
   ): void;
 
   /**

packages/firestore/src/local/simple_db.ts

@@ -19,9 +19,10 @@ import { getGlobal, getUA, isIndexedDBAvailable } from '@firebase/util';
 
 import { debugAssert } from '../util/assert';
 import { Code, FirestoreError } from '../util/error';
-import { logDebug, logError } from '../util/log';
+import { logDebug, logError, logWarn } 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()
@@ -158,8 +159,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> {
@@ -392,22 +393,35 @@ export class SimpleDb {
       );
     }
 
-    if (this.versionchangelistener) {
-      this.db.onversionchange = event => this.versionchangelistener!(event);
-    }
+    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 }
+    );
 
     return this.db;
   }
 
-  setVersionChangeListener(
-    versionChangeListener: (event: IDBVersionChangeEvent) => void
+  setDatabaseDeletedListener(
+    databaseDeletedListener: DatabaseDeletedListener
   ): void {
-    this.versionchangelistener = versionChangeListener;
-    if (this.db) {
-      this.db.onversionchange = (event: IDBVersionChangeEvent) => {
-        return versionChangeListener(event);
-      };
+    if (this.databaseDeletedListener) {
+      throw new Error(
+        'setDatabaseDeletedListener() may only be called once, ' +
+          'and it has already been called'
+      );
     }
+    this.databaseDeletedListener = databaseDeletedListener;
   }
 
   async runTransaction<T>(

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

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