From 5f49b2f3f9866e4db13d09857eb3b548239cc62e Mon Sep 17 00:00:00 2001
From: Michael Lehenbauer <mikelehen@gmail.com>
Date: Wed, 21 Mar 2018 14:59:49 -0700
Subject: Fix for b/74749605: Cancel pending backoff operations when closing
 streams. (#958)

Port of https://github.com/firebase/firebase-js-sdk/pull/564.
---
 Firestore/Source/Remote/FSTExponentialBackoff.h  |  3 +++
 Firestore/Source/Remote/FSTExponentialBackoff.mm | 12 +++++++++---
 Firestore/Source/Remote/FSTStream.mm             |  9 ++++++++-
 Firestore/Source/Util/FSTDispatchQueue.mm        |  7 ++-----
 4 files changed, 22 insertions(+), 9 deletions(-)

(limited to 'Firestore/Source')

diff --git a/Firestore/Source/Remote/FSTExponentialBackoff.h b/Firestore/Source/Remote/FSTExponentialBackoff.h
index 03558ee..3beafbf 100644
--- a/Firestore/Source/Remote/FSTExponentialBackoff.h
+++ b/Firestore/Source/Remote/FSTExponentialBackoff.h
@@ -76,6 +76,9 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (void)backoffAndRunBlock:(void (^)(void))block;
 
+/** Cancels any pending backoff block scheduled via backoffAndRunBlock:. */
+- (void)cancel;
+
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/Firestore/Source/Remote/FSTExponentialBackoff.mm b/Firestore/Source/Remote/FSTExponentialBackoff.mm
index dddf164..20b50a5 100644
--- a/Firestore/Source/Remote/FSTExponentialBackoff.mm
+++ b/Firestore/Source/Remote/FSTExponentialBackoff.mm
@@ -66,9 +66,8 @@ using firebase::firestore::util::SecureRandom;
 }
 
 - (void)backoffAndRunBlock:(void (^)(void))block {
-  if (self.timerCallback) {
-    [self.timerCallback cancel];
-  }
+  [self cancel];
+
   // First schedule the block using the current base (which may be 0 and should be honored as such).
   NSTimeInterval delayWithJitter = _currentBase + [self jitterDelay];
   if (_currentBase > 0) {
@@ -89,6 +88,13 @@ using firebase::firestore::util::SecureRandom;
   }
 }
 
+- (void)cancel {
+  if (self.timerCallback) {
+    [self.timerCallback cancel];
+    self.timerCallback = nil;
+  }
+}
+
 /** Returns a random value in the range [-currentBase/2, currentBase/2] */
 - (NSTimeInterval)jitterDelay {
   std::uniform_real_distribution<double> dist;
diff --git a/Firestore/Source/Remote/FSTStream.mm b/Firestore/Source/Remote/FSTStream.mm
index a5c36c8..f65230b 100644
--- a/Firestore/Source/Remote/FSTStream.mm
+++ b/Firestore/Source/Remote/FSTStream.mm
@@ -326,7 +326,8 @@ static const NSTimeInterval kIdleTimeout = 60.0;
 /** Resumes stream start after backing off. */
 - (void)resumeStartFromBackoffWithDelegate:(id)delegate {
   if (self.state == FSTStreamStateStopped) {
-    // Streams 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;
   }
 
@@ -367,8 +368,14 @@ static const NSTimeInterval kIdleTimeout = 60.0;
             @"Can't provide an error when not in an error state.");
 
   [self.workerDispatchQueue verifyIsCurrentQueue];
+
+  // The stream will be closed so we don't need our idle close timer anymore.
   [self cancelIdleCheck];
 
+  // Ensure we don't leave a pending backoff operation queued (in case close()
+  // was called while we were waiting to reconnect).
+  [self.backoff cancel];
+
   if (finalState != FSTStreamStateError) {
     // If this is an intentional close ensure we don't delay our next connection attempt.
     [self.backoff reset];
diff --git a/Firestore/Source/Util/FSTDispatchQueue.mm b/Firestore/Source/Util/FSTDispatchQueue.mm
index 52482df..15d6e7b 100644
--- a/Firestore/Source/Util/FSTDispatchQueue.mm
+++ b/Firestore/Source/Util/FSTDispatchQueue.mm
@@ -230,11 +230,8 @@ NS_ASSUME_NONNULL_BEGIN
                                      block:(void (^)(void))block {
   // While not necessarily harmful, we currently don't expect to have multiple callbacks with the
   // same timerID in the queue, so defensively reject them.
-  // TODO(b/74749605): If a user change happens while offline we can end up with multiple backoff
-  // callbacks in the dispatch queue. This is non-harmful so I'm just disabling the assert until we
-  // get that cleaned up.
-  // FSTAssert(![self containsDelayedCallbackWithTimerID:timerID],
-  //           @"Attempted to schedule multiple callbacks with id %ld", (unsigned long)timerID);
+  FSTAssert(![self containsDelayedCallbackWithTimerID:timerID],
+            @"Attempted to schedule multiple callbacks with id %ld", (unsigned long)timerID);
   FSTDelayedCallback *delayedCallback = [FSTDelayedCallback createAndScheduleWithQueue:self
                                                                                timerID:timerID
                                                                                  delay:delay
-- 
cgit v1.2.3