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

Port of https://github.com/firebase/firebase-js-sdk/pull/564.

9 files changed, 150 insertions, 35 deletions

diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
index 1e16250..0c3d9a1 100644
--- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
+++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
@@ -296,6 +296,11 @@ static NSString *const kNoIOSTag = @"no-ios";
 - (void)doWatchStreamClose:(NSDictionary *)closeSpec {
   NSDictionary *errorSpec = closeSpec[@"error"];
   int code = ((NSNumber *)(errorSpec[@"code"])).intValue;
+
+  NSNumber *runBackoffTimer = closeSpec[@"runBackoffTimer"];
+  // TODO(b/72313632): Incorporate backoff in iOS Spec Tests.
+  FSTAssert(runBackoffTimer.boolValue, @"iOS Spec Tests don't support backoff.");
+
   [self.driver receiveWatchStreamError:code userInfo:errorSpec];
 }
 
diff --git a/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json b/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json
index ab42241..abd2cf4 100644
--- a/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json
+++ b/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json
@@ -337,7 +337,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "stateExpect": {
           "activeTargets": {
diff --git a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json
index fc9f295..db8324b 100644
--- a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json
+++ b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json
@@ -34,7 +34,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -42,7 +43,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "expect": [
           {
@@ -62,7 +64,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -70,7 +73,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       }
     ]
@@ -115,7 +119,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -123,7 +128,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -131,7 +137,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "expect": [
           {
@@ -151,7 +158,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -159,7 +167,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       }
     ]
@@ -199,7 +208,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -207,7 +217,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "expect": [
           {
@@ -240,7 +251,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -270,7 +282,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -278,7 +291,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "expect": [
           {
@@ -381,7 +395,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "stateExpect": {
           "activeTargets": {
@@ -401,7 +416,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -409,7 +425,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "expect": [
           {
@@ -595,7 +612,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "stateExpect": {
           "activeTargets": {
@@ -623,7 +641,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -631,7 +650,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -764,7 +784,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -772,7 +793,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -834,7 +856,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "stateExpect": {
           "activeTargets": {
diff --git a/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json b/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json
index 26bb520..6852c90 100644
--- a/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json
+++ b/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json
@@ -488,7 +488,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         }
       },
       {
@@ -543,5 +544,76 @@
         ]
       }
     ]
+  },
+  "Handles user changes while offline (b/74749605).": {
+    "describeName": "Remote store:",
+    "itName": "Handles user changes while offline (b/74749605).",
+    "tags": [
+      "no-android",
+      "no-ios"
+    ],
+    "config": {
+      "useGarbageCollection": true
+    },
+    "steps": [
+      {
+        "userListen": [
+          2,
+          {
+            "path": "collection",
+            "filters": [],
+            "orderBys": []
+          }
+        ],
+        "stateExpect": {
+          "activeTargets": {
+            "2": {
+              "query": {
+                "path": "collection",
+                "filters": [],
+                "orderBys": []
+              },
+              "resumeToken": ""
+            }
+          }
+        }
+      },
+      {
+        "watchStreamClose": {
+          "error": {
+            "code": 14,
+            "message": "Simulated Backend Error"
+          },
+          "runBackoffTimer": false
+        },
+        "stateExpect": {
+          "activeTargets": {}
+        }
+      },
+      {
+        "changeUser": "abc",
+        "stateExpect": {
+          "activeTargets": {
+            "2": {
+              "query": {
+                "path": "collection",
+                "filters": [],
+                "orderBys": []
+              },
+              "resumeToken": ""
+            }
+          }
+        }
+      },
+      {
+        "watchStreamClose": {
+          "error": {
+            "code": 14,
+            "message": "Simulated Backend Error"
+          },
+          "runBackoffTimer": true
+        }
+      }
+    ]
   }
 }
diff --git a/Firestore/Example/Tests/SpecTests/json/resume_token_spec_test.json b/Firestore/Example/Tests/SpecTests/json/resume_token_spec_test.json
index 25ea84a..f411d98 100644
--- a/Firestore/Example/Tests/SpecTests/json/resume_token_spec_test.json
+++ b/Firestore/Example/Tests/SpecTests/json/resume_token_spec_test.json
@@ -85,7 +85,8 @@
           "error": {
             "code": 14,
             "message": "Simulated Backend Error"
-          }
+          },
+          "runBackoffTimer": true
         },
         "stateExpect": {
           "activeTargets": {
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