Make SDCH dictionaries persistent across browser restart.

net/url_request/sdch_dictionary_fetcher.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 "net/url_request/sdch_dictionary_fetcher.h"
 
 #include <stdint.h>
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/compiler_specific.h"
+

[mmenke, 2015/02/19 20:14:35]

Remove blank line

[Elly Fong-Jones, 2015/02/20 20:35:25]

Done.

 #include "base/profiler/scoped_tracker.h"
 #include "base/thread_task_runner_handle.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_log.h"
 #include "net/base/sdch_net_log_params.h"
+#include "net/http/http_response_headers.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_status.h"
 #include "net/url_request/url_request_throttler_manager.h"
 
 namespace net {
 
 namespace {
 
 const int kBufferSize = 4096;
 
 // Map the bytes_read result from a read attempt and a URLRequest's
 // status into a single net return value.
 int GetReadResult(int bytes_read, const URLRequest* request) {
   int rv = request->status().error();
   if (request->status().is_success() && bytes_read < 0) {
     rv = ERR_FAILED;
     request->net_log().AddEventWithNetErrorCode(
         NetLog::TYPE_SDCH_DICTIONARY_FETCH_IMPLIED_ERROR, rv);
   }
 
   if (rv == OK)
     rv = bytes_read;
 
   return rv;
 }
 
 }  // namespace
 
-SdchDictionaryFetcher::SdchDictionaryFetcher(
-    URLRequestContext* context,
-    const OnDictionaryFetchedCallback& callback)
+SdchDictionaryFetcher::SdchDictionaryFetcher(URLRequestContext* context)
     : next_state_(STATE_NONE),
       in_loop_(false),
       context_(context),
-      dictionary_fetched_callback_(callback),
       weak_factory_(this) {
   DCHECK(CalledOnValidThread());
   DCHECK(context);
 }
 
 SdchDictionaryFetcher::~SdchDictionaryFetcher() {
-  DCHECK(CalledOnValidThread());
 }
 
-void SdchDictionaryFetcher::Schedule(const GURL& dictionary_url) {
-  DCHECK(CalledOnValidThread());
+void SdchDictionaryFetcher::Schedule(
+    const GURL& dictionary_url,
+    const OnDictionaryFetchedCallback& callback) {
+  ScheduleInternal(dictionary_url, false, callback);
+}
 
-  // Avoid pushing duplicate copy onto queue. We may fetch this url again later
-  // and get a different dictionary, but there is no reason to have it in the
-  // queue twice at one time.
-  if ((!fetch_queue_.empty() && fetch_queue_.back() == dictionary_url) ||
-      attempted_load_.find(dictionary_url) != attempted_load_.end()) {
-    // TODO(rdsmith): log this error to the net log of the URLRequest
-    // initiating this fetch, once URLRequest will be passed here.
-    SdchManager::SdchErrorRecovery(
-        SDCH_DICTIONARY_PREVIOUSLY_SCHEDULED_TO_DOWNLOAD);
-    return;
-  }
-
-  attempted_load_.insert(dictionary_url);
-  fetch_queue_.push(dictionary_url);
-
-  // If the loop is already processing, it'll pick up the above in the
-  // normal course of events.
-  if (next_state_ != STATE_NONE)
-    return;
-
-  next_state_ = STATE_SEND_REQUEST;
-
-  // There are no callbacks to user code from the dictionary fetcher,
-  // and Schedule() is only called from user code, so this call to DoLoop()
-  // does not require an |if (in_loop_) return;| guard.
-  DoLoop(OK);
+void SdchDictionaryFetcher::ScheduleReload(
+    const GURL& dictionary_url,
+    const OnDictionaryFetchedCallback& callback) {
+  ScheduleInternal(dictionary_url, true, callback);
 }
 
 void SdchDictionaryFetcher::Cancel() {
   DCHECK(CalledOnValidThread());
 
   next_state_ = STATE_NONE;
+  current_request_.reset();
+  buffer_ = nullptr;
+  current_callback_.Reset();
 
-  while (!fetch_queue_.empty())
-    fetch_queue_.pop();
   attempted_load_.clear();
   weak_factory_.InvalidateWeakPtrs();
-  current_request_.reset(NULL);
-  buffer_ = NULL;
   dictionary_.clear();
 }
 
 void SdchDictionaryFetcher::OnResponseStarted(URLRequest* request) {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/423948 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "423948 SdchDictionaryFetcher::OnResponseStarted"));
 
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(request, current_request_.get());
   DCHECK_EQ(next_state_, STATE_SEND_REQUEST_COMPLETE);
   DCHECK(!in_loop_);
 
-  DoLoop(request->status().error());
+  // Confirm that the response isn't a stale read from the cache (as
+  // may happen in the reload case).  If the response was not retrieved over
+  // HTTP, it is presumed to be fresh.
+  HttpResponseHeaders* response_headers = request->response_headers();
+  int result = request->status().error();
+  if (result == OK && response_headers) {
+    ValidationType validation_type = response_headers->RequiresValidation(
+        request->response_info().request_time,
+        request->response_info().response_time, base::Time::Now());
+    // TODO(rdsmith): Maybe handle VALIDATION_ASYNCHRONOUS by queueing
+    // a non-reload request for the dictionary.
+    if (validation_type != VALIDATION_NONE) {
+      result = ERR_FAILED;
+    }
+  }
+
+  DoLoop(result);
 }
 
 void SdchDictionaryFetcher::OnReadCompleted(URLRequest* request,
                                             int bytes_read) {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/423948 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "423948 SdchDictionaryFetcher::OnReadCompleted"));
 
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(request, current_request_.get());
   DCHECK_EQ(next_state_, STATE_READ_BODY_COMPLETE);
   DCHECK(!in_loop_);
 
   DoLoop(GetReadResult(bytes_read, current_request_.get()));
 }
 
+SdchDictionaryFetcher::QueuedFetchInfo::QueuedFetchInfo()
+    : download_only_from_cache(false) {
+}
+
+SdchDictionaryFetcher::QueuedFetchInfo::QueuedFetchInfo(
+    const GURL& url,
+    bool download_only_from_cache,
+    const OnDictionaryFetchedCallback& callback)
+    : url(url),
+      download_only_from_cache(download_only_from_cache),
+      callback(callback) {
+}
+
+SdchDictionaryFetcher::QueuedFetchInfo::QueuedFetchInfo(
+    const QueuedFetchInfo& rhs) = default;
+
+SdchDictionaryFetcher::QueuedFetchInfo&
+SdchDictionaryFetcher::QueuedFetchInfo::operator=(
+    const QueuedFetchInfo& rhs) = default;
+
+SdchDictionaryFetcher::QueuedFetchInfo::~QueuedFetchInfo() {
+}
+
+void SdchDictionaryFetcher::ScheduleInternal(
+    const GURL& dictionary_url,
+    bool reload,
+    const OnDictionaryFetchedCallback& callback) {
+  DCHECK(CalledOnValidThread());
+
+  // Avoid pushing duplicate copy onto queue. We may fetch this url again later
+  // and get a different dictionary, but there is no reason to have it in the
+  // queue twice at one time.
+  if ((!fetch_queue_.empty() && fetch_queue_.back().url == dictionary_url) ||
+      attempted_load_.find(dictionary_url) != attempted_load_.end()) {
+    // TODO(rdsmith): Log this error to the net log.  In the case of a
+    // normal fetch, this can be through the URLRequest
+    // initiating this fetch (once the URLRequest is passed to the fetcher);
+    // in the case of a reload, it's more complicated.
+    SdchManager::SdchErrorRecovery(
+        SDCH_DICTIONARY_PREVIOUSLY_SCHEDULED_TO_DOWNLOAD);
+    return;
+  }
+
+  if (!reload)
+    attempted_load_.insert(dictionary_url);

[mmenke, 2015/02/19 20:14:35]

Not relevant to this CL, but I'm rather skeptical of |attempted_load_|...Once
it's true, it can stay true until Chrome is restarted, which seems rather weird.

[mmenke, 2015/02/19 20:14:35]

Also, if we're still loading something from the cache on startup, and then we
see a real request try to use it, calling Schedule, the metadata from the new
fetch will completely overwrite the same info loaded from the pref file, if the
cache load succeeds (use count, etc).  That seems kinda weird.

[mmenke, 2015/02/19 20:14:35]

This should have an explanatory comment.

[mmenke, 2015/02/19 20:14:35]

I don't think we have any tests for this.

[mmenke, 2015/02/19 21:44:58]

And just to clarify, this wouldn't happen if fetch_queue_ contained the current
fetch.

Also, if the order were reversed - we do the "reload" after the non-reload
request, the reload will always be cancelled in favor of the load, so we lose
the meta-data from the pref file in that case, too.

[Elly Fong-Jones, 2015/02/20 20:35:25]

Done.

+  fetch_queue_.push(QueuedFetchInfo(dictionary_url, reload, callback));
+
+  // If the loop is already processing, it'll pick up the above in the
+  // normal course of events.
+  if (next_state_ != STATE_NONE)
+    return;
+
+  next_state_ = STATE_SEND_REQUEST;
+
+  // There are no callbacks to user code from the dictionary fetcher,
+  // and Schedule() is only called from user code, so this call to DoLoop()
+  // does not require an |if (in_loop_) return;| guard.
+  DoLoop(OK);
+}
+
+void SdchDictionaryFetcher::ResetRequest() {
+  current_request_.reset();
+  buffer_ = nullptr;
+  current_callback_.Reset();
+  next_state_ = STATE_SEND_REQUEST;

[mmenke, 2015/02/19 20:14:35]

I thought we were going to remove the next_state_ thing from here?  Assuming
clearing the current request means we're ready to start the next one makes me
very nervous.

[Elly Fong-Jones, 2015/02/20 20:35:25]

I don't think so? We removed the DoLoop invocation from here, but resetting a
request *should* mean we're ready to start the next one, I think.

[mmenke, 2015/02/20 21:40:00]

I think this is a completely unexpected side effect in a method named
"ResetRequest" and makes the transition logic for each state much harder to
follow.

+  dictionary_.clear();
+  return;
+}
+
 int SdchDictionaryFetcher::DoLoop(int rv) {
   DCHECK(!in_loop_);
   base::AutoReset<bool> auto_reset_in_loop(&in_loop_, true);
 
   do {
     State state = next_state_;
     next_state_ = STATE_NONE;
     switch (state) {
       case STATE_SEND_REQUEST:
         rv = DoSendRequest(rv);
@@ skipping 25 matching lines @@
   // affect the next request.
 
   if (fetch_queue_.empty() || current_request_.get()) {
     next_state_ = STATE_NONE;
     return OK;
   }
 
   next_state_ = STATE_SEND_REQUEST_COMPLETE;
 
   current_request_ =
-      context_->CreateRequest(fetch_queue_.front(), IDLE, this, NULL);
-  current_request_->SetLoadFlags(LOAD_DO_NOT_SEND_COOKIES |
-                                 LOAD_DO_NOT_SAVE_COOKIES);
+      context_->CreateRequest(fetch_queue_.front().url, IDLE, this, NULL);
+  int load_flags = LOAD_DO_NOT_SEND_COOKIES | LOAD_DO_NOT_SAVE_COOKIES;
+  if (fetch_queue_.front().download_only_from_cache)
+    load_flags |= LOAD_ONLY_FROM_CACHE;
+  current_request_->SetLoadFlags(load_flags);
+
   buffer_ = new IOBuffer(kBufferSize);
+  current_callback_ = fetch_queue_.front().callback;
   fetch_queue_.pop();
 
   current_request_->Start();
   current_request_->net_log().AddEvent(NetLog::TYPE_SDCH_DICTIONARY_FETCH);
 
   return ERR_IO_PENDING;
 }
 
 int SdchDictionaryFetcher::DoSendRequestComplete(int rv) {
   DCHECK(CalledOnValidThread());
 
   // If there's been an error, abort the current request.
   if (rv != OK) {
     current_request_.reset();
     buffer_ = NULL;
     next_state_ = STATE_SEND_REQUEST;
 
     return OK;
   }
 
   next_state_ = STATE_READ_BODY;
   return OK;
 }
 
 int SdchDictionaryFetcher::DoReadBody(int rv) {
   DCHECK(CalledOnValidThread());
 
   // If there's been an error, abort the current request.
   if (rv != OK) {
-    current_request_.reset();
-    buffer_ = NULL;
-    next_state_ = STATE_SEND_REQUEST;
-
+    ResetRequest();
     return OK;
   }
 
   next_state_ = STATE_READ_BODY_COMPLETE;
   int bytes_read = 0;
   current_request_->Read(buffer_.get(), kBufferSize, &bytes_read);
   if (current_request_->status().is_io_pending())
     return ERR_IO_PENDING;
 
   return GetReadResult(bytes_read, current_request_.get());
@@ skipping 20 matching lines @@
   }
 
   // End of file; complete the request.
   next_state_ = STATE_REQUEST_COMPLETE;
   return OK;
 }
 
 int SdchDictionaryFetcher::DoCompleteRequest(int rv) {
   DCHECK(CalledOnValidThread());
 
-  // DoReadBodyComplete() only transitions to this state
-  // on success.
-  DCHECK_EQ(OK, rv);
+  // If the dictionary was successfully fetched, add it to the manager.
+  if (rv == OK) {
+    current_callback_.Run(dictionary_, current_request_->url(),
+                          current_request_->net_log());
+  }
 
-  dictionary_fetched_callback_.Run(dictionary_, current_request_->url(),
-                                   current_request_->net_log());
-  current_request_.reset();
-  buffer_ = NULL;
-  dictionary_.clear();
-
-  next_state_ = STATE_SEND_REQUEST;
-
+  ResetRequest();
   return OK;
 }
 
 }  // namespace net