Merge pull request #510 from firebase/mrschmidt-crash

Fixing race in FSTWriteStream

2 files changed, 16 insertions, 6 deletions

diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md
index f1cd832..0c5bcdc 100644
--- a/Firestore/CHANGELOG.md
+++ b/Firestore/CHANGELOG.md
@@ -3,6 +3,8 @@
 - [fixed] Fixed a crash when using path names with international characters
   with persistence enabled.
 
+- [fixed] Addressed race condition during the teardown of idle streams (#490).
+
 # v0.9.3
 - [changed] Improved performance loading documents matching a query.
 - [changed] Cleanly shut down idle write streams.
diff --git a/Firestore/Source/Remote/FSTStream.m b/Firestore/Source/Remote/FSTStream.m
index 614f611..ac51cb9 100644
--- a/Firestore/Source/Remote/FSTStream.m
+++ b/Firestore/Source/Remote/FSTStream.m
@@ -343,6 +343,9 @@ static const NSTimeInterval kIdleTimeout = 60.0;
 - (void)closeWithFinalState:(FSTStreamState)finalState error:(nullable NSError *)error {
   FSTAssert(finalState == FSTStreamStateError || error == nil,
             @"Can't provide an error when not in an error state.");
+  FSTAssert(self.delegate,
+            @"closeWithFinalState should only be called for a started stream that has an active "
+            @"delegate.");
 
   [self.workerDispatchQueue verifyIsCurrentQueue];
   [self cancelIdleCheck];
@@ -388,9 +391,6 @@ static const NSTimeInterval kIdleTimeout = 60.0;
   }
 
   // Clear the delegates to avoid any possible bleed through of events from GRPC.
-  FSTAssert(_delegate,
-            @"closeWithFinalState should only be called for a started stream that has an active "
-            @"delegate.");
   _delegate = nil;
 }
 
@@ -515,7 +515,11 @@ static const NSTimeInterval kIdleTimeout = 60.0;
  */
 - (void)handleStreamClose:(nullable NSError *)error {
   FSTLog(@"%@ %p close: %@", NSStringFromClass([self class]), (__bridge void *)self, error);
-  FSTAssert([self isStarted], @"Can't handle server close in non-started state.");
+
+  if (![self isStarted]) {   // The stream could have already been closed by the idle close timer.
+    FSTLog(@"%@ Ignoring server close for already closed stream.", NSStringFromClass([self class]));
+    return;
+  }
 
   // In theory the stream could close cleanly, however, in our current model we never expect this
   // to happen because if we stop a stream ourselves, this callback will never be called. To
@@ -615,7 +619,9 @@ static const NSTimeInterval kIdleTimeout = 60.0;
 }
 
 - (void)notifyStreamInterruptedWithError:(nullable NSError *)error {
-  [self.delegate watchStreamWasInterruptedWithError:error];
+  id<FSTWatchStreamDelegate> delegate = self.delegate;
+  self.delegate = nil;
+  [delegate watchStreamWasInterruptedWithError:error];
 }
 
 - (void)watchQuery:(FSTQueryData *)query {
@@ -701,7 +707,9 @@ static const NSTimeInterval kIdleTimeout = 60.0;
 }
 
 - (void)notifyStreamInterruptedWithError:(nullable NSError *)error {
-  [self.delegate writeStreamWasInterruptedWithError:error];
+  id<FSTWriteStreamDelegate> delegate = self.delegate;
+  self.delegate = nil;
+  [delegate writeStreamWasInterruptedWithError:error];
 }
 
 - (void)tearDown {