Make SDCH dictionaries persistent across browser restart.

net/url_request/sdch_dictionary_fetcher.cc

Index: net/url_request/sdch_dictionary_fetcher.cc
diff --git a/net/url_request/sdch_dictionary_fetcher.cc b/net/url_request/sdch_dictionary_fetcher.cc
index 3607bfd42a08531c55d05bc0075aa61e48261d0c..8374c3ed97972fa6f2807ef8ab1960a6a8e294b9 100644
--- a/net/url_request/sdch_dictionary_fetcher.cc
+++ b/net/url_request/sdch_dictionary_fetcher.cc
@@ -9,12 +9,14 @@
 #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"
@@ -43,64 +45,40 @@ int GetReadResult(int bytes_read, const URLRequest* request) {
 
 }  // 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());
-
-  // 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;
+void SdchDictionaryFetcher::Schedule(
+    const GURL& dictionary_url,
+    const OnDictionaryFetchedCallback& callback) {
+  ScheduleInternal(dictionary_url, false, callback);
+}
 
-  // 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();
 }
 
@@ -115,7 +93,23 @@ void SdchDictionaryFetcher::OnResponseStarted(URLRequest* request) {
   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,
@@ -133,6 +127,75 @@ void SdchDictionaryFetcher::OnReadCompleted(URLRequest* request,
   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);
@@ -178,10 +241,14 @@ int SdchDictionaryFetcher::DoSendRequest(int rv) {
   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();
@@ -211,10 +278,7 @@ int SdchDictionaryFetcher::DoReadBody(int rv) {
 
   // 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;
   }
 
@@ -255,18 +319,13 @@ int SdchDictionaryFetcher::DoReadBodyComplete(int rv) {
 int SdchDictionaryFetcher::DoCompleteRequest(int rv) {
   DCHECK(CalledOnValidThread());
 
-  // DoReadBodyComplete() only transitions to this state
-  // on success.
-  DCHECK_EQ(OK, rv);
-
-  dictionary_fetched_callback_.Run(dictionary_, current_request_->url(),
-                                   current_request_->net_log());
-  current_request_.reset();
-  buffer_ = NULL;
-  dictionary_.clear();
-
-  next_state_ = STATE_SEND_REQUEST;
+  // 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());
+  }
 
+  ResetRequest();
   return OK;
 }