The optional Malware Details also collects HTTP cache information. See design...

chrome/browser/safe_browsing/malware_details_cache.cc

+// Copyright (c) 2011 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.
+//
+// Implementation of the MalwareDetails class.
+
+#include "chrome/browser/safe_browsing/malware_details.h"
+
+#include "base/callback.h"
+#include "base/lazy_instance.h"
+#include "base/md5.h"
+#include "chrome/browser/net/chrome_url_request_context.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/malware_details_cache.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/safe_browsing/report.pb.h"
+#include "chrome/common/net/url_request_context_getter.h"
+#include "content/browser/browser_thread.h"
+#include "net/base/io_buffer.h"
+#include "net/disk_cache/disk_cache.h"
+#include "net/http/http_cache.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_response_info.h"
+
+using safe_browsing::ClientMalwareReportRequest;
+
+// Only send small files for now, a better strategy would use the size
+// of the whole report and the user's bandwidth.
+static const int kMaxBodySize = 1024;

[noelutz, 2011/04/06 20:34:19]

Is this in bytes or KB?

[panayiotis, 2011/04/07 20:08:32]

Done.

+
+MalwareDetailsCacheCollector::MalwareDetailsCacheCollector()
+    : cache_state_(STATE_NONE),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          cache_callback_(this , &MalwareDetailsCacheCollector::CacheLoop)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          entry_callback_(
+              new net::CancelableCompletionCallback<
+              MalwareDetailsCacheCollector>(
+                  this, &MalwareDetailsCacheCollector::CacheLoop))) {
+}
+
+MalwareDetailsCacheCollector::~MalwareDetailsCacheCollector() {
+}
+
+// We modify |resources| and |result|, so it must remain alive.

[noelutz, 2011/04/06 20:34:19]

Consider moving this comment to the header file.  It's more useful there.  And
what about the other pointers?

[panayiotis, 2011/04/07 20:08:32]

Done.

+void MalwareDetailsCacheCollector::StartCacheCollection(
+    URLRequestContextGetter* request_context_getter,
+    safe_browsing::ResourceMap* resources,
+    int* result,
+    Task* callback) {
+  // Start the data collection from the HTTP cache.
+  DVLOG(1) << "Getting disk_cache data for all urls...";
+  request_context_getter_ = request_context_getter;
+  resources_ = resources;
+  resources_it_ = resources_->begin();
+  cache_state_ = STATE_OPEN_CACHE;
+  result_ = result;
+  callback_ = callback;
+
+  AddRef();  // The cache_callback_ calls are asynchronous and not refcounted.
+
+  // Post a task in the message loop, so the callers don't need to
+  // check if we call their callback immediately.
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      NewRunnableMethod(
+          this, &MalwareDetailsCacheCollector::CacheLoop, net::OK));
+}
+
+bool MalwareDetailsCacheCollector::InProgress() {
+  return cache_state_ != STATE_NONE;

[noelutz, 2011/04/06 20:34:19]

I assume this is also always called on the IO thread?

[panayiotis, 2011/04/07 20:08:32]

Done.

+}
+
+// The IO thread loop. Called once the DOM details are callected,
+// and then is passed to all the cache lookups as a callback.
+void MalwareDetailsCacheCollector::CacheLoop(int result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(cache_state_ != STATE_NONE);
+
+  int rv = result;
+  do {
+    DVLOG(1) << "cache_state_:" << cache_state_;
+    CacheState state = cache_state_;
+    cache_state_ = STATE_NONE;
+    switch (state) {
+      case STATE_OPEN_CACHE:
+        rv = DoOpenCache();
+        break;
+      case STATE_OPEN_CACHE_COMPLETE:
+        rv = DoOpenCacheComplete(rv);
+        break;
+      case STATE_OPEN_ENTRY:
+        rv = DoOpenEntry();
+        break;
+      case STATE_OPEN_ENTRY_COMPLETE:
+        rv = DoOpenEntryComplete(rv);
+        break;
+      case STATE_READ_RESPONSE:
+        rv = DoReadResponse();
+        break;
+      case STATE_READ_RESPONSE_COMPLETE:
+        rv = DoReadResponseComplete(rv);
+        break;
+      case STATE_READ_DATA:
+        rv = DoReadData();
+        break;
+      case STATE_READ_DATA_COMPLETE:
+        rv = DoReadDataComplete(rv);
+        break;
+      default:
+        NOTREACHED() << "Bad state:" << state;
+        rv = net::ERR_FAILED;
+        break;
+    }
+  } while (rv != net::ERR_IO_PENDING && cache_state_ != STATE_NONE);

[noelutz, 2011/04/06 20:34:19]

I'm wondering if it's OK to do all these operations without breaking them up
into multiple smaller bits.  As far as I can tell it's possible and all these
methods are called one after another if net::ERR_IO_PENDING is never returned. 
What is the likelihood of that happening?  Wouldn't you take too much time on
the IO thread in that case?  I'm wondering if you don't want to force each of
these methods to run as a separate task on the IO message loop.

[panayiotis, 2011/04/07 20:08:32]

Done.

+
+  if (rv != net::ERR_IO_PENDING) {
+    DVLOG(1) << "CacheIO done.";
+    DoCacheDone(rv);
+  }
+}
+
+net::URLRequestContext* MalwareDetailsCacheCollector::GetURLRequestContext() {
+  if (!request_context_getter_) {
+    DVLOG(1) << "Missing request context getter";
+    return NULL;
+  }
+  return request_context_getter_->GetURLRequestContext();
+}
+
+int MalwareDetailsCacheCollector::DoOpenCache() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  cache_state_ = STATE_OPEN_CACHE_COMPLETE;
+
+  net::URLRequestContext* context = GetURLRequestContext();
+  if (!context) {
+    LOG(ERROR) << "Could not get URLRequestContext";

[noelutz, 2011/04/06 20:34:19]

Is there a particular reason you LOG(ERROR) here but not below?

[panayiotis, 2011/04/07 20:08:32]

Done.

+    cache_state_ = STATE_NONE;
+    return net::ERR_FAILED;
+  }
+
+  if (!context->http_transaction_factory()) {
+    DVLOG(1) << "Could not get HTTP transaction factory";
+    cache_state_ = STATE_NONE;
+    return net::ERR_FAILED;
+  }
+
+  net::HttpCache* http_cache =
+      context->http_transaction_factory()->GetCache();
+  if (!http_cache) {
+    DVLOG(1) << "Could not get http cache";
+    cache_state_ = STATE_NONE;
+    return net::ERR_FAILED;
+  }
+
+  return http_cache->GetBackend(&cache_, &cache_callback_);
+}
+
+int MalwareDetailsCacheCollector::DoOpenCacheComplete(int result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  cache_state_ = STATE_OPEN_ENTRY;
+
+  if (result == net::ERR_FAILED || !cache_) {
+    LOG(ERROR) << "Could not get disk cache";
+    cache_state_ = STATE_NONE;
+    return net::ERR_FAILED;
+  }
+
+  return net::OK;
+}
+
+int MalwareDetailsCacheCollector::DoOpenEntry() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  cache_state_ = STATE_OPEN_ENTRY_COMPLETE;
+
+  if (resources_it_ == resources_->end()) {
+    cache_state_ = STATE_NONE;
+    return net::OK;
+  }
+
+  entry_ = NULL;  // Reset the entry.
+  return cache_->OpenEntry(resources_it_->first, &entry_, &cache_callback_);

[noelutz, 2011/04/06 20:34:19]

Maybe mention what resources_it_ refers to?

[panayiotis, 2011/04/07 20:08:32]

Done.

+}
+
+int MalwareDetailsCacheCollector::DoOpenEntryComplete(int rv) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  cache_state_ = STATE_READ_RESPONSE;
+  return net::OK;

[noelutz, 2011/04/06 20:34:19]

I think there might be a bug here.  I think OpenEntry might return
et::ERR_IO_PENDING and use the cache_callback_ to say when the entry is
available.  What if an error occurs and rv is an error?  In that case entry_ is
probably invalid in which case you should probably abort, no?

[panayiotis, 2011/04/07 20:08:32]

Done.

+}
+
+int MalwareDetailsCacheCollector::DoReadResponse() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  cache_state_ = STATE_READ_RESPONSE_COMPLETE;
+
+  if (!entry_) {  // No entry in the cache for this url.

[noelutz, 2011/04/06 20:34:19]

Is that how you handle errors mentioned above?  Are you guaranteed that entry_
will not be assigned if there was an error in OpenEntry?

It might be nicer to skip some of the stages (i.e., change the value of the
cache_state_ if you know that the next stage isn't going to do anything anyways.

[panayiotis, 2011/04/07 20:08:32]

Done.

+    return net::OK;
+  }
+
+  // net::HttpCache::kResponseInfoIndex = 0.
+  buf_len_ = entry_->GetDataSize(0);

[noelutz, 2011/04/06 20:34:19]

why not use the constant then?

[panayiotis, 2011/04/07 20:08:32]

It's not public. 

+  if (!buf_len_) {
+    DVLOG(1) << "Empty response";
+    return buf_len_;

[noelutz, 2011/04/06 20:34:19]

This is confusing.  You always return net::something except here.  Can you
return net::OK instead?

[panayiotis, 2011/04/07 20:08:32]

well, now we return net::ERR_PENDING

but DoReadResponseComplete actually gets the size of the buffer ... confusing,
indeed. 

+  }
+
+  buf_ = new net::IOBuffer(buf_len_);
+  return entry_->ReadData(0, 0, buf_, buf_len_, entry_callback_);
+}
+
+int MalwareDetailsCacheCollector::DoReadResponseComplete(int result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  cache_state_ = STATE_READ_DATA;
+
+  if (!entry_ || !result || result != buf_len_) {
+    DVLOG(1) << "No entry";
+  } else {
+    linked_ptr<ClientMalwareReportRequest::Resource>
+        pb_resource = resources_it_->second;
+    net::HttpResponseInfo response;
+    bool truncated;
+    if (net::HttpCache::ParseResponseInfo(buf_->data(), buf_len_, &response,
+                                          &truncated)) {
+      if (!truncated) {
+        if (response.headers) {
+          ClientMalwareReportRequest::HTTPResponse* pb_response =
+              pb_resource->mutable_response();
+          pb_response->mutable_firstline()->set_code(
+              response.headers->response_code());
+          void* iter = NULL;
+          std::string name, value;
+          while (response.headers->EnumerateHeaderLines(&iter, &name, &value)) {
+            // Strip any Set-Cookie headers.
+            if (LowerCaseEqualsASCII(name, "set-cookie")) {
+              continue;
+            }
+            ClientMalwareReportRequest::HTTPHeader* pb_header =
+                pb_response->add_headers();
+            pb_header->set_name(name);
+            pb_header->set_value(value);
+          }
+        } else {
+          DVLOG(1) << "Missing response headers.";
+        }
+      } else {
+        DVLOG(1) << "Response truncated";
+      }
+    } else {
+      LOG(ERROR) << "Could not parse HTTP cache entry";
+    }
+  }
+
+  return net::OK;
+}
+
+int MalwareDetailsCacheCollector::DoReadData() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  cache_state_ = STATE_READ_DATA_COMPLETE;

[noelutz, 2011/04/06 20:34:19]

skip this stage too if entry_ wasn't set?

[panayiotis, 2011/04/07 20:08:32]

Done.

+
+  if (!entry_) {  // No entry in the cache for this url.
+    return net::OK;
+  }
+
+  // net::HttpCache::kResponseContentIndex = 1.
+  buf_len_ = entry_->GetDataSize(1);

[noelutz, 2011/04/06 20:34:19]

why not use the contant here too?

[panayiotis, 2011/04/07 20:08:32]

same reason as above, not public. 

+  if (!buf_len_) {
+    DVLOG(1) << "Empty data";
+    return buf_len_;
+  }
+
+  buf_ = new net::IOBuffer(buf_len_);
+  return entry_->ReadData(1, 0, buf_, buf_len_, entry_callback_);

[noelutz, 2011/04/06 20:34:19]

also use the contant here?

[panayiotis, 2011/04/07 20:08:32]

same

+}
+
+int MalwareDetailsCacheCollector::DoReadDataComplete(int result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  cache_state_ = STATE_OPEN_ENTRY;
+
+  if (entry_ && result && result == buf_len_) {
+    linked_ptr<ClientMalwareReportRequest::Resource>
+        pb_resource = resources_it_->second;
+    ClientMalwareReportRequest::HTTPResponse* pb_response =
+        pb_resource->mutable_response();
+    if (buf_len_ <= kMaxBodySize) {  // Only send small bodies for now.

[noelutz, 2011/04/06 20:34:19]

you know in the previous stage (DoReadData) that you won't read that response. 
Would it make sense to skip it in DoReadData already and avoid a call to
ReadData?

[panayiotis, 2011/04/07 20:08:32]

Done.

[noelutz, 2011/04/11 20:32:10]

I don't think this is done.  Is there any reason not do move this check to
DoREadData?

[panayiotis, 2011/04/11 21:42:16]

Oh sorry, I missed this. I actually do read the response, so that I can
calculate the md5 digest. I just skip adding it to the protocol buffer if it's
too big. 

On 2011/04/11 20:32:10, noelutz wrote:

[noelutz, 2011/04/11 21:49:15]

sounds good.  i missed the digest part.

+      pb_response->set_body(buf_->data(), buf_len_);
+    }
+    pb_response->set_bodylength(buf_len_);
+    MD5Digest digest;
+    MD5Sum(buf_->data(), buf_len_, &digest);
+    pb_response->set_bodydigest(MD5DigestToBase16(digest));
+    entry_->Close();

[noelutz, 2011/04/06 20:34:19]

If you do what I mentioned above make sure to close the entry :).  Also, I'm not
sure this always works.  What if the entry_ was read properly but the second
ReadData failed and result here is negative?  Don't you still have to call
entry_->Close() in this case?

[panayiotis, 2011/04/07 20:08:32]

Done.

+  }
+
+  ++resources_it_;  // Advance to the next entry.

[noelutz, 2011/04/06 20:34:19]

A few things here:
 - Before you go read the next entry do you want to set everything to NULL
(buf_, entry_, buf_len_)? 
  - I think you also need to release the IOBuffer object, no?

[panayiotis, 2011/04/07 20:08:32]

setting buf_ = NULL now. I verified this releases & deletes the IOBuffer. 

+  return net::OK;
+}
+
+void MalwareDetailsCacheCollector::DoCacheDone(int result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  *result_ = result;
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, callback_);
+
+  Release();
+  // We are now deleted.
+}