Merge remote-tracking branch 'origin/master' into FixCacheEmptyResultBug

Author: dconeybe

Firestore/CHANGELOG.md

@@ -1,4 +1,7 @@
 # Unreleased
+- [fixed] Fix FAILED_PRECONDITION when writing to a deleted document in a
+  transaction (#10431).
+- [fixed] Fixed data race in credentials provider (#10393).
 - [fixed] Fix Firestore failing to raise initial snapshot from empty local cache
   result (#10437).
 

Firestore/Example/Tests/Integration/FSTTransactionTests.mm

@@ -104,6 +104,15 @@ - (void)assertSnapshot:(FIRDocumentSnapshot *)snapshot
   [transaction getDocument:doc error:&error];
 };
 
+typedef NS_ENUM(NSUInteger, FIRFromDocumentType) {
+  // The operation will be performed on a document that exists.
+  FIRFromDocumentTypeExisting,
+  // The operation will be performed on a document that has never existed.
+  FIRFromDocumentTypeNonExistent,
+  // The operation will be performed on a document that existed, but was deleted.
+  FIRFromDocumentTypeDeleted,
+};
+
 /**
  * Used for testing that all possible combinations of executing transactions result in the desired
  * document value or error.
@@ -117,14 +126,15 @@ - (void)assertSnapshot:(FIRDocumentSnapshot *)snapshot
 @interface FSTTransactionTester : NSObject
 - (FSTTransactionTester *)withExistingDoc;
 - (FSTTransactionTester *)withNonexistentDoc;
+- (FSTTransactionTester *)withDeletedDoc;
 - (FSTTransactionTester *)runWithStages:(NSArray<TransactionStage> *)stages;
 - (void)expectDoc:(NSObject *)expected;
 - (void)expectNoDoc;
 - (void)expectError:(FIRFirestoreErrorCode)expected;
 
 @property(atomic, strong, readwrite) NSArray<TransactionStage> *stages;
 @property(atomic, strong, readwrite) FIRDocumentReference *docRef;
-@property(atomic, assign, readwrite) BOOL fromExistingDoc;
+@property(atomic, assign, readwrite) FIRFromDocumentType fromDocumentType;
 @end
 
 @implementation FSTTransactionTester {
@@ -137,19 +147,25 @@ - (instancetype)initWithDb:(FIRFirestore *)db testCase:(FSTTransactionTests *)te
   if (self) {
     _db = db;
     _stages = [NSArray array];
+    _fromDocumentType = FIRFromDocumentTypeNonExistent;
     _testCase = testCase;
     _testExpectations = [NSMutableArray array];
   }
   return self;
 }
 
 - (FSTTransactionTester *)withExistingDoc {
-  self.fromExistingDoc = YES;
+  self.fromDocumentType = FIRFromDocumentTypeExisting;
   return self;
 }
 
 - (FSTTransactionTester *)withNonexistentDoc {
-  self.fromExistingDoc = NO;
+  self.fromDocumentType = FIRFromDocumentTypeNonExistent;
+  return self;
+}
+
+- (FSTTransactionTester *)withDeletedDoc {
+  self.fromDocumentType = FIRFromDocumentTypeDeleted;
   return self;
 }
 
@@ -195,10 +211,30 @@ - (void)expectError:(FIRFirestoreErrorCode)expected {
 
 - (void)prepareDoc {
   self.docRef = [[_db collectionWithPath:@"nonexistent"] documentWithAutoID];
-  if (_fromExistingDoc) {
-    NSError *setError = [self writeDocumentRef:self.docRef data:@{@"foo" : @"bar"}];
-    NSString *message = [NSString stringWithFormat:@"Failed set at %@", [self stageNames]];
-    [_testCase assertNilError:setError message:message];
+  switch (_fromDocumentType) {
+    case FIRFromDocumentTypeExisting: {
+      NSError *setError = [self writeDocumentRef:self.docRef data:@{@"foo" : @"bar"}];
+      NSString *message = [NSString stringWithFormat:@"Failed set at %@", [self stageNames]];
+      [_testCase assertNilError:setError message:message];
+      break;
+    }
+    case FIRFromDocumentTypeNonExistent: {
+      // Nothing to do; document does not exist.
+      break;
+    }
+    case FIRFromDocumentTypeDeleted: {
+      {
+        NSError *setError = [self writeDocumentRef:self.docRef data:@{@"foo" : @"bar"}];
+        NSString *message = [NSString stringWithFormat:@"Failed set at %@", [self stageNames]];
+        [_testCase assertNilError:setError message:message];
+      }
+      {
+        NSError *deleteError = [self deleteDocumentRef:self.docRef];
+        NSString *message = [NSString stringWithFormat:@"Failed delete at %@", [self stageNames]];
+        [_testCase assertNilError:deleteError message:message];
+      }
+      break;
+    }
   }
 }
 
@@ -215,6 +251,17 @@ - (NSError *)writeDocumentRef:(FIRDocumentReference *)ref
   return errorResult;
 }
 
+- (NSError *)deleteDocumentRef:(FIRDocumentReference *)ref {
+  __block NSError *errorResult;
+  XCTestExpectation *expectation = [_testCase expectationWithDescription:@"prepareDoc:delete"];
+  [ref deleteDocumentWithCompletion:^(NSError *error) {
+    errorResult = error;
+    [expectation fulfill];
+  }];
+  [_testCase awaitExpectations];
+  return errorResult;
+}
+
 - (void)runSuccessfulTransaction {
   XCTestExpectation *expectation =
       [_testCase expectationWithDescription:@"runSuccessfulTransaction"];
@@ -322,6 +369,32 @@ - (void)testRunsTransactionsAfterGettingNonexistentDoc {
   [[[tt withNonexistentDoc] runWithStages:@[ get, set1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
 }
 
+// This test is identical to the test above, except that withNonexistentDoc() is replaced by
+// withDeletedDoc(), to guard against regression of
+// https://github.com/firebase/firebase-js-sdk/issues/5871, where transactions would incorrectly
+// fail with FAILED_PRECONDITION when operations were performed on a deleted document (rather than
+// a non-existent document).
+- (void)testRunsTransactionsAfterGettingDeletedDoc {
+  FIRFirestore *firestore = [self firestore];
+  FSTTransactionTester *tt = [[FSTTransactionTester alloc] initWithDb:firestore testCase:self];
+
+  [[[tt withDeletedDoc] runWithStages:@[ get, delete1, delete1 ]] expectNoDoc];
+  [[[tt withDeletedDoc] runWithStages:@[ get, delete1, update2 ]]
+      expectError:FIRFirestoreErrorCodeInvalidArgument];
+  [[[tt withDeletedDoc] runWithStages:@[ get, delete1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
+
+  [[[tt withDeletedDoc] runWithStages:@[ get, update1, delete1 ]]
+      expectError:FIRFirestoreErrorCodeInvalidArgument];
+  [[[tt withDeletedDoc] runWithStages:@[ get, update1, update2 ]]
+      expectError:FIRFirestoreErrorCodeInvalidArgument];
+  [[[tt withDeletedDoc] runWithStages:@[ get, update1, set2 ]]
+      expectError:FIRFirestoreErrorCodeInvalidArgument];
+
+  [[[tt withDeletedDoc] runWithStages:@[ get, set1, delete1 ]] expectNoDoc];
+  [[[tt withDeletedDoc] runWithStages:@[ get, set1, update2 ]] expectDoc:@{@"foo" : @"bar2"}];
+  [[[tt withDeletedDoc] runWithStages:@[ get, set1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
+}
+
 - (void)testRunsTransactionOnExistingDoc {
   FIRFirestore *firestore = [self firestore];
   FSTTransactionTester *tt = [[FSTTransactionTester alloc] initWithDb:firestore testCase:self];
@@ -361,6 +434,27 @@ - (void)testRunsTransactionsOnNonexistentDoc {
   [[[tt withNonexistentDoc] runWithStages:@[ set1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
 }
 
+- (void)testRunsTransactionsOnDeletedDoc {
+  FIRFirestore *firestore = [self firestore];
+  FSTTransactionTester *tt = [[FSTTransactionTester alloc] initWithDb:firestore testCase:self];
+
+  [[[tt withDeletedDoc] runWithStages:@[ delete1, delete1 ]] expectNoDoc];
+  [[[tt withDeletedDoc] runWithStages:@[ delete1, update2 ]]
+      expectError:FIRFirestoreErrorCodeInvalidArgument];
+  [[[tt withDeletedDoc] runWithStages:@[ delete1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
+
+  [[[tt withDeletedDoc] runWithStages:@[ update1, delete1 ]]
+      expectError:FIRFirestoreErrorCodeNotFound];
+  [[[tt withDeletedDoc] runWithStages:@[ update1, update2 ]]
+      expectError:FIRFirestoreErrorCodeNotFound];
+  [[[tt withDeletedDoc] runWithStages:@[ update1, set2 ]]
+      expectError:FIRFirestoreErrorCodeNotFound];
+
+  [[[tt withDeletedDoc] runWithStages:@[ set1, delete1 ]] expectNoDoc];
+  [[[tt withDeletedDoc] runWithStages:@[ set1, update2 ]] expectDoc:@{@"foo" : @"bar2"}];
+  [[[tt withDeletedDoc] runWithStages:@[ set1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
+}
+
 - (void)testSetDocumentWithMerge {
   FIRFirestore *firestore = [self firestore];
   FIRDocumentReference *doc = [[firestore collectionWithPath:@"towns"] documentWithAutoID];
@@ -653,6 +747,56 @@ - (void)testDoesNotRetryOnPermanentError {
   [self awaitExpectations];
 }
 
+- (void)testRetryOnAlreadyExistsError {
+  FIRFirestore *firestore = [self firestore];
+  FIRDocumentReference *doc1 = [[firestore collectionWithPath:@"counters"] documentWithAutoID];
+  auto transactionCallbackCallCount = std::make_shared<std::atomic_int>(0);
+
+  // Skip backoff delays.
+  [firestore workerQueue]->SkipDelaysForTimerId(TimerId::RetryTransaction);
+
+  XCTestExpectation *expectation = [self expectationWithDescription:@"transaction"];
+  [firestore
+      runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **error) {
+        int callbackNum = ++(*transactionCallbackCallCount);
+
+        FIRDocumentSnapshot *snapshot = [transaction getDocument:doc1 error:error];
+        XCTAssertNil(*error);
+
+        if (callbackNum == 1) {
+          XCTAssertFalse(snapshot.exists);
+          // Create the document outside of the transaction to cause the commit to fail with
+          // ALREADY_EXISTS.
+          dispatch_semaphore_t writeSemaphore = dispatch_semaphore_create(0);
+          [doc1 setData:@{@"foo1" : @"bar1"}
+              completion:^(NSError *) {
+                dispatch_semaphore_signal(writeSemaphore);
+              }];
+          // We can block on it, because transactions run on a background queue.
+          dispatch_semaphore_wait(writeSemaphore, DISPATCH_TIME_FOREVER);
+        } else if (callbackNum == 2) {
+          XCTAssertTrue(snapshot.exists);
+        } else {
+          XCTFail(@"unexpected callbackNum: %@", @(callbackNum));
+        }
+
+        [transaction setData:@{@"foo2" : @"bar2"} forDocument:doc1];
+
+        return nil;
+      }
+      completion:^(id, NSError *_Nullable error) {
+        [expectation fulfill];
+        XCTAssertNil(error);
+      }];
+  [self awaitExpectations];
+
+  XCTAssertEqual(transactionCallbackCallCount->load(), 2);
+  FIRDocumentSnapshot *snapshot = [self readDocumentForRef:doc1];
+  XCTAssertNotNil(snapshot);
+  XCTAssertTrue(snapshot.exists);
+  XCTAssertEqualObjects(snapshot.data, (@{@"foo2" : @"bar2"}));
+}
+
 - (void)testMakesDefaultMaxAttempts {
   FIRFirestore *firestore = [self firestore];
   FIRDocumentReference *doc1 = [[firestore collectionWithPath:@"counters"] documentWithAutoID];

Firestore/core/src/core/transaction.cc

@@ -131,7 +131,11 @@ void Transaction::WriteMutations(std::vector<Mutation>&& mutations) {
 Precondition Transaction::CreatePrecondition(const DocumentKey& key) {
   absl::optional<SnapshotVersion> version = GetVersion(key);
   if (written_docs_.count(key) == 0 && version.has_value()) {
-    return Precondition::UpdateTime(version.value());
+    if (version.value() == SnapshotVersion::None()) {
+      return Precondition::Exists(false);
+    } else {
+      return Precondition::UpdateTime(version.value());
+    }
   } else {
     return Precondition::None();
   }

Firestore/core/src/core/transaction_runner.cc

@@ -33,10 +33,11 @@ using util::TimerId;
 
 bool IsRetryableTransactionError(const util::Status& error) {
   // In transactions, the backend will fail outdated reads with
-  // FAILED_PRECONDITION and non-matching document versions with ABORTED. These
+  // FAILED_PRECONDITION, non-matching document versions with ABORTED, and
+  // attempts to create already-existing document with ALREADY_EXISTS. These
   // errors should be retried.
   Error code = error.code();
-  return code == Error::kErrorAborted ||
+  return code == Error::kErrorAborted || code == Error::kErrorAlreadyExists ||
          code == Error::kErrorFailedPrecondition ||
          !remote::Datastore::IsPermanentError(error);
 }

Firestore/core/src/credentials/credentials_provider.h

@@ -17,6 +17,7 @@
 #ifndef FIRESTORE_CORE_SRC_CREDENTIALS_CREDENTIALS_PROVIDER_H_
 #define FIRESTORE_CORE_SRC_CREDENTIALS_CREDENTIALS_PROVIDER_H_
 
+#include <atomic>
 #include <functional>
 #include <string>
 
@@ -46,6 +47,7 @@ template <class TokenType, class ValueType>
 class CredentialsProvider {
  public:
   CredentialsProvider() : change_listener_(nullptr) {
+    force_refresh_ = false;
   }
 
   virtual ~CredentialsProvider() = default;
@@ -81,7 +83,7 @@ class CredentialsProvider {
    */
   CredentialChangeListener<ValueType> change_listener_;
 
-  bool force_refresh_ = false;
+  std::atomic<bool> force_refresh_;
 };
 
 }  // namespace credentials

Firestore/core/test/unit/credentials/firebase_auth_credentials_provider_test.mm

@@ -153,6 +153,28 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
   });
 }
 
+// Catch race conditions in FirebaseAuthCredentialsProvider::GetToken() when
+// run under thread sanitizer.
+// See https://github.com/firebase/firebase-ios-sdk/issues/10393
+TEST(FirebaseAuthCredentialsProviderTest, GetTokenCalledByAnotherThread) {
+  FIRApp* app = testutil::AppForUnitTesting();
+  FSTAuthFake* auth = [[FSTAuthFake alloc] initWithToken:@"token for fake uid"
+                                                     uid:@"fake uid"];
+  FirebaseAuthCredentialsProvider credentials_provider(app, auth);
+
+  std::thread thread1([&credentials_provider] {
+    credentials_provider.GetToken(
+        [](util::StatusOr<AuthToken> result) { EXPECT_TRUE(result.ok()); });
+  });
+  std::thread thread2([&credentials_provider] {
+    credentials_provider.GetToken(
+        [](util::StatusOr<AuthToken> result) { EXPECT_TRUE(result.ok()); });
+  });
+
+  thread1.join();
+  thread2.join();
+}
+
 }  // namespace credentials
 }  // namespace firestore
 }  // namespace firebase