[Sync] Forward commit failures.

Forward commit failures for proxy and forwarding model type processors.

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

chrome/browser/sync/test/integration/single_client_sharing_message_sync_test.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <vector>
+
+#include "base/test/mock_callback.h"
+#include "chrome/browser/sharing/sharing_message_bridge.h"
+#include "chrome/browser/sharing/sharing_message_bridge_factory.h"
+#include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
+#include "chrome/browser/sync/test/integration/sync_test.h"
+
+namespace {
+
+using sync_pb::SharingMessageSpecifics;
+
+MATCHER_P(HasErrorCode, expected_error_code, "") {
+  return arg.error_code() == expected_error_code;
+}
+
+class NextCycleIterationChecker : public SingleClientStatusChangeChecker {
+ public:
+  explicit NextCycleIterationChecker(syncer::ProfileSyncService* service)
+      : SingleClientStatusChangeChecker(service) {
+    last_synced_time_ = SingleClientStatusChangeChecker::service()
+                            ->GetLastSyncedTimeForDebugging();
+  }
+
+  bool IsExitConditionSatisfied(std::ostream* os) override {
+    *os << "Waiting server side for next sync cycle.";
+
+    if (last_synced_time_ == service()->GetLastSyncedTimeForDebugging()) {
+      return false;
+    }
+
+    return true;
+  }
+
+ private:
+  base::Time last_synced_time_;
+};
+
+class SharingMessageEqualityChecker : public SingleClientStatusChangeChecker {
+ public:
+  SharingMessageEqualityChecker(
+      syncer::ProfileSyncService* service,
+      fake_server::FakeServer* fake_server,
+      std::vector<SharingMessageSpecifics> expected_specifics)
+      : SingleClientStatusChangeChecker(service),
+        fake_server_(fake_server),
+        expected_specifics_(std::move(expected_specifics)) {}
+
+  bool IsExitConditionSatisfied(std::ostream* os) override {
+    *os << "Waiting server side SHARING_MESSAGE to match expected.";
+    std::vector<sync_pb::SyncEntity> entities =
+        fake_server_->GetSyncEntitiesByModelType(syncer::SHARING_MESSAGE);
+
+    // |entities.size()| is only going to grow, if |entities.size()| ever
+    // becomes bigger then all hope is lost of passing, stop now.
+    EXPECT_GE(expected_specifics_.size(), entities.size());
+
+    if (expected_specifics_.size() > entities.size()) {
+      return false;
+    }
+
+    // Number of events on server matches expected, exit condition can be
+    // satisfied. Let's verify that content matches as well. It is safe to
+    // modify |expected_specifics_|.
+    for (const sync_pb::SyncEntity& entity : entities) {
+      const SharingMessageSpecifics& server_specifics =
+          entity.specifics().sharing_message();
+      // It is likely to have the same order, it is ok to use find here.
+      auto iter = std::find_if(
+          expected_specifics_.begin(), expected_specifics_.end(),
+          [&server_specifics](const SharingMessageSpecifics& specifics) {
+            return specifics.payload() == server_specifics.payload();
+          });
+      EXPECT_TRUE(iter != expected_specifics_.end());
+      if (iter == expected_specifics_.end())
+        return false;
+      expected_specifics_.erase(iter);
+    }
+
+    return true;
+  }
+
+ private:
+  fake_server::FakeServer* fake_server_;
+  std::vector<SharingMessageSpecifics> expected_specifics_;
+};
+
+class SingleClientSharingMessageSyncTest : public SyncTest {
+ public:
+  SingleClientSharingMessageSyncTest() : SyncTest(SINGLE_CLIENT) {
+    DisableVerifier();
+  }
+
+  bool WaitForSharingMessage(
+      std::vector<SharingMessageSpecifics> expected_specifics) {
+    return SharingMessageEqualityChecker(GetSyncService(0), GetFakeServer(),
+                                         std::move(expected_specifics))
+        .Wait();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(SingleClientSharingMessageSyncTest, ShouldSubmit) {
+  base::MockOnceCallback<void(const sync_pb::SharingMessageCommitError&)>
+      callback;
+  EXPECT_CALL(callback,
+              Run(HasErrorCode(sync_pb::SharingMessageCommitError::NONE)));
+
+  ASSERT_TRUE(SetupSync());
+  ASSERT_EQ(0u, GetFakeServer()
+                    ->GetSyncEntitiesByModelType(syncer::SHARING_MESSAGE)
+                    .size());
+
+  SharingMessageBridge* sharing_message_bridge =
+      SharingMessageBridgeFactory::GetForBrowserContext(GetProfile(0));
+  SharingMessageSpecifics specifics;
+  specifics.set_payload("payload");
+  sharing_message_bridge->SendSharingMessage(
+      std::make_unique<SharingMessageSpecifics>(specifics), callback.Get());
+
+  EXPECT_TRUE(WaitForSharingMessage({specifics}));
+}
+
+// ChromeOS does not support late signin after profile creation, so the test
+// below does not apply, at least in the current form.
+#if !defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_F(SingleClientSharingMessageSyncTest,
+                       ShouldSubmitInTransportMode) {
+  // We avoid calling SetupSync(), because we don't want to turn on full sync,
+  // only sign in such that the standalone transport starts.
+  ASSERT_TRUE(SetupClients());
+  ASSERT_TRUE(GetClient(0)->SignInPrimaryAccount());
+  ASSERT_TRUE(GetClient(0)->AwaitEngineInitialization());
+  ASSERT_TRUE(AwaitQuiescence());
+  ASSERT_FALSE(GetSyncService(0)->IsSyncFeatureActive())
+      << "Full sync should be disabled";
+  ASSERT_EQ(syncer::SyncService::TransportState::ACTIVE,
+            GetSyncService(0)->GetTransportState());
+  ASSERT_TRUE(
+      GetSyncService(0)->GetActiveDataTypes().Has(syncer::SHARING_MESSAGE));
+
+  base::MockOnceCallback<void(const sync_pb::SharingMessageCommitError&)>
+      callback;
+  EXPECT_CALL(callback,
+              Run(HasErrorCode(sync_pb::SharingMessageCommitError::NONE)));
+
+  SharingMessageBridge* sharing_message_bridge =
+      SharingMessageBridgeFactory::GetForBrowserContext(GetProfile(0));
+  SharingMessageSpecifics specifics;
+  specifics.set_payload("payload");
+  sharing_message_bridge->SendSharingMessage(
+      std::make_unique<SharingMessageSpecifics>(specifics), callback.Get());
+
+  EXPECT_TRUE(WaitForSharingMessage({specifics}));
+}
+#endif  // !defined(OS_CHROMEOS)
+
+IN_PROC_BROWSER_TEST_F(SingleClientSharingMessageSyncTest,
+                       ShouldPropagateCommitFailure) {
+  base::MockOnceCallback<void(const sync_pb::SharingMessageCommitError&)>
+      callback;
+  EXPECT_CALL(
+      callback,
+      Run(HasErrorCode(sync_pb::SharingMessageCommitError::SYNC_ERROR)));
+
+  ASSERT_TRUE(SetupSync());
+  GetFakeServer()->TriggerCommitError(sync_pb::SyncEnums::TRANSIENT_ERROR);
+
+  SharingMessageBridge* sharing_message_bridge =
+      SharingMessageBridgeFactory::GetForBrowserContext(GetProfile(0));
+  SharingMessageSpecifics specifics;
+  specifics.set_payload("payload");
+  sharing_message_bridge->SendSharingMessage(
+      std::make_unique<SharingMessageSpecifics>(specifics), callback.Get());
+
+  ASSERT_TRUE(NextCycleIterationChecker(GetSyncService(0)).Wait());
+}
+
+}  // namespace

chrome/test/BUILD.gn

@@ -6242,6 +6242,7 @@ if (!is_android && !is_fuchsia) {
       "../browser/sync/test/integration/single_client_secondary_account_sync_test.cc",
       "../browser/sync/test/integration/single_client_send_tab_to_self_sync_test.cc",
       "../browser/sync/test/integration/single_client_sessions_sync_test.cc",
+      "../browser/sync/test/integration/single_client_sharing_message_sync_test.cc",
       "../browser/sync/test/integration/single_client_standalone_transport_sync_test.cc",
       "../browser/sync/test/integration/single_client_themes_sync_test.cc",
       "../browser/sync/test/integration/single_client_typed_urls_sync_test.cc",

components/sync/engine/model_type_processor_proxy.cc

@@ -62,6 +62,12 @@ void ModelTypeProcessorProxy::OnCommitCompleted(
                      type_state, committed_response_list, error_response_list));
 }
 
+void ModelTypeProcessorProxy::OnCommitFailed() {
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&ModelTypeProcessor::OnCommitFailed, processor_));
+}
+
 void ModelTypeProcessorProxy::OnUpdateReceived(
     const sync_pb::ModelTypeState& type_state,
     UpdateResponseDataList updates) {

components/sync/engine/model_type_processor_proxy.h

@@ -29,6 +29,7 @@ class ModelTypeProcessorProxy : 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/nigori/forwarding_model_type_processor.cc

@@ -41,6 +41,10 @@ void ForwardingModelTypeProcessor::OnCommitCompleted(
                                 error_response_list);
 }
 
+void ForwardingModelTypeProcessor::OnCommitFailed() {
+  processor_->OnCommitFailed();
+}
+
 void ForwardingModelTypeProcessor::OnUpdateReceived(
     const sync_pb::ModelTypeState& type_state,
     UpdateResponseDataList updates) {

components/sync/nigori/forwarding_model_type_processor.h

@@ -30,6 +30,7 @@ class ForwardingModelTypeProcessor : 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;