Make Blacklist::IsBlacklist asynchronous (it will need to be for safe

chrome/browser/extensions/blacklist.cc

 // Copyright 2012 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 "chrome/browser/extensions/blacklist.h"
 
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/message_loop.h"
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/pref_names.h"
 
 namespace extensions {
 
 Blacklist::Observer::Observer(Blacklist* blacklist) : blacklist_(blacklist) {
   blacklist_->AddObserver(this);
 }
 
 Blacklist::Observer::~Observer() {
   blacklist_->RemoveObserver(this);
 }
 
 Blacklist::Blacklist(ExtensionPrefs* prefs) : prefs_(prefs) {
 }
 
 Blacklist::~Blacklist() {
 }
 
-bool Blacklist::IsBlacklisted(const Extension* extension) const {
-  return prefs_->IsExtensionBlacklisted(extension->id());
+void Blacklist::IsBlacklisted(const std::set<std::string>& ids,
+                              const IsBlacklistedCallback& callback) {
+  // TODO(kalman): Get the blacklisted IDs from the safebrowsing list.
+  // This will require going to the IO thread and back.
+  std::set<std::string> blacklisted_ids;
+  for (std::set<std::string>::const_iterator it = ids.begin();
+       it != ids.end(); ++it) {
+    if (prefs_->IsExtensionBlacklisted(*it))
+      blacklisted_ids.insert(*it);
+  }
+  MessageLoop::current()->PostTask(FROM_HERE,
+                                   base::Bind(callback, blacklisted_ids));
 }
 
-bool Blacklist::IsBlacklisted(const std::string& extension_id) const {
-  return prefs_->IsExtensionBlacklisted(extension_id);
+void Blacklist::IsBlacklisted(const std::string& id,
+                              const IsBlacklistedCallback& callback) {
+  std::set<std::string> id_set;
+  id_set.insert(id);
+  IsBlacklisted(id_set, callback);
 }
 
 void Blacklist::SetFromUpdater(const std::vector<std::string>& ids,
                                const std::string& version) {
   std::set<std::string> ids_as_set;
   for (std::vector<std::string>::const_iterator it = ids.begin();
        it != ids.end(); ++it) {
     if (Extension::IdIsValid(*it))
       ids_as_set.insert(*it);
     else
       LOG(WARNING) << "Got invalid extension ID \"" << *it << "\"";
   }
 
-  prefs_->UpdateBlacklist(ids_as_set);
+  std::set<std::string> from_prefs = prefs_->GetBlacklistedExtensions();
+
+  std::set<std::string> no_longer_blacklisted;
+  std::set_difference(from_prefs.begin(), from_prefs.end(),
+                      ids_as_set.begin(), ids_as_set.end(),
+                      std::inserter(no_longer_blacklisted,
+                                    no_longer_blacklisted.begin()));
+  std::set<std::string> not_yet_blacklisted;
+  std::set_difference(ids_as_set.begin(), ids_as_set.end(),
+                      from_prefs.begin(), from_prefs.end(),
+                      std::inserter(not_yet_blacklisted,
+                                    not_yet_blacklisted.begin()));

[asargent_no_longer_on_chrome, 2012/11/30 21:44:22]

It would be sort of cool if we had a SetDifference helper method to cut down on
all the STL boilerplate here. I wonder if enough other places in the code do
this that it would be worth having in base/stl_util.h.

[not at google - send to devlin, 2012/11/30 23:09:54]

Sounds good to me.

Here we go: https://codereview.chromium.org/11415239/

If that change goes in before this one, I'll convert this to using it. Otherwise
I'll do it in a follow-up.

+
+  for (std::set<std::string>::iterator it = no_longer_blacklisted.begin();
+       it != no_longer_blacklisted.end(); ++it) {
+    prefs_->SetExtensionBlacklisted(*it, false);
+  }
+  for (std::set<std::string>::iterator it = not_yet_blacklisted.begin();
+       it != not_yet_blacklisted.end(); ++it) {
+    prefs_->SetExtensionBlacklisted(*it, true);
+  }

[not at google - send to devlin, 2012/11/30 18:57:20]

I'm thinking about moving the prefs modification stuff into the similar-looking
code in ExtensionService, and making Blacklist just cache the result of
SetFromUpdater (reading in the initial blacklist from prefs). WDYT?

[asargent_no_longer_on_chrome, 2012/11/30 21:49:59]

Sorry, I missed this comment when going through the CL. What's the long-term
vision for the storage of this set of id's? You mention using safebrowsing -
does that mean we'll have a separate data store for them outside of the
profile-specific prefs? (I assume we'd just store in something like leveldb, as
opposed to getting fancy and having a bloom filter with fallback to network
requests to avoid false positives, since the data set is so much smaller.)

[not at google - send to devlin, 2012/11/30 22:06:39]

The nice thing about the safebrowsing databases is we won't get false positives,
since the hashes they use are 32 bytes, the same length as extension IDs. So we
can just store the IDs directly and not have to worry about round trips etc. I'm
writing the code to add the new database to the safebrowsing's set and it seems
pretty uncontroversial (cargo cult so far).

I believe we actually are going to start getting a lot more extensions
blacklisted, the process of which is blocked on getting this code out. During
migration perhaps we'll still have the old set but keep it small (only the very
bad extensions) then put the full set in safebrowsing.

(and yeah, obviously the blacklist property for extensions will remain in prefs,
just only for the extensions that are actually installed)

[not at google - send to devlin, 2012/11/30 23:09:54]

As you say, I think this change would make more sense to do as a follow-up when
the role of ExtensionPrefs as the blacklist has been fully replaced by
safebrowsing.

[Yoyo Zhou, 2012/11/30 23:44:07]

Just as a general comment, there should be less stuff in ExtensionService, not
more, so I'd prefer to have blacklist-related prefs interaction here in
Blacklist rather than in ExtensionService.

[not at google - send to devlin, 2012/12/01 02:51:30]

My plan was to have Blacklist be essentially an interface for querying a
blacklist, and notifying things when it changes, not actually doing the
management itself. For once I think that the logic I've added to
ExtensionService actually fits its designed purpose - managing installed
extensions.

The thing that bugs me a bit here is just that ExtensionService needs to read
the blacklist state of extensions from prefs (see synchronous check in
AddExtension) and yet doesn't actually set it.

Or to put it another way - when ExtensionService disables an extension it sets
the disabled bit in prefs. Blacklisting should work the same.

However, until the full-blacklist and extension-is-blacklisted concepts are
unconflated, I think it's best that it stays this way.

+
   prefs_->pref_service()->SetString(prefs::kExtensionBlacklistUpdateVersion,
                                     version);
 
   FOR_EACH_OBSERVER(Observer, observers_, OnBlacklistUpdated());
 }
 
 void Blacklist::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 void Blacklist::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 }  // namespace extensions