Update AttachmentServiceImpl to retry attachment uploads.

sync/internal_api/attachments/attachment_uploader_impl.cc

Index: sync/internal_api/attachments/attachment_uploader_impl.cc
diff --git a/sync/internal_api/attachments/attachment_uploader_impl.cc b/sync/internal_api/attachments/attachment_uploader_impl.cc
index 4d197eb6da7edb9d4114c7111c77b0eafe15aca3..99b22228c9830a6c7201763403a4d18426f163dd 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl.cc
@@ -133,20 +133,22 @@ const Attachment& AttachmentUploaderImpl::UploadState::GetAttachment() {
 void AttachmentUploaderImpl::UploadState::OnURLFetchComplete(
     const net::URLFetcher* source) {
   DCHECK(CalledOnValidThread());
-  UploadResult result = UPLOAD_UNSPECIFIED_ERROR;
+  UploadResult result = UPLOAD_TRANSIENT_ERROR;
   AttachmentId attachment_id = attachment_.GetId();
-  if (source->GetResponseCode() == net::HTTP_OK) {
+  const int response_code = source->GetResponseCode();
+  if (response_code == net::HTTP_OK) {
     result = UPLOAD_SUCCESS;
-  } else if (source->GetResponseCode() == net::HTTP_UNAUTHORIZED) {
-    // TODO(maniscalco): One possibility is that we received a 401 because our
-    // access token has expired.  We should probably fetch a new access token
-    // and retry this upload before giving up and reporting failure to our
-    // caller (bug 380437).
+  } else if (response_code == net::HTTP_UNAUTHORIZED) {
+    // Server tells us we've got a bad token so invalidate it.
     OAuth2TokenServiceRequest::InvalidateToken(
         token_service_provider_, account_id_, scopes_, access_token_);
-  } else {
-    // TODO(maniscalco): Once the protocol is better defined, deal with the
-    // various HTTP response codes we may encounter.
+    // Fail the request, but indicate that it may be successful if retried.
+    result = UPLOAD_TRANSIENT_ERROR;
+  } else if (response_code == net::HTTP_FORBIDDEN) {
+    // User is not allowed to use attachments.  Retrying won't help.
+    result = UPLOAD_UNSPECIFIED_ERROR;
+  } else if (response_code == net::URLFetcher::RESPONSE_CODE_INVALID) {
+    result = UPLOAD_TRANSIENT_ERROR;
   }

[pavely, 2014/09/09 19:42:23]

We discussed that all 4xx except "forbidden" are persistent errors. will you
make corresponding change in the future?

[pavely, 2014/09/09 19:47:42]

Sorry, I meant "unauthorized"

[maniscalco, 2014/09/09 21:15:43]

I think you mean all are transient except for forbidden (forbidden is returned
when the user is not on the whitelist for attachments).  Right?  My goal is to
return UPLOAD_TRANSIENT_ERROR for all 4XXs except forbidden.  I believe that's
what this code does, or did I misunderstand?

[maniscalco, 2014/09/09 21:15:43]

Acknowledged.

   ReportResult(result, attachment_id);
 }
@@ -160,6 +162,7 @@ void AttachmentUploaderImpl::UploadState::OnGetTokenSuccess(
   access_token_ = access_token;
   fetcher_.reset(
       net::URLFetcher::Create(upload_url_, net::URLFetcher::POST, this));
+  fetcher_->SetAutomaticallyRetryOn5xx(false);
   fetcher_->SetRequestContext(url_request_context_getter_.get());
   // TODO(maniscalco): Is there a better way?  Copying the attachment data into
   // a string feels wrong given how large attachments may be (several MBs).  If
@@ -184,7 +187,7 @@ void AttachmentUploaderImpl::UploadState::OnGetTokenFailure(
     const GoogleServiceAuthError& error) {
   DCHECK_EQ(access_token_request_.get(), request);
   access_token_request_.reset();
-  ReportResult(UPLOAD_UNSPECIFIED_ERROR, attachment_.GetId());
+  ReportResult(UPLOAD_TRANSIENT_ERROR, attachment_.GetId());

[pavely, 2014/09/09 19:42:23]

Could you put a TODO that errors should be treated differently here as well?
Let's discuss in person what the behavior should be.

[maniscalco, 2014/09/09 21:15:44]

Happy to chat more in person.  TODO added.

Were you thinking we should examine the cause of failing to get a token?  That
sometimes you don't want to retry here?

 }
 
 void AttachmentUploaderImpl::UploadState::GetToken() {