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..5806e659048817a446c03076cb2cca4eae38fbe0 100644
--- a/net/url_request/sdch_dictionary_fetcher.cc
+++ b/net/url_request/sdch_dictionary_fetcher.cc
@@ -15,6 +15,7 @@
 #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"
@@ -41,66 +42,116 @@ int GetReadResult(int bytes_read, const URLRequest* request) {
   return rv;
 }
 
+struct FetchInfo {
+  FetchInfo(const GURL& url,
+            bool cache_only,
+            const SdchDictionaryFetcher::OnDictionaryFetchedCallback& callback)
+      : url(url), cache_only(cache_only), callback(callback) {}
+  FetchInfo() {}
+  GURL url;

[mmenke, 2015/02/26 17:27:20]

blank line before url.

[Elly Fong-Jones, 2015/03/03 21:37:46]

Done.

+  bool cache_only;
+  SdchDictionaryFetcher::OnDictionaryFetchedCallback callback;
+};
+
+// A UniqueFetchQueue is used to queue outgoing requests, which are either cache
+// requests or network requests (which *may* still be served from cache).
+// The UniqueFetchQueue enforces that a dictionary can be loaded-from-cache and
+// loaded-from-network each at most once, and that if a dictionary has been
+// loaded-from-network, it cannot subsequently be loaded-from-cache. Practically
+// this means there are three valid patterns of requests for a URL u:
+//    1. load-from-cache(u)
+//    2. load-from-network(u)
+//    3. load-from-cache(u); load-from-network(u)
+// Any fetches that would violate these invariants are rejected by Push().
+class UniqueFetchQueue {
+ public:
+  UniqueFetchQueue();
+  ~UniqueFetchQueue();
+
+  bool Push(const FetchInfo& info);
+  bool Pop(FetchInfo* info);
+  bool IsEmpty() const;
+  void ForgetPriorFetches();

[Randy Smith (Not in Mondays), 2015/02/23 21:23:49]

nit, suggestion: Name change to Clear()?

[Elly Fong-Jones, 2015/03/03 21:37:45]

Done.

+ private:

[mmenke, 2015/02/26 17:27:20]

nit:  Blank line before private.

[Elly Fong-Jones, 2015/03/03 21:37:45]

Done.

+  std::list<FetchInfo> queue_;

[mmenke, 2015/02/26 17:27:20]

Any reason this is a list and not a queue?  Also, you're including <queue> and
not <list> in the header.

[Elly Fong-Jones, 2015/03/03 21:37:45]

Oops, it was meant to be a queue.

+  std::set<GURL> ever_cache_fetched_;

[mmenke, 2015/02/26 17:27:20]

Do we even need this?  We load a dictionary of URLs, don't we?  So we never end
up double-cache-fetching a URL, right?

[Elly Fong-Jones, 2015/03/03 21:37:45]

Done.

+  std::set<GURL> ever_fetched_;

[Randy Smith (Not in Mondays), 2015/02/23 21:23:49]

nit, suggestion: This is a bit wasteful of space; how about a
map:GURL->(bool,bool) or enum?

[mmenke, 2015/02/26 17:27:20]

+1.

[mmenke, 2015/02/26 17:27:20]

The fact that something that was just fetched from the cache was not
"ever_fetched" is not at all intuitive.  Should document that.  Should also
document that "fetched" means "fetch queued", rather than "fetch attempted" or
"fetched successfully".

[mmenke, 2015/02/26 17:27:20]

DISALLOW_COPY_AND_ASSIGN

[Elly Fong-Jones, 2015/03/03 21:37:45]

Done.

[Elly Fong-Jones, 2015/03/03 21:37:46]

Done.

[Elly Fong-Jones, 2015/03/03 21:37:48]

Done.

[Elly Fong-Jones, 2015/03/03 21:37:48]

Done.

+};
+
+UniqueFetchQueue::UniqueFetchQueue() {}
+UniqueFetchQueue::~UniqueFetchQueue() {}
+
+bool UniqueFetchQueue::Push(const FetchInfo& info) {
+  if (info.cache_only) {
+    if (ever_cache_fetched_.count(info.url) != 0 ||
+        ever_fetched_.count(info.url) != 0) {
+      return false;
+    }
+    ever_cache_fetched_.insert(info.url);
+  } else {
+    if (ever_fetched_.count(info.url) != 0) {

[mmenke, 2015/02/26 17:27:20]

optional:  Think it's a bit clearer if we do this first, before the cache only
check.

[Elly Fong-Jones, 2015/03/03 21:37:46]

Done.

+      return false;
+    }

[mmenke, 2015/02/26 17:27:20]

nit:  Don't use braces with single-line if's.

[Elly Fong-Jones, 2015/03/03 21:37:45]

Done.

+    ever_fetched_.insert(info.url);
+  }

[Randy Smith (Not in Mondays), 2015/02/23 21:23:49]

suggestion: I'd find this code easier to read if it cascaded failure (false)
returns, i.e.:

if (ever_fetched_.count(info.url) != 0) 
   return false;

if (info.cache_only && ever_fetched_.cache(info.url))
  return false;

// Insert into appropriate queue.

[Elly Fong-Jones, 2015/03/03 21:37:46]

Done.

+  queue_.push_back(info);
+  return true;
+}
+
+bool UniqueFetchQueue::Pop(FetchInfo* info) {
+  if (IsEmpty())
+    return false;
+  *info = queue_.front();
+  queue_.pop_front();
+  return true;
+}
+
+bool UniqueFetchQueue::IsEmpty() const {
+  return queue_.empty();
+}
+
+void UniqueFetchQueue::ForgetPriorFetches() {
+  ever_cache_fetched_.clear();
+  ever_fetched_.clear();
+}
+
 }  // namespace
 
-SdchDictionaryFetcher::SdchDictionaryFetcher(
-    URLRequestContext* context,
-    const OnDictionaryFetchedCallback& callback)
+SdchDictionaryFetcher::SdchDictionaryFetcher(URLRequestContext* context)
     : next_state_(STATE_NONE),
       in_loop_(false),
+      fetch_queue_(new UniqueFetchQueue()),
       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;
+bool SdchDictionaryFetcher::Schedule(
+    const GURL& dictionary_url,
+    const OnDictionaryFetchedCallback& callback) {
+  return 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);
+bool SdchDictionaryFetcher::ScheduleReload(
+    const GURL& dictionary_url,
+    const OnDictionaryFetchedCallback& callback) {
+  return ScheduleInternal(dictionary_url, true, callback);
 }
 
 void SdchDictionaryFetcher::Cancel() {
   DCHECK(CalledOnValidThread());
 
   next_state_ = STATE_NONE;
+  current_request_.reset();
+  buffer_ = nullptr;
+  current_callback_.Reset();

[mmenke, 2015/02/26 17:27:20]

This looks a lot like ResetRequest...In fact, we're doing the exact same
thing...Except next_state_ is different.  We should use it and get rid of the
next_state_ transition in ResetRequest.  Or rename ResetRequest to
ResetRequestAndPrepareForNextRequest or something.

Sorry to harp so much on this, but having something name "ResetRequest" do
something else, which is something you want to do 3 out of 4 times when reset
the request just seems like a bad idea.

[Elly Fong-Jones, 2015/03/03 21:37:48]

Done.

 
-  while (!fetch_queue_.empty())
-    fetch_queue_.pop();
-  attempted_load_.clear();
+  fetch_queue_->ForgetPriorFetches();
   weak_factory_.InvalidateWeakPtrs();
-  current_request_.reset(NULL);
-  buffer_ = NULL;
   dictionary_.clear();
 }
 
@@ -115,7 +166,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;
+    }

[mmenke, 2015/02/26 17:27:20]

nit:  Don't use braces on 2 line ifs.

[Elly Fong-Jones, 2015/03/03 21:37:45]

Done.

+  }
+
+  DoLoop(result);
 }
 
 void SdchDictionaryFetcher::OnReadCompleted(URLRequest* request,
@@ -133,6 +200,47 @@ void SdchDictionaryFetcher::OnReadCompleted(URLRequest* request,
   DoLoop(GetReadResult(bytes_read, current_request_.get()));
 }
 
+bool SdchDictionaryFetcher::ScheduleInternal(
+    const GURL& dictionary_url,
+    bool reload,
+    const OnDictionaryFetchedCallback& callback) {
+  DCHECK(CalledOnValidThread());
+
+  // If Push() fails, |dictionary_url| has already been fetched or scheduled to
+  // be fetched.
+  if (!fetch_queue_->Push(FetchInfo(dictionary_url, reload, callback))) {
+    // 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 false;
+  }
+
+  // If the loop is already processing, it'll pick up the above in the
+  // normal course of events.
+  if (next_state_ != STATE_NONE)
+    return true;
+
+  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);
+  return true;
+}
+
+void SdchDictionaryFetcher::ResetRequest() {
+  current_request_.reset();
+  buffer_ = nullptr;
+  current_callback_.Reset();
+  next_state_ = STATE_SEND_REQUEST;
+  dictionary_.clear();
+  return;
+}
+
 int SdchDictionaryFetcher::DoLoop(int rv) {
   DCHECK(!in_loop_);
   base::AutoReset<bool> auto_reset_in_loop(&in_loop_, true);
@@ -170,19 +278,24 @@ int SdchDictionaryFetcher::DoSendRequest(int rv) {
   // |rv| is ignored, as the result from the previous request doesn't
   // affect the next request.
 
-  if (fetch_queue_.empty() || current_request_.get()) {
+  if (fetch_queue_->IsEmpty() || current_request_.get()) {
     next_state_ = STATE_NONE;
     return OK;
   }
 
   next_state_ = STATE_SEND_REQUEST_COMPLETE;
 
+  FetchInfo info;
+  fetch_queue_->Pop(&info);

[Randy Smith (Not in Mondays), 2015/02/23 21:23:49]

DCHECK that return value is true?

[Elly Fong-Jones, 2015/03/03 21:37:46]

Done.

   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(info.url, IDLE, this, NULL);
+  int load_flags = LOAD_DO_NOT_SEND_COOKIES | LOAD_DO_NOT_SAVE_COOKIES;
+  if (info.cache_only)
+    load_flags |= LOAD_ONLY_FROM_CACHE;
+  current_request_->SetLoadFlags(load_flags);
+
   buffer_ = new IOBuffer(kBufferSize);
-  fetch_queue_.pop();
+  current_callback_ = info.callback;
 
   current_request_->Start();
   current_request_->net_log().AddEvent(NetLog::TYPE_SDCH_DICTIONARY_FETCH);
@@ -211,10 +324,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 +365,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;
 }