[Sync] Propagate commit failure to SharingMessageBridge.

Add callback in sharing message bridge to handle commit failures.

In next patch enum for specific errors will be added.

Bug: 1047123
Change-Id: I60451e080da0f23763cfad1240cd7d8a517b9f9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2028769Reviewed-by: Alex Chau <alexchau@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#736929}

chrome/browser/sharing/sharing_fcm_sender.cc

@@ -297,6 +297,7 @@ void SharingFCMSender::OnMessageSentViaSync(
     case sync_pb::SharingMessageCommitError::UNAUTHENTICATED:
     case sync_pb::SharingMessageCommitError::PERMISSION_DENIED:
     case sync_pb::SharingMessageCommitError::SYNC_TURNED_OFF:
+    case sync_pb::SharingMessageCommitError::SYNC_ERROR:
       send_message_result = SharingSendMessageResult::kInternalError;
       break;
   }

chrome/browser/sharing/sharing_fcm_sender_unittest.cc

@@ -403,6 +403,8 @@ struct CommitErrorCodeTestData {
     {sync_pb::SharingMessageCommitError::PERMISSION_DENIED,
      SharingSendMessageResult::kInternalError},
     {sync_pb::SharingMessageCommitError::SYNC_TURNED_OFF,
+     SharingSendMessageResult::kInternalError},
+    {sync_pb::SharingMessageCommitError::SYNC_ERROR,
      SharingSendMessageResult::kInternalError}};
 
 class SharingFCMSenderCommitErrorCodeTest

chrome/browser/sharing/sharing_message_bridge_impl.cc

@@ -145,6 +145,19 @@ void SharingMessageBridgeImpl::OnCommitAttemptErrors(
   }
 }
 
+void SharingMessageBridgeImpl::OnCommitAttemptFailed() {
+  // Full commit failed means we need to drop all entities and report an error
+  // using callback.
+  sync_pb::SharingMessageCommitError sync_error_message;
+  sync_error_message.set_error_code(
+      sync_pb::SharingMessageCommitError::SYNC_ERROR);
+  for (auto& cth_and_callback : commit_callbacks_) {
+    change_processor()->UntrackEntityForClientTagHash(cth_and_callback.first);
+    ReplyToCallback(std::move(cth_and_callback.second), sync_error_message);
+  }
+  commit_callbacks_.clear();
+}
+
 void SharingMessageBridgeImpl::ApplyStopSyncChanges(
     std::unique_ptr<syncer::MetadataChangeList> metadata_change_list) {
   sync_pb::SharingMessageCommitError sync_disabled_error_message;

chrome/browser/sharing/sharing_message_bridge_impl.h

@@ -45,6 +45,7 @@ class SharingMessageBridgeImpl : public SharingMessageBridge,
   std::string GetStorageKey(const syncer::EntityData& entity_data) override;
   void OnCommitAttemptErrors(
       const syncer::FailedCommitResponseDataList& error_response_list) override;
+  void OnCommitAttemptFailed() override;
   void ApplyStopSyncChanges(std::unique_ptr<syncer::MetadataChangeList>
                                 metadata_change_list) override;
 

chrome/browser/sharing/sharing_message_bridge_impl_unittest.cc

@@ -190,4 +190,21 @@ TEST_F(SharingMessageBridgeTest, ShouldInvokeCallbackOnSyncStoppedEvent) {
       sync_pb::SharingMessageCommitError::SYNC_TURNED_OFF, 1);
 }
 
+TEST_F(SharingMessageBridgeTest, ShouldInvokeCallbackOnSyncCommitFailure) {
+  base::HistogramTester histogram_tester;
+  base::MockCallback<SharingMessageBridge::CommitFinishedCallback> callback;
+  bridge()->SendSharingMessage(CreateSpecifics("test_payload"), callback.Get());
+  ASSERT_EQ(bridge()->GetCallbacksCountForTesting(), 1u);
+
+  EXPECT_CALL(
+      callback,
+      Run(HasErrorCode(sync_pb::SharingMessageCommitError::SYNC_ERROR)));
+  bridge()->OnCommitAttemptFailed();
+
+  EXPECT_EQ(bridge()->GetCallbacksCountForTesting(), 0u);
+  histogram_tester.ExpectUniqueSample(
+      "Sync.SharingMessage.CommitResult",
+      sync_pb::SharingMessageCommitError::SYNC_ERROR, 1);
+}
+
 }  // namespace

components/sync/model/model_type_sync_bridge.cc

@@ -59,6 +59,10 @@ void ModelTypeSyncBridge::OnCommitAttemptErrors(
   // By default the bridge just ignores failed commit items.
 }
 
+void ModelTypeSyncBridge::OnCommitAttemptFailed() {
+  // By default ignore any failures.
+}
+
 size_t ModelTypeSyncBridge::EstimateSyncOverheadMemoryUsage() const {
   return 0U;
 }

components/sync/model/model_type_sync_bridge.h

@@ -168,6 +168,11 @@ class ModelTypeSyncBridge {
   virtual void OnCommitAttemptErrors(
       const syncer::FailedCommitResponseDataList& error_response_list);
 
+  // Called only when a commit failed due to server error. The commit will
+  // automatically be retried, so most implementations don't need to handle
+  // this.
+  virtual void OnCommitAttemptFailed();
+
   // Returns an estimate of memory usage attributed to sync (that is, excludes
   // the actual model). Because the resulting UMA metrics are often used to
   // compare with the non-USS equivalent implementations (SyncableService), it's

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -659,6 +659,11 @@ bool HasClearAllDirective(const sync_pb::ModelTypeState& model_type_state) {
       .has_version_watermark();
 }
 
+void ClientTagBasedModelTypeProcessor::OnCommitFailed() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  bridge_->OnCommitAttemptFailed();
+}
+
 void ClientTagBasedModelTypeProcessor::OnUpdateReceived(
     const sync_pb::ModelTypeState& model_type_state,
     UpdateResponseDataList updates) {

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -92,6 +92,7 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
       const sync_pb::ModelTypeState& type_state,
       const CommitResponseDataList& committed_response_list,
       const FailedCommitResponseDataList& error_response_list) override;
+  void OnCommitFailed() override;
   void OnUpdateReceived(const sync_pb::ModelTypeState& type_state,
                         UpdateResponseDataList updates) override;
 

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -129,6 +129,7 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
   int merge_call_count() const { return merge_call_count_; }
   int apply_call_count() const { return apply_call_count_; }
   int get_storage_key_call_count() const { return get_storage_key_call_count_; }
+  int commit_failures_count() const { return commit_failures_count_; }
 
   // FakeModelTypeSyncBridge overrides.
 
@@ -174,6 +175,8 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
     }
   }
 
+  void OnCommitAttemptFailed() override { commit_failures_count_++; }
+
   void SetOnCommitAttemptErrorsCallback(
       base::OnceCallback<void(const FailedCommitResponseDataList&)> callback) {
     on_commit_attempt_errors_callback_ = std::move(callback);
@@ -193,6 +196,7 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
   int merge_call_count_ = 0;
   int apply_call_count_ = 0;
   int get_storage_key_call_count_ = 0;
+  int commit_failures_count_ = 0;
 
   // Stores the data callback between GetData() and OnCommitDataLoaded().
   base::OnceClosure data_callback_;
@@ -2507,6 +2511,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
       /*committed_response_list=*/CommitResponseDataList(), failed_list);
 
   ASSERT_EQ(1u, actual_error_response_list.size());
+  EXPECT_EQ(0, bridge()->commit_failures_count());
   EXPECT_EQ(response_data.client_tag_hash,
             actual_error_response_list[0].client_tag_hash);
   EXPECT_EQ(response_data.response_type,
@@ -2533,6 +2538,14 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
       model_type_state(),
       /*committed_response_list=*/CommitResponseDataList(),
       /*rror_response_list=*/FailedCommitResponseDataList());
+  EXPECT_EQ(0, bridge()->commit_failures_count());
+}
+
+TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldPropagateFullCommitFailure) {
+  ASSERT_EQ(0, bridge()->commit_failures_count());
+
+  type_processor()->OnCommitFailed();
+  EXPECT_EQ(1, bridge()->commit_failures_count());
 }
 
 class CommitOnlyClientTagBasedModelTypeProcessorTest

components/sync/protocol/sharing_message_specifics.proto

@@ -68,6 +68,8 @@ message SharingMessageCommitError {
     PERMISSION_DENIED = 7;
     // Client-specific error code.
     SYNC_TURNED_OFF = 8;
+    // Error code for network errors or unparsable server response.
+    SYNC_ERROR = 9;
   }
 
   optional ErrorCode error_code = 1;

tools/metrics/histograms/enums.xml

@@ -62666,6 +62666,7 @@ would be helpful to identify which type is being sent.
   <int value="6" label="Unauthenticated"/>
   <int value="7" label="Permission denied"/>
   <int value="8" label="Sync turned off"/>
+  <int value="9" label="Sync error"/>
 </enum>
 
 <enum name="SyncSimpleConflictResolutions">