Precache bytes cap should be based on total bytes fetched from network

components/precache/core/precache_fetcher.cc

 // Copyright 2013 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 "components/precache/core/precache_fetcher.h"
 
 #include <algorithm>
 #include <limits>
 #include <string>
 #include <utility>
@@ skipping 217 matching lines @@
   }
   network_url_fetcher_->Start();
 }
 
 void PrecacheFetcher::Fetcher::OnURLFetchDownloadProgress(
     const net::URLFetcher* source,
     int64_t current,
     int64_t total) {
   // If going over the per-resource download cap.
   if (fetch_stage_ == FetchStage::NETWORK &&
-      // |current| is guaranteed to be non-negative, so this cast is safe.
-      static_cast<size_t>(std::max(current, total)) > max_bytes_) {
+      source->GetTotalReceivedBytes() > max_bytes_) {

[twifkak, 2016/07/01 23:59:25]

Also, the proto says "This max applies only to new downloads; cached resources
are not capped." Is that still true? I don't have a strong opinion on what it
should be -- only that the documentation should agree.

[twifkak, 2016/07/01 23:59:25]

AFAICT this is only available after the request is finished, not during
download, so it won't let us cancel mid-stream:
https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=...

This is a problem since we're downloading from third-party servers and can't
trust that they won't send us a really big file.

[Raj, 2016/07/14 18:41:30]

Yes. You are right. This is updated only after request is finished. It does not
seem easy to implement this in net/ as well to return network bytes or
compressed size progressively.

[Raj, 2016/07/14 18:41:30]

The proto is correct. Only new downloads have FetchStage::NETWORK and
max_bytes_per_resource cap is applied only for those.

     VLOG(1) << "Cancelling " << url_ << ": (" << current << "/" << total
             << ") is over " << max_bytes_;
 
     // Cancel the download.
     network_url_fetcher_.reset();
 
     // Call the completion callback, to attempt the next download, or to trigger
     // cleanup in precache_delegate_->OnDone().
     response_bytes_ = network_response_bytes_ = current;
 
@@ skipping 17 matching lines @@
     // refresh the cache with minimal network load.
     LoadFromNetwork();
     return;
   }
 
   // If any of:
   // - The request was for a config or manifest.
   // - The resource was a cache hit without validators.
   // - The response came from the network.
   // Then Fetcher is done with this URL and can return control to the caller.
   response_bytes_ = source->GetReceivedResponseContentLength();

[Raj, 2016/07/14 18:41:30]

I checked that response_bytes_ here includes bytes from network (for 200 OK) or
bytes from cache (for 304 Not-Modified). Also for gzipped resources the
compressed size is returned. So our calculation is correct.

   network_response_bytes_ = source->GetTotalReceivedBytes();
   callback_.Run(*this);
 }
 
 // static
 void PrecacheFetcher::RecordCompletionStatistics(
     const PrecacheUnfinishedWork& unfinished_work,
     size_t remaining_manifest_urls_to_fetch,
     size_t remaining_resource_urls_to_fetch) {
   // These may be unset in tests.
@@ skipping 112 matching lines @@
                  base::Unretained(this)),
       false /* is_resource_request */, std::numeric_limits<int32_t>::max())));
 }
 
 void PrecacheFetcher::StartNextResourceFetch() {
   DCHECK(unfinished_work_->has_config_settings());
   while (!resource_urls_to_fetch_.empty() && pool_.IsAvailable()) {
     const size_t max_bytes =
         std::min(unfinished_work_->config_settings().max_bytes_per_resource(),
                  unfinished_work_->config_settings().max_bytes_total() -
-                     unfinished_work_->total_bytes());
+                     unfinished_work_->network_bytes());
     VLOG(3) << "Fetching " << resource_urls_to_fetch_.front();
     pool_.Add(base::WrapUnique(
         new Fetcher(request_context_.get(), resource_urls_to_fetch_.front(),
                     base::Bind(&PrecacheFetcher::OnResourceFetchComplete,
                                base::Unretained(this)),
                     true /* is_resource_request */, max_bytes)));
 
     resource_urls_to_fetch_.pop_front();
   }
 }
@@ skipping 19 matching lines @@
     size_t remaining_resource_urls_to_fetch) {
   RecordCompletionStatistics(*unfinished_work_,
                              remaining_manifest_urls_to_fetch,
                              remaining_resource_urls_to_fetch);
   precache_delegate_->OnDone();
 }
 
 void PrecacheFetcher::StartNextFetch() {
   DCHECK(unfinished_work_->has_config_settings());
   // If over the precache total size cap, then stop prefetching.
-  if (unfinished_work_->total_bytes() >
+  if (unfinished_work_->network_bytes() >

[twifkak, 2016/07/01 23:59:25]

Does this include the size of cache resources, too? It should, as our goal with
this cap is to keep from eating the whole cache (doing so incrementally isn't
much better). Per the proto, "this includes new downloads as well as cached
resources."

[Raj, 2016/07/14 18:41:30]

My code is wrong. It should not be changed. total_bytes() accounts for bytes in
the network or bytes in the cache, depending on http response 200 or 304.

       unfinished_work_->config_settings().max_bytes_total()) {
     size_t pending_manifests_in_pool = 0;
     size_t pending_resources_in_pool = 0;
     for (const auto& element_pair : pool_.elements()) {
       const Fetcher* fetcher = element_pair.first;
       if (fetcher->is_resource_request())
         pending_resources_in_pool++;
       else if (fetcher->url() != config_url_)
         pending_manifests_in_pool++;
     }
@@ skipping 106 matching lines @@
 
 void PrecacheFetcher::UpdateStats(int64_t response_bytes,
                                   int64_t network_response_bytes) {
   unfinished_work_->set_total_bytes(
       unfinished_work_->total_bytes() + response_bytes);
   unfinished_work_->set_network_bytes(
       unfinished_work_->network_bytes() + network_response_bytes);
 }
 
 }  // namespace precache