Move away support for net errors from FakeServer

A server shouldn't be capable of returning network errors; that belongs
some other layer that represents the network. This patch moves this
support to FakeServerHttpPostProvider which is arguably an improvement
already.

Bug: 774180
Change-Id: I1e5ebae89ff82f2a1ece78555b79c9732a71ee02
Reviewed-on: https://chromium-review.googlesource.com/c/1348030
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610395}

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

@@ -11,6 +11,7 @@
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "components/browser_sync/profile_sync_service.h"
+#include "components/sync/test/fake_server/fake_server_http_post_provider.h"
 #include "net/base/network_change_notifier.h"
 
 namespace {
@@ -73,7 +74,7 @@ IN_PROC_BROWSER_TEST_F(SyncExponentialBackoffTest, OfflineToOnline) {
   ASSERT_TRUE(AddFolder(0, 0, "folder1"));
   ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
 
-  GetFakeServer()->DisableNetwork();
+  fake_server::FakeServerHttpPostProvider::DisableNetwork();
 
   // Add a new item to trigger another sync cycle.
   ASSERT_TRUE(AddFolder(0, 0, "folder2"));
@@ -84,7 +85,7 @@ IN_PROC_BROWSER_TEST_F(SyncExponentialBackoffTest, OfflineToOnline) {
 
   // Trigger network change notification and remember time when it happened.
   // Ensure that scheduler runs canary job immediately.
-  GetFakeServer()->EnableNetwork();
+  fake_server::FakeServerHttpPostProvider::EnableNetwork();
   net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
       net::NetworkChangeNotifier::CONNECTION_ETHERNET);
 

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

@@ -21,6 +21,7 @@
 #include "components/sync/model/model_type_change_processor.h"
 #include "components/sync/model/model_type_controller_delegate.h"
 #include "components/sync/model_impl/client_tag_based_model_type_processor.h"
+#include "components/sync/test/fake_server/fake_server_http_post_provider.h"
 #include "net/base/network_change_notifier.h"
 
 using browser_sync::ChromeSyncClient;
@@ -347,14 +348,14 @@ IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, ConflictResolution) {
 
   // Disable network, write value on client 0 and wait for it to attempt
   // committing the value. This client now has conflicting change.
-  GetFakeServer()->DisableNetwork();
+  fake_server::FakeServerHttpPostProvider::DisableNetwork();
   model0->WriteItem(kKey1, kValue2);
   ASSERT_TRUE(SyncCycleFailedChecker(GetSyncService(0)).Wait());
 
   // Enable network, write different value on client 1 and wait for it to arrive
   // on server. Server now has value different from client 0 which will cause
   // conflict when client 0 performs GetUpdates.
-  GetFakeServer()->EnableNetwork();
+  fake_server::FakeServerHttpPostProvider::EnableNetwork();
   model1->WriteItem(kKey1, kValue3);
   model1->WriteItem(kKey2, kValue1);
   ASSERT_TRUE(ServerCountMatchStatusChecker(syncer::PREFERENCES, 2).Wait());

components/sync/test/fake_server/fake_server.cc

@@ -38,7 +38,6 @@ FakeServer::FakeServer()
       error_type_(sync_pb::SyncEnums::SUCCESS),
       alternate_triggered_errors_(false),
       request_counter_(0),
-      network_enabled_(true),
       weak_ptr_factory_(this) {
   base::ThreadRestrictions::SetIOAllowed(true);
   loopback_server_storage_ = std::make_unique<base::ScopedTempDir>();
@@ -160,32 +159,20 @@ bool AreWalletDataProgressMarkersEquivalent(
 }
 
 void FakeServer::HandleCommand(const std::string& request,
-                               const base::Closure& completion_closure,
-                               int* error_code,
-                               int* response_code,
+                               int* http_response_code,
                                std::string* response) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (!network_enabled_) {
-    *error_code = net::ERR_FAILED;
-    *response_code = net::ERR_FAILED;
-    *response = std::string();
-    completion_closure.Run();
-    return;
-  }
   request_counter_++;
 
   if (!authenticated_) {
-    *error_code = 0;
-    *response_code = net::HTTP_UNAUTHORIZED;
+    *http_response_code = net::HTTP_UNAUTHORIZED;
     *response = std::string();
-    completion_closure.Run();
     return;
   }
 
   sync_pb::ClientToServerResponse response_proto;
-  *response_code = 200;
-  *error_code = 0;
+  *http_response_code = net::HTTP_OK;
   if (error_type_ != sync_pb::SyncEnums::SUCCESS &&
       ShouldSendTriggeredError()) {
     response_proto.set_error_code(error_type_);
@@ -216,25 +203,23 @@ void FakeServer::HandleCommand(const std::string& request,
     // before handling those requests.
     std::unique_ptr<sync_pb::DataTypeProgressMarker> wallet_marker =
         RemoveWalletProgressMarkerIfExists(&message);
-    *response_code =
+    *http_response_code =
         SendToLoopbackServer(message.SerializeAsString(), response);
     if (wallet_marker != nullptr) {
       *message.mutable_get_updates()->add_from_progress_marker() =
           *wallet_marker;
-      if (*response_code == net::HTTP_OK) {
+      if (*http_response_code == net::HTTP_OK) {
         HandleWalletRequest(message, *wallet_marker, response);
       }
     }
-    if (*response_code == net::HTTP_OK) {
+    if (*http_response_code == net::HTTP_OK) {
       InjectClientCommand(response);
     }
-    completion_closure.Run();
     return;
   }
 
   response_proto.set_store_birthday(loopback_server_->GetStoreBirthday());
   *response = response_proto.SerializeAsString();
-  completion_closure.Run();
 }
 
 void FakeServer::HandleWalletRequest(
@@ -447,16 +432,6 @@ void FakeServer::OnCommit(const std::string& committer_id,
     observer.OnCommit(committer_id, committed_model_types);
 }
 
-void FakeServer::EnableNetwork() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  network_enabled_ = true;
-}
-
-void FakeServer::DisableNetwork() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  network_enabled_ = false;
-}
-
 void FakeServer::EnableStrongConsistencyWithConflictDetectionModel() {
   DCHECK(thread_checker_.CalledOnValidThread());
   loopback_server_->EnableStrongConsistencyWithConflictDetectionModel();

components/sync/test/fake_server/fake_server.h

@@ -12,7 +12,6 @@
 #include <string>
 #include <vector>
 
-#include "base/callback.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/observer_list.h"
 #include "base/threading/thread_checker.h"
@@ -55,15 +54,11 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
   FakeServer();
   ~FakeServer() override;
 
-  // Handles a /command POST (with the given |request|) to the server. Three
-  // output arguments, |error_code|, |response_code|, and |response|, are used
-  // to pass data back to the caller. The command has failed if the value
-  // pointed to by |error_code| is nonzero. |completion_closure| will be called
-  // immediately before return.
+  // Handles a /command POST (with the given |request|) to the server. Two
+  // output arguments, |http_response_code|, and |response|, are used
+  // to pass data back to the caller.
   void HandleCommand(const std::string& request,
-                     const base::Closure& completion_closure,
-                     int* error_code,
-                     int* response_code,
+                     int* http_response_code,
                      std::string* response);
 
   // Helpers for fetching the last Commit or GetUpdates messages, respectively.
@@ -170,13 +165,6 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
   // must be called if AddObserver was ever called with |observer|.
   void RemoveObserver(Observer* observer);
 
-  // Undoes the effects of DisableNetwork.
-  void EnableNetwork();
-
-  // Forces every request to fail in a way that simulates a network failure.
-  // This can be used to trigger exponential backoff in the client.
-  void DisableNetwork();
-
   // Enables strong consistency model (i.e. server detects conflicts).
   void EnableStrongConsistencyWithConflictDetectionModel();
 
@@ -241,10 +229,6 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
   // FakeServer's observers.
   base::ObserverList<Observer, true>::Unchecked observers_;
 
-  // When true, the server operates normally. When false, a failure is returned
-  // on every request. This is used to simulate a network failure on the client.
-  bool network_enabled_;
-
   // The last received client to server messages.
   sync_pb::ClientToServerMessage last_commit_message_;
   sync_pb::ClientToServerMessage last_getupdates_message_;

components/sync/test/fake_server/fake_server_http_post_provider.cc

@@ -4,6 +4,8 @@
 
 #include "components/sync/test/fake_server/fake_server_http_post_provider.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/time/time.h"
@@ -14,6 +16,19 @@ using syncer::HttpPostProviderInterface;
 
 namespace fake_server {
 
+// static
+bool FakeServerHttpPostProvider::network_enabled_ = true;
+
+namespace {
+
+void RunAndSignal(base::OnceClosure task,
+                  base::WaitableEvent* completion_event) {
+  std::move(task).Run();
+  completion_event->Signal();
+}
+
+}  // namespace
+
 FakeServerHttpPostProviderFactory::FakeServerHttpPostProviderFactory(
     const base::WeakPtr<FakeServer>& fake_server,
     scoped_refptr<base::SequencedTaskRunner> fake_server_task_runner)
@@ -66,26 +81,35 @@ void FakeServerHttpPostProvider::SetPostPayload(const char* content_type,
 
 bool FakeServerHttpPostProvider::MakeSynchronousPost(int* error_code,
                                                      int* response_code) {
+  if (!network_enabled_) {
+    response_.clear();
+    *error_code = net::ERR_INTERNET_DISCONNECTED;
+    *response_code = 0;
+    return false;
+  }
+
   // It is assumed that a POST is being made to /command.
-  int post_error_code = -1;
   int post_response_code = -1;
   std::string post_response;
 
   base::WaitableEvent post_complete(
       base::WaitableEvent::ResetPolicy::AUTOMATIC,
       base::WaitableEvent::InitialState::NOT_SIGNALED);
-  base::Closure signal_closure = base::Bind(&base::WaitableEvent::Signal,
-                                            base::Unretained(&post_complete));
 
   bool result = fake_server_task_runner_->PostTask(
       FROM_HERE,
-      base::BindOnce(&FakeServer::HandleCommand, fake_server_,
-                     base::ConstRef(request_content_),
-                     base::ConstRef(signal_closure), &post_error_code,
-                     &post_response_code, &post_response));
-
-  if (!result)
+      base::BindOnce(&RunAndSignal,
+                     base::BindOnce(&FakeServer::HandleCommand, fake_server_,
+                                    base::ConstRef(request_content_),
+                                    &post_response_code, &post_response),
+                     base::Unretained(&post_complete)));
+
+  if (!result) {
+    response_.clear();
+    *error_code = net::ERR_UNEXPECTED;
+    *response_code = 0;
     return false;
+  }
 
   // Note: This is a potential deadlock. Here we're on the sync thread, and
   // we're waiting for something to happen on the UI thread (where the
@@ -97,12 +121,12 @@ bool FakeServerHttpPostProvider::MakeSynchronousPost(int* error_code,
     *error_code = net::ERR_TIMED_OUT;
     return false;
   }
-  post_error_code_ = post_error_code;
-  post_response_code_ = post_response_code;
+
+  // Zero means success.
+  *error_code = 0;
+  *response_code = post_response_code;
   response_ = post_response;
 
-  *error_code = post_error_code_;
-  *response_code = post_response_code_;
   return *error_code == 0;
 }
 
@@ -121,4 +145,12 @@ const std::string FakeServerHttpPostProvider::GetResponseHeaderValue(
 
 void FakeServerHttpPostProvider::Abort() {}
 
+void FakeServerHttpPostProvider::DisableNetwork() {
+  network_enabled_ = false;
+}
+
+void FakeServerHttpPostProvider::EnableNetwork() {
+  network_enabled_ = true;
+}
+
 }  // namespace fake_server

components/sync/test/fake_server/fake_server_http_post_provider.h

@@ -41,12 +41,21 @@ class FakeServerHttpPostProvider
   const std::string GetResponseHeaderValue(
       const std::string& name) const override;
 
+  // Forces every request to fail in a way that simulates a network failure.
+  // This can be used to trigger exponential backoff in the client.
+  static void DisableNetwork();
+
+  // Undoes the effects of DisableNetwork.
+  static void EnableNetwork();
+
  protected:
   ~FakeServerHttpPostProvider() override;
 
  private:
   friend class base::RefCountedThreadSafe<FakeServerHttpPostProvider>;
 
+  static bool network_enabled_;
+
   // |fake_server_| should only be dereferenced on the same thread as
   // |fake_server_task_runner_| runs on.
   base::WeakPtr<FakeServer> fake_server_;
@@ -58,8 +67,6 @@ class FakeServerHttpPostProvider
   std::string request_content_;
   std::string request_content_type_;
   std::string extra_request_headers_;
-  int post_error_code_;
-  int post_response_code_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeServerHttpPostProvider);
 };