Update AttachmentServiceImpl to retry attachment uploads.

sync/internal_api/attachments/attachment_uploader_impl.cc

 // Copyright 2014 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 "sync/internal_api/public/attachments/attachment_uploader_impl.h"
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/threading/non_thread_safe.h"
 #include "google_apis/gaia/gaia_constants.h"
@@ skipping 115 matching lines @@
 }
 
 const Attachment& AttachmentUploaderImpl::UploadState::GetAttachment() {
   DCHECK(CalledOnValidThread());
   return attachment_;
 }
 
 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);
 }
 
 void AttachmentUploaderImpl::UploadState::OnGetTokenSuccess(
     const OAuth2TokenService::Request* request,
     const std::string& access_token,
     const base::Time& expiration_time) {
   DCHECK_EQ(access_token_request_.get(), request);
   access_token_request_.reset();
   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
   // we may end up switching from URLFetcher to URLRequest, this copy won't be
   // necessary.
   scoped_refptr<base::RefCountedMemory> memory = attachment_.GetData();
   const std::string upload_content(memory->front_as<char>(), memory->size());
   fetcher_->SetUploadData(kContentType, upload_content);
   const std::string auth_header("Authorization: Bearer " + access_token_);
   fetcher_->AddExtraRequestHeader(auth_header);
   fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES |
                          net::LOAD_DO_NOT_SEND_COOKIES |
                          net::LOAD_DISABLE_CACHE);
   // TODO(maniscalco): Set an appropriate headers (User-Agent, Content-type, and
   // Content-length) on the request and include the content's MD5,
   // AttachmentId's unique_id and the "sync birthday" (bug 371521).
   fetcher_->Start();
 }
 
 void AttachmentUploaderImpl::UploadState::OnGetTokenFailure(
     const OAuth2TokenService::Request* request,
     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() {
   access_token_request_ = OAuth2TokenServiceRequest::CreateAndStart(
       token_service_provider_, account_id_, scopes_, this);
 }
 
 void AttachmentUploaderImpl::UploadState::ReportResult(
     const UploadResult& result,
     const AttachmentId& attachment_id) {
@@ skipping 71 matching lines @@
   GURL::Replacements replacements;
   replacements.SetPathStr(path);
   return sync_service_url.ReplaceComponents(replacements);
 }
 
 void AttachmentUploaderImpl::DeleteUploadStateFor(const UniqueId& unique_id) {
   state_map_.erase(unique_id);
 }
 
 }  // namespace syncer