SyncerError: Make sure NetworkConnectionUnavailable() is used

SyncerErrors with type NETWORK_CONNECTION_UNAVAILABLE should only be
created through the NetworkConnectionUnavailable() factory function, to
make sure that the expected net_error_code is always set.

Bug: 842096
Change-Id: Ibad86d6f0be6a36306db8b7e0e0d28404321f841
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1560190
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Auto-Submit: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#649663}

components/sync/base/syncer_error.h

@@ -7,6 +7,8 @@
 
 #include <string>
 
+#include "base/logging.h"
+
 namespace syncer {
 
 // This class describes all the possible results of a sync cycle. It should be
@@ -48,7 +50,11 @@ class SyncerError {
   constexpr SyncerError() : value_(UNSET), net_error_code_(0) {}
 
   explicit constexpr SyncerError(Value value)
-      : value_(value), net_error_code_(0) {}
+      : value_(value), net_error_code_(0) {
+    // NETWORK_CONNECTION_UNAVAILABLE error must be created via the separate
+    // factory method NetworkConnectionUnavailable().
+    DCHECK_NE(value_, NETWORK_CONNECTION_UNAVAILABLE);
+  }
 
   static constexpr SyncerError NetworkConnectionUnavailable(
       int net_error_code) {

components/sync/engine_impl/DEPS

@@ -7,6 +7,7 @@ include_rules = [
   "+components/sync/protocol",
   "+components/sync/syncable",
   "+components/sync/test",
+  "+net",
   "+services/network/public",
   "+services/network/test",
   "+third_party/protobuf",

components/sync/engine_impl/backoff_delay_provider_unittest.cc

@@ -9,6 +9,7 @@
 #include "components/sync/base/syncer_error.h"
 #include "components/sync/engine/cycle/model_neutral_state.h"
 #include "components/sync/engine/polling_constants.h"
+#include "net/base/net_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::TimeDelta;
@@ -49,7 +50,7 @@ TEST_F(BackoffDelayProviderTest, GetInitialDelay) {
             delay->GetInitialDelay(state).InSeconds());
 
   state.last_download_updates_result =
-      SyncerError(SyncerError::NETWORK_CONNECTION_UNAVAILABLE);
+      SyncerError::NetworkConnectionUnavailable(net::ERR_FAILED);
   EXPECT_EQ(kInitialBackoffRetrySeconds,
             delay->GetInitialDelay(state).InSeconds());
 
@@ -82,7 +83,7 @@ TEST_F(BackoffDelayProviderTest, GetInitialDelay) {
             delay->GetInitialDelay(state).InSeconds());
 
   state.commit_result =
-      SyncerError(SyncerError::NETWORK_CONNECTION_UNAVAILABLE);
+      SyncerError::NetworkConnectionUnavailable(net::ERR_FAILED);
   EXPECT_EQ(kInitialBackoffRetrySeconds,
             delay->GetInitialDelay(state).InSeconds());
 

components/sync/engine_impl/cycle/test_util.cc

@@ -4,6 +4,8 @@
 
 #include "components/sync/engine_impl/cycle/test_util.h"
 
+#include "net/base/net_errors.h"
+
 namespace syncer {
 namespace test_util {
 
@@ -41,7 +43,7 @@ void SimulateConfigureConnectionFailure(
   cycle->mutable_status_controller()->set_last_get_key_result(
       SyncerError(SyncerError::SYNCER_OK));
   cycle->mutable_status_controller()->set_last_download_updates_result(
-      SyncerError(SyncerError::NETWORK_CONNECTION_UNAVAILABLE));
+      SyncerError::NetworkConnectionUnavailable(net::ERR_FAILED));
 }
 
 void SimulateNormalSuccess(ModelTypeSet requested_types,
@@ -75,7 +77,7 @@ void SimulateConnectionFailure(ModelTypeSet requested_types,
                                NudgeTracker* nudge_tracker,
                                SyncCycle* cycle) {
   cycle->mutable_status_controller()->set_last_download_updates_result(
-      SyncerError(SyncerError::NETWORK_CONNECTION_UNAVAILABLE));
+      SyncerError::NetworkConnectionUnavailable(net::ERR_FAILED));
 }
 
 void SimulatePollSuccess(ModelTypeSet requested_types, SyncCycle* cycle) {

components/sync/engine_impl/syncer_proto_util.cc

@@ -111,9 +111,10 @@ SyncerError ServerConnectionErrorAsSyncerError(
     case HttpResponse::IO_ERROR:
       return SyncerError(SyncerError::NETWORK_IO_ERROR);
     case HttpResponse::SYNC_SERVER_ERROR:
-      // FIXME what does this mean?
+      // This means the server returned a non-401 HTTP error.
       return SyncerError(SyncerError::SYNC_SERVER_ERROR);
     case HttpResponse::SYNC_AUTH_ERROR:
+      // This means the server returned an HTTP 401 (unauthorized) error.
       return SyncerError(SyncerError::SYNC_AUTH_ERROR);
     case HttpResponse::SERVER_CONNECTION_OK:
     case HttpResponse::NONE: