Use modified URLRequest::Read() and delegate methods in /appcache/

content/browser/appcache/appcache_update_job.cc

 // Copyright (c) 2012 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 "content/browser/appcache/appcache_update_job.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
@@ skipping 166 matching lines @@
 }
 
 void AppCacheUpdateJob::URLFetcher::OnReceivedRedirect(
     net::URLRequest* request,
     const net::RedirectInfo& redirect_info,
     bool* defer_redirect) {
   DCHECK_EQ(request_.get(), request);
   // Redirect is not allowed by the update process.
   job_->MadeProgress();
   redirect_response_code_ = request->GetResponseCode();
-  request->Cancel();
+  int result = request->Cancel();
   result_ = REDIRECT_ERROR;
-  OnResponseCompleted();
+  OnResponseCompleted(result);

[michaeln, 2016/09/07 20:49:39]

I think i'd prefer to see net::ERR_ABORTED explicitly here, i just spent a
couple minutes verifying that OK was not an expected return value.

[maksims (do not use this acc), 2016/09/12 12:45:58]

I can use net::ERR_ABORTED here, but if there were any other errors set for some
reason, net::ERR_ABORTED would not be set as an error. I don't know how often it
might happen, but there is a probability of this. 

[michaeln, 2016/09/12 20:14:02]

Thnx

This class is explicitly choosing to abort the request for this non network
error condition. We're processing some kind of redirect response, so i don't
think the request can be in an error state, otherwise, we'd be getting an
OnResponseCompleted callback with the error instead. Even if it is, this class
is explicitly choosing to cancel for other reasons that have nothing todo with
any existing error.

 }
 
-void AppCacheUpdateJob::URLFetcher::OnResponseStarted(
-    net::URLRequest *request) {
+void AppCacheUpdateJob::URLFetcher::OnResponseStarted(net::URLRequest* request,
+                                                      int net_error) {
   DCHECK_EQ(request_.get(), request);
+  DCHECK_NE(net::ERR_IO_PENDING, net_error);
+
   int response_code = -1;
-  if (request->status().is_success()) {
+  if (net_error == net::OK) {
     response_code = request->GetResponseCode();
     job_->MadeProgress();
   }
 
   if ((response_code / 100) != 2) {
     if (response_code > 0)
       result_ = SERVER_ERROR;
     else
       result_ = NETWORK_ERROR;
-    OnResponseCompleted();
+    OnResponseCompleted(net_error);
     return;
   }
 
   if (url_.SchemeIsCryptographic()) {
     // Do not cache content with cert errors.
     // Also, we willfully violate the HTML5 spec at this point in order
     // to support the appcaching of cross-origin HTTPS resources.
     // We've opted for a milder constraint and allow caching unless
     // the resource has a "no-store" header. A spec change has been
     // requested on the whatwg list.
     // See http://code.google.com/p/chromium/issues/detail?id=69594
     // TODO(michaeln): Consider doing this for cross-origin HTTP too.
     const net::HttpNetworkSession::Params* session_params =
         request->context()->GetNetworkSessionParams();
     bool ignore_cert_errors = session_params &&
                               session_params->ignore_certificate_errors;
     if ((net::IsCertStatusError(request->ssl_info().cert_status) &&
             !ignore_cert_errors) ||
         (url_.GetOrigin() != job_->manifest_url_.GetOrigin() &&
             request->response_headers()->
                 HasHeaderValue("cache-control", "no-store"))) {
       DCHECK_EQ(-1, redirect_response_code_);
-      request->Cancel();
+      int result = request->Cancel();
       result_ = SECURITY_ERROR;
-      OnResponseCompleted();
+      OnResponseCompleted(result);

[michaeln, 2016/09/07 20:49:39]

ditto preference for net::ERR_ABORTED

[maksims (do not use this acc), 2016/09/12 12:45:58]

Done.

       return;
     }
   }
 
   // Write response info to storage for URL fetches. Wait for async write
   // completion before reading any response data.
   if (fetch_type_ == URL_FETCH || fetch_type_ == MASTER_ENTRY_FETCH) {
     response_writer_.reset(job_->CreateResponseWriter());
     scoped_refptr<HttpResponseInfoIOBuffer> io_buffer(
         new HttpResponseInfoIOBuffer(
             new net::HttpResponseInfo(request->response_info())));
     response_writer_->WriteInfo(
         io_buffer.get(),
         base::Bind(&URLFetcher::OnWriteComplete, base::Unretained(this)));
   } else {
     ReadResponseData();
   }
 }
 
 void AppCacheUpdateJob::URLFetcher::OnReadCompleted(
     net::URLRequest* request, int bytes_read) {

[michaeln, 2016/09/07 20:49:39]

maybe DCHECK_NE(net::ERR_IO_PENDING, bytes_read) for more consistency with other
overrides of this method in other classes (we'd have to change the callsite too)

[maksims (do not use this acc), 2016/09/12 12:45:58]

Done.

   DCHECK_EQ(request_.get(), request);
   bool data_consumed = true;
-  if (request->status().is_success() && bytes_read > 0) {
+  if (bytes_read > 0) {
     job_->MadeProgress();
     data_consumed = ConsumeResponseData(bytes_read);
     if (data_consumed) {
       bytes_read = 0;
-      while (request->Read(buffer_.get(), kBufferSize, &bytes_read)) {
+      while (bytes_read > 0) {

[michaeln, 2016/09/07 20:49:39]

somethings off here, the body of the while loop when never be entered. did you
mean to delete line 261? or maybe just make this while (true) break more
explicitly on the two conditions

while (true) {
  bytes_read = requst->Read();
  if (bytes_read <= 0)
    break;
  data_consumed = ConsumeResponseData();
  if (!data_consumed)
    break;
}

[maksims (do not use this acc), 2016/09/12 12:45:58]

Yes, I've forgotten to remove that line. Thanks!

+        bytes_read = request->Read(buffer_.get(), kBufferSize);
         if (bytes_read > 0) {
           data_consumed = ConsumeResponseData(bytes_read);
           if (!data_consumed)
             break;  // wait for async data processing, then read more
-        } else {
-          break;
         }
       }
     }
   }
-  if (data_consumed && !request->status().is_io_pending()) {
+
+  if (data_consumed && bytes_read != net::ERR_IO_PENDING) {
     DCHECK_EQ(UPDATE_OK, result_);
-    OnResponseCompleted();
+    OnResponseCompleted(bytes_read);
   }
 }
 
 void AppCacheUpdateJob::URLFetcher::AddConditionalHeaders(
     const net::HttpResponseHeaders* headers) {
   DCHECK(request_);
   DCHECK(headers);
   net::HttpRequestHeaders extra_headers;
 
   // Add If-Modified-Since header if response info has Last-Modified header.
@@ skipping 12 matching lines @@
   if (!etag_value.empty()) {
     extra_headers.SetHeader(net::HttpRequestHeaders::kIfNoneMatch,
                             etag_value);
   }
   if (!extra_headers.IsEmpty())
     request_->SetExtraRequestHeaders(extra_headers);
 }
 
 void  AppCacheUpdateJob::URLFetcher::OnWriteComplete(int result) {
   if (result < 0) {
-    request_->Cancel();
+    int result = request_->Cancel();
     result_ = DISKCACHE_ERROR;
-    OnResponseCompleted();
+    OnResponseCompleted(result);

[michaeln, 2016/09/07 20:49:39]

ditto pref for net::ERR_ABORTED

[maksims (do not use this acc), 2016/09/12 12:45:58]

Done.

     return;
   }
   ReadResponseData();
 }
 
 void AppCacheUpdateJob::URLFetcher::ReadResponseData() {
   InternalUpdateState state = job_->internal_state_;
   if (state == CACHE_FAILURE || state == CANCELLED || state == COMPLETED)
     return;
-  int bytes_read = 0;
-  request_->Read(buffer_.get(), kBufferSize, &bytes_read);
+  int bytes_read = request_->Read(buffer_.get(), kBufferSize);
   OnReadCompleted(request_.get(), bytes_read);

[michaeln, 2016/09/07 20:49:39]

maybe avoid calling this if byte_read == IO_PENDING, it'd be more in keeping
wither other impls of this method to be able to make that assumption/assertion

[maksims (do not use this acc), 2016/09/12 12:45:58]

Yes, sure. You're right. But as I noticed not all of the other impls have this
kind of logic. They don't call OnReadCompleted by themselves! But good point.
I'll take it into consideration. Thanks!

 }
 
 // Returns false if response data is processed asynchronously, in which
 // case ReadResponseData will be invoked when it is safe to continue
 // reading more response data from the request.
 bool AppCacheUpdateJob::URLFetcher::ConsumeResponseData(int bytes_read) {
   DCHECK_GT(bytes_read, 0);
   switch (fetch_type_) {
     case MANIFEST_FETCH:
     case MANIFEST_REFETCH:
       manifest_data_.append(buffer_->data(), bytes_read);
       break;
     case URL_FETCH:
     case MASTER_ENTRY_FETCH:
       DCHECK(response_writer_.get());
       response_writer_->WriteData(
           buffer_.get(),
           bytes_read,
           base::Bind(&URLFetcher::OnWriteComplete, base::Unretained(this)));
       return false;  // wait for async write completion to continue reading
     default:
       NOTREACHED();
   }
   return true;
 }
 
-void AppCacheUpdateJob::URLFetcher::OnResponseCompleted() {
-  if (request_->status().is_success())
+void AppCacheUpdateJob::URLFetcher::OnResponseCompleted(int net_error) {
+  if (net_error == net::OK)
     job_->MadeProgress();
 
   // Retry for 503s where retry-after is 0.
-  if (request_->status().is_success() &&
-      request_->GetResponseCode() == 503 &&
+  if (net_error == net::OK && request_->GetResponseCode() == 503 &&
       MaybeRetryRequest()) {
     return;
   }
 
   switch (fetch_type_) {
     case MANIFEST_FETCH:
-      job_->HandleManifestFetchCompleted(this);
+      job_->HandleManifestFetchCompleted(this, net_error);
       break;
     case URL_FETCH:
-      job_->HandleUrlFetchCompleted(this);
+      job_->HandleUrlFetchCompleted(this, net_error);
       break;
     case MASTER_ENTRY_FETCH:
-      job_->HandleMasterEntryFetchCompleted(this);
+      job_->HandleMasterEntryFetchCompleted(this, net_error);
       break;
     case MANIFEST_REFETCH:
-      job_->HandleManifestRefetchCompleted(this);
+      job_->HandleManifestRefetchCompleted(this, net_error);
       break;
     default:
       NOTREACHED();
   }
 
   delete this;
 }
 
 bool AppCacheUpdateJob::URLFetcher::MaybeRetryRequest() {
   if (retry_503_attempts_ >= kMax503Retries ||
@@ skipping 192 matching lines @@
     return;
   }
 
   DCHECK(internal_state_ == REFETCH_MANIFEST);
   DCHECK(manifest_response_info_.get());
   manifest_fetcher_->set_existing_response_headers(
       manifest_response_info_->headers.get());
   manifest_fetcher_->Start();
 }
 
-
-void AppCacheUpdateJob::HandleManifestFetchCompleted(
-    URLFetcher* fetcher) {
+void AppCacheUpdateJob::HandleManifestFetchCompleted(URLFetcher* fetcher,
+                                                     int net_error) {
   DCHECK_EQ(internal_state_, FETCH_MANIFEST);
   DCHECK_EQ(manifest_fetcher_, fetcher);
+
   manifest_fetcher_ = NULL;
 
   net::URLRequest* request = fetcher->request();
   int response_code = -1;
   bool is_valid_response_code = false;
-  if (request->status().is_success()) {
+  if (net_error == net::OK) {
     response_code = request->GetResponseCode();
     is_valid_response_code = (response_code / 100 == 2);
 
     std::string mime_type;
     request->GetMimeType(&mime_type);
     manifest_has_valid_mime_type_ = (mime_type == "text/cache-manifest");
   }
 
   if (is_valid_response_code) {
     manifest_data_ = fetcher->manifest_data();
@@ skipping 107 matching lines @@
     LogConsoleMessageToAll(message);
   }
 
   group_->SetUpdateAppCacheStatus(AppCacheGroup::DOWNLOADING);
   NotifyAllAssociatedHosts(APPCACHE_DOWNLOADING_EVENT);
   FetchUrls();
   FetchMasterEntries();
   MaybeCompleteUpdate();  // if not done, continues when async fetches complete
 }
 
-void AppCacheUpdateJob::HandleUrlFetchCompleted(URLFetcher* fetcher) {
+void AppCacheUpdateJob::HandleUrlFetchCompleted(URLFetcher* fetcher,
+                                                int net_error) {
   DCHECK(internal_state_ == DOWNLOADING);
 
   net::URLRequest* request = fetcher->request();
   const GURL& url = request->original_url();
   pending_url_fetches_.erase(url);
   NotifyAllProgress(url);
   ++url_fetches_completed_;
 
-  int response_code = request->status().is_success()
-                          ? request->GetResponseCode()
-                          : fetcher->redirect_response_code();
+  int response_code = net_error == net::OK ? request->GetResponseCode()
+                                           : fetcher->redirect_response_code();
 
   AppCacheEntry& entry = url_file_list_.find(url)->second;
 
   if (response_code / 100 == 2) {
     // Associate storage with the new entry.
     DCHECK(fetcher->response_writer());
     entry.set_response_id(fetcher->response_writer()->response_id());
     entry.set_response_size(fetcher->response_writer()->amount_written());
     if (!inprogress_cache_->AddOrModifyEntry(url, entry))
       duplicate_response_ids_.push_back(entry.response_id());
 
     // TODO(michaeln): Check for <html manifest=xxx>
     // See http://code.google.com/p/chromium/issues/detail?id=97930
     // if (entry.IsMaster() && !(entry.IsExplicit() || fallback || intercept))
     //   if (!manifestAttribute) skip it
 
     // Foreign entries will be detected during cache selection.
     // Note: 6.9.4, step 17.9 possible optimization: if resource is HTML or XML
     // file whose root element is an html element with a manifest attribute
     // whose value doesn't match the manifest url of the application cache
     // being processed, mark the entry as being foreign.
   } else {
-    VLOG(1) << "Request status: " << request->status().status()
-            << " error: " << request->status().error()
+    VLOG(1) << "Request error: " << net_error
             << " response code: " << response_code;
     if (entry.IsExplicit() || entry.IsFallback() || entry.IsIntercept()) {
       if (response_code == 304 && fetcher->existing_entry().has_response_id()) {
         // Keep the existing response.
         entry.set_response_id(fetcher->existing_entry().response_id());
         entry.set_response_size(fetcher->existing_entry().response_size());
         inprogress_cache_->AddOrModifyEntry(url, entry);
       } else {
         const char kFormatString[] = "Resource fetch failed (%d) %s";
         std::string message = FormatUrlErrorMessage(
@@ skipping 41 matching lines @@
       inprogress_cache_->AddOrModifyEntry(url, entry);
     }
   }
 
   // Fetch another URL now that one request has completed.
   DCHECK(internal_state_ != CACHE_FAILURE);
   FetchUrls();
   MaybeCompleteUpdate();
 }
 
-void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(
-    URLFetcher* fetcher) {
+void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(URLFetcher* fetcher,
+                                                        int net_error) {
   DCHECK(internal_state_ == NO_UPDATE || internal_state_ == DOWNLOADING);
 
   // TODO(jennb): Handle downloads completing during cache failure when update
   // no longer fetches master entries directly. For now, we cancel all pending
   // master entry fetches when entering cache failure state so this will never
   // be called in CACHE_FAILURE state.
 
   net::URLRequest* request = fetcher->request();
   const GURL& url = request->original_url();
   master_entry_fetches_.erase(url);
   ++master_entries_completed_;
 
-  int response_code = request->status().is_success()
-      ? request->GetResponseCode() : -1;
+  int response_code = net_error == net::OK ? request->GetResponseCode() : -1;
 
   PendingMasters::iterator found = pending_master_entries_.find(url);
   DCHECK(found != pending_master_entries_.end());
   PendingHosts& hosts = found->second;
 
   // Section 6.9.4. No update case: step 7.3, else step 22.
   if (response_code / 100 == 2) {
     // Add fetched master entry to the appropriate cache.
     AppCache* cache = inprogress_cache_.get() ? inprogress_cache_.get()
                                               : group_->newest_complete_cache();
@@ skipping 61 matching lines @@
         return;
       }
     }
   }
 
   DCHECK(internal_state_ != CACHE_FAILURE);
   FetchMasterEntries();
   MaybeCompleteUpdate();
 }
 
-void AppCacheUpdateJob::HandleManifestRefetchCompleted(
-    URLFetcher* fetcher) {
+void AppCacheUpdateJob::HandleManifestRefetchCompleted(URLFetcher* fetcher,
+                                                       int net_error) {
   DCHECK(internal_state_ == REFETCH_MANIFEST);
   DCHECK(manifest_fetcher_ == fetcher);
   manifest_fetcher_ = NULL;
 
-  net::URLRequest* request = fetcher->request();
-  int response_code = request->status().is_success()
-      ? request->GetResponseCode() : -1;
+  int response_code =
+      net_error == net::OK ? fetcher->request()->GetResponseCode() : -1;
   if (response_code == 304 || manifest_data_ == fetcher->manifest_data()) {
     // Only need to store response in storage if manifest is not already
     // an entry in the cache.
     AppCacheEntry* entry = inprogress_cache_->GetEntry(manifest_url_);
     if (entry) {
       entry->add_types(AppCacheEntry::MANIFEST);
       StoreGroupAndCache();
     } else {
       manifest_response_writer_.reset(CreateResponseWriter());
       scoped_refptr<HttpResponseInfoIOBuffer> io_buffer(
           new HttpResponseInfoIOBuffer(manifest_response_info_.release()));
       manifest_response_writer_->WriteInfo(
           io_buffer.get(),
           base::Bind(&AppCacheUpdateJob::OnManifestInfoWriteComplete,
                      base::Unretained(this)));
     }
   } else {
-    VLOG(1) << "Request status: " << request->status().status()
-            << " error: " << request->status().error()
+    VLOG(1) << "Request error: " << net_error
             << " response code: " << response_code;
     ScheduleUpdateRetry(kRerunDelayMs);
     if (response_code == 200) {
       HandleCacheFailure(AppCacheErrorDetails("Manifest changed during update",
                                       APPCACHE_CHANGED_ERROR,
                                       GURL(),
                                       0,
                                       false /*is_cross_origin*/),
                          MANIFEST_ERROR,
                          GURL());
@@ skipping 756 matching lines @@
   // on this object after we've posted a task to delete ourselves.
   if (group_) {
     group_->SetUpdateAppCacheStatus(AppCacheGroup::IDLE);
     group_ = NULL;
   }
 
   base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
 }
 
 }  // namespace content