Fix for b/74749605: Cancel pending backoff operations when closing streams. (#564)

Author: mikelehen

packages/firestore/CHANGELOG.md

@@ -1,4 +1,10 @@
 # Unreleased
+- [fixed] Fixed a regression in the Firebase JS release 4.11.0 that could 
+  cause a crash if a user signs out while the client is offline, resulting in 
+  an error of "Attempted to schedule multiple operations with timer id 
+  listen_stream_connection_backoff".
+
+# 0.3.5
 - [changed] If the SDK's attempt to connect to the Cloud Firestore backend
   neither succeeds nor fails within 10 seconds, the SDK will consider itself
   "offline", causing get() calls to resolve with cached results, rather than

packages/firestore/src/remote/backoff.ts

@@ -87,9 +87,8 @@ export class ExponentialBackoff {
    * already, it will be canceled.
    */
   backoffAndRun(op: () => Promise<void>): void {
-    if (this.timerPromise !== null) {
-      this.timerPromise.cancel();
-    }
+    // Cancel any pending backoff operation.
+    this.cancel();
 
     // First schedule using the current base (which may be 0 and should be
     // honored as such).
@@ -118,6 +117,13 @@ export class ExponentialBackoff {
     }
   }
 
+  cancel(): void {
+    if (this.timerPromise !== null) {
+      this.timerPromise.cancel();
+      this.timerPromise = null;
+    }
+  }
+
   /** Returns a random value in the range [-currentBaseMs/2, currentBaseMs/2] */
   private jitterDelayMs(): number {
     return (Math.random() - 0.5) * this.currentBaseMs;

packages/firestore/src/remote/persistent_stream.ts

@@ -314,8 +314,13 @@ export abstract class PersistentStream<
       "Can't provide an error when not in an error state."
     );
 
+    // The stream will be closed so we don't need our idle close timer anymore.
     this.cancelIdleCheck();
 
+    // Ensure we don't leave a pending backoff operation queued (in case close()
+    // was called while we were waiting to reconnect).
+    this.backoff.cancel();
+
     if (finalState !== PersistentStreamState.Error) {
       // If this is an intentional close ensure we don't delay our next connection attempt.
       this.backoff.reset();
@@ -462,10 +467,16 @@ export abstract class PersistentStream<
 
     this.backoff.backoffAndRun(async () => {
       if (this.state === PersistentStreamState.Stopped) {
-        // Stream can be stopped while waiting for backoff to complete.
+        // We should have canceled the backoff timer when the stream was
+        // closed, but just in case we make this a no-op.
         return;
       }
 
+      assert(
+        this.state === PersistentStreamState.Backoff,
+        'Backoff should have been canceled if we left the Backoff state.'
+      );
+
       this.state = PersistentStreamState.Initial;
       this.start(listener);
       assert(this.isStarted(), 'PersistentStream should have started');

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

@@ -17,7 +17,7 @@
 import { expect } from 'chai';
 import { Query } from '../../../src/core/query';
 import { Code } from '../../../src/util/error';
-import { doc, path } from '../../util/helpers';
+import { doc, path, resumeTokenForSnapshot } from '../../util/helpers';
 
 import { describeSpec, specTest } from './describe_spec';
 import { spec } from './spec_builder';
@@ -83,4 +83,36 @@ describeSpec('Remote store:', [], () => {
         .expectEvents(query, { added: [doc1] })
     );
   });
+
+  // TODO(b/72313632): This test is web-only because the Android / iOS spec
+  // tests exclude backoff entirely.
+  specTest(
+    'Handles user changes while offline (b/74749605).',
+    ['no-android', 'no-ios'],
+    () => {
+      const query = Query.atPath(path('collection'));
+      const docA = doc('collection/a', 1000, { key: 'a' });
+      return (
+        spec()
+          .userListens(query)
+
+          // close the stream (this should trigger retry with backoff; but don't
+          // run it in an attempt to reproduce b/74749605).
+          .watchStreamCloses(Code.UNAVAILABLE, { runBackoffTimer: false })
+
+          // Because we didn't let the backoff timer run and restart the watch
+          // stream, there will be no active targets.
+          .expectActiveTargets()
+
+          // Change user (will shut down existing streams and start new ones).
+          .changeUser('abc')
+          // Our query should be sent to the new stream.
+          .expectActiveTargets({ query, resumeToken: '' })
+
+          // Close the (newly-created) stream as if it too failed (should trigger
+          // retry with backoff, potentially reproducing the crash in b/74749605).
+          .watchStreamCloses(Code.UNAVAILABLE)
+      );
+    }
+  );
 });

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

@@ -496,14 +496,22 @@ export class SpecBuilder {
     return this;
   }
 
-  watchStreamCloses(error: Code): SpecBuilder {
+  watchStreamCloses(
+    error: Code,
+    opts?: { runBackoffTimer: boolean }
+  ): SpecBuilder {
+    if (!opts) {
+      opts = { runBackoffTimer: true };
+    }
+
     this.nextStep();
     this.currentStep = {
       watchStreamClose: {
         error: {
           code: mapRpcCodeFromCode(error),
           message: 'Simulated Backend Error'
-        }
+        },
+        runBackoffTimer: opts.runBackoffTimer
       }
     };
     return this;

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

@@ -700,7 +700,7 @@ abstract class TestRunner {
       )
     );
     // The watch stream should re-open if we have active listeners.
-    if (!this.queryListeners.isEmpty()) {
+    if (spec.runBackoffTimer && !this.queryListeners.isEmpty()) {
       await this.queue.runDelayedOperationsEarly(
         TimerId.ListenStreamConnectionBackoff
       );
@@ -1167,6 +1167,7 @@ export type SpecSnapshotVersion = TestSnapshotVersion;
 
 export type SpecWatchStreamClose = {
   error: SpecError;
+  runBackoffTimer: boolean;
 };
 
 export type SpecWriteAck = {