Fetch extension blacklist states from SafeBrowsing server

chrome/browser/extensions/blacklist.cc

Index: chrome/browser/extensions/blacklist.cc
diff --git a/chrome/browser/extensions/blacklist.cc b/chrome/browser/extensions/blacklist.cc
index f39d1137a968dff41c438a8ed0ab947182b65446..ab8f1d3776b0f4b6e7719e313fe42951be717b04 100644
--- a/chrome/browser/extensions/blacklist.cc
+++ b/chrome/browser/extensions/blacklist.cc
@@ -14,6 +14,7 @@
 #include "base/stl_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/blacklist_fetcher.h"
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/safe_browsing_util.h"
@@ -154,7 +155,7 @@ Blacklist::ScopedDatabaseManagerForTest::~ScopedDatabaseManagerForTest() {
   SetDatabaseManager(original_);
 }
 
-Blacklist::Blacklist(ExtensionPrefs* prefs) {
+Blacklist::Blacklist(ExtensionPrefs* prefs) : fetcher_() {

[not at google - send to devlin, 2013/11/18 17:03:49]

fetcher_() is implicit?

[Oleg Eterevsky, 2013/11/20 12:58:45]

Removed.

   scoped_refptr<SafeBrowsingDatabaseManager> database_manager =
       g_database_manager.Get().get();
   if (database_manager) {
@@ -223,7 +224,9 @@ void Blacklist::GetBlacklistStateForIDs(
        it != blacklisted_ids.end(); ++it) {
     BlacklistStateMap::const_iterator cache_it =
         blacklist_state_cache_.find(*it);
-    if (cache_it == blacklist_state_cache_.end())
+    if (cache_it == blacklist_state_cache_.end() ||
+        cache_it->second == BLACKLISTED_UNKNOWN)  // Do not return UNKNOWN
+                                                  // from cache, retry request.
       ids_unknown_state.insert(*it);
     else
       extensions_state[*it] = cache_it->second;
@@ -251,8 +254,9 @@ void Blacklist::ReturnBlacklistStateMap(
        it != blacklisted_ids.end(); ++it) {
     BlacklistStateMap::const_iterator cache_it =
         blacklist_state_cache_.find(*it);
-    if (cache_it != blacklist_state_cache_.end())
+    if (cache_it != blacklist_state_cache_.end()) {
       extensions_state[*it] = cache_it->second;
+    }

[not at google - send to devlin, 2013/11/18 17:03:49]

preferred the way it was

[Oleg Eterevsky, 2013/11/20 12:58:45]

Must have added something and then removed. Changed back.

     // If for some reason we still haven't cached the state of this extension,
     // we silently skip it.
   }
@@ -263,15 +267,50 @@ void Blacklist::ReturnBlacklistStateMap(
 void Blacklist::RequestExtensionsBlacklistState(
     const std::set<std::string> ids, base::Callback<void()> callback) {

[not at google - send to devlin, 2013/11/18 17:03:49]

Both of these should have been const&; in the latter case I wasn't aware, but in
reading the callback.h documentation I saw this:
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&sq...

[Oleg Eterevsky, 2013/11/20 12:58:45]

Done.

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // This is a stub. The request will be made here, but the server is not up
-  // yet. For compatibility with current blacklist logic, mark all extensions
-  // as malicious.
+  if (!fetcher_) {
+    fetcher_.reset(new BlacklistFetcher());
+  }

[not at google - send to devlin, 2013/11/18 17:03:49]

no {} around single-line if bodies

[Oleg Eterevsky, 2013/11/20 12:58:45]

Done.

+
+  state_requests_.push_back(
+      make_pair(std::vector<std::string>(ids.begin(), ids.end()), callback));
   for (std::set<std::string>::const_iterator it = ids.begin();
        it != ids.end();
        ++it) {
-    blacklist_state_cache_[*it] = BLACKLISTED_MALWARE;
+    fetcher_->Request(*it, base::Bind(&Blacklist::OnBlacklistStateReceived,
+                                      AsWeakPtr(), *it));
+  }
+}
+
+void Blacklist::OnBlacklistStateReceived(std::string id, BlacklistState state) {

[not at google - send to devlin, 2013/11/18 17:03:49]

const std::string&

base::Bind can handle it.

[Oleg Eterevsky, 2013/11/20 12:58:45]

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  blacklist_state_cache_[id] = state;
+
+  StateRequestsList::iterator requests_it = state_requests_.begin();

[not at google - send to devlin, 2013/11/18 17:03:49]

I'm not sure you need state_requests_ at all. BlacklistFetcher knows how to
coalesce fetches. You do caching. If you pass around the result map with a
callback and a request count, then run the callback when size(result map) ==
request count, you'd save the effort of looping over all state requests every
time any response comes back... and this class would be carrying around less
state, which is nice.

[Oleg Eterevsky, 2013/11/20 12:58:45]

I'm a bit reluctant about it. To do this we'll have to pass BlacklistStateMap&
to this function and I am not entirely sure, what will happen to non-const value
like that in the callback. Who will free it once it is not use? Do you propose
to make it scoped_refptr? I feel safer when I have some state in the class as
opposed to some curried callback.

Also in most cases there sholdn't be more than 1 simultaneous request, so the
linear search over requests shouldn't be inefficient.

On 2013/11/18 17:03:49, kalman wrote:

[not at google - send to devlin, 2013/11/20 23:44:48]

You would pass around a pointer with base::Owned(). Fair point about probably
only being called once, though I prefer it if the class didn't maintain
transient state like that, IMO it's nicer to curry transient state where
possible. I think the code would be simpler overall as well.

[Oleg Eterevsky, 2013/11/21 16:53:13]

base::Owned won't work here, since OnBlacklistStateReceived will be called
several times if several blacklist statuses are requested, and the instances of
base::Callback will have to share this map.

I also thought of passing to OnBlacklistStateReceived the set of ids and a final
callback, but this can fail also, since if there are two concurrent requests for
the same 2 ids, the callback will be called twice.

The only possible way to deal with it is to have a reference-counted
BlacklistStateMap, and I dislike this solution.

Adding a comment on what's happening here.

[not at google - send to devlin, 2013/11/21 18:42:57]

You're right, and now that my memory is working correctly, I suggested passing
around a counter I think, which was just a crappy version of refcounting.

Honestly I don't think refcounting is that big a deal, it's a not-uncommon idiom
of these things to create a refcounted wrapper class with the map as a member
variable, populate it as callbacks come in, then send the response on
destruction of that wrapper class. Using that idiom is safe with respect to the
message loop and/or the state fetcher from going away.

+  while (requests_it != state_requests_.end()) {
+    const std::vector<std::string>& ids = requests_it->first;
+
+    bool have_all_in_cache = true;
+    for (std::vector<std::string>::const_iterator ids_it = ids.begin();
+         ids_it != ids.end();
+         ++ids_it) {
+      if (blacklist_state_cache_.find(*ids_it) ==
+          blacklist_state_cache_.end()) {

[not at google - send to devlin, 2013/11/18 17:03:49]

since you're not holding onto the iterator result of find(), you could use
blacklist_state_cache_.count(*ids_it) here.

[Oleg Eterevsky, 2013/11/20 12:58:45]

I just googled "find vs count stl" and people tend to find find() more readable.
It is also more compatible with other types of containers.

[not at google - send to devlin, 2013/11/20 23:44:48]

I'm told you should use ContainsKey:

https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&q=...

[Oleg Eterevsky, 2013/11/21 16:53:13]

Replaced it here and in other places.

On 2013/11/20 23:44:48, kalman wrote:

+        have_all_in_cache = false;
+        break;
+      }
+    }
+
+    if (have_all_in_cache) {
+      requests_it->second.Run();
+      requests_it = state_requests_.erase(requests_it);
+    } else {
+      ++requests_it;
+    }
   }
-  callback.Run();
+}
+
+void Blacklist::SetBlacklistFetcherForTest(BlacklistFetcher* fetcher) {
+  fetcher_.reset(fetcher);
 }
 
 void Blacklist::AddObserver(Observer* observer) {