Prevent duplicate webstore install requests being serviced.

chrome/browser/extensions/api/webstore_private/webstore_private_api.cc

Index: chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
diff --git a/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc b/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
index a8ae144ba42618acfe2c6e54f8d09f1653be36bc..45ffa31ad0f410bb38c2f15eb798d97eda09feda 100644
--- a/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
+++ b/chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
@@ -52,17 +52,20 @@ namespace extensions {
 namespace {
 
 // Holds the Approvals between the time we prompt and start the installs.
-struct PendingApprovals {
-  typedef ScopedVector<WebstoreInstaller::Approval> ApprovalList;
-
+class PendingApprovals {
+ public:
   PendingApprovals();
   ~PendingApprovals();
 
   void PushApproval(scoped_ptr<WebstoreInstaller::Approval> approval);
   scoped_ptr<WebstoreInstaller::Approval> PopApproval(
       Profile* profile, const std::string& id);
+ private:
+  typedef ScopedVector<WebstoreInstaller::Approval> ApprovalList;
+
+  ApprovalList approvals_;
 
-  ApprovalList approvals;
+  DISALLOW_COPY_AND_ASSIGN(PendingApprovals);
 };
 
 PendingApprovals::PendingApprovals() {}
@@ -70,24 +73,78 @@ PendingApprovals::~PendingApprovals() {}
 
 void PendingApprovals::PushApproval(
     scoped_ptr<WebstoreInstaller::Approval> approval) {
-  approvals.push_back(approval.release());
+  approvals_.push_back(approval.release());
 }
 
 scoped_ptr<WebstoreInstaller::Approval> PendingApprovals::PopApproval(
     Profile* profile, const std::string& id) {
-  for (size_t i = 0; i < approvals.size(); ++i) {
-    WebstoreInstaller::Approval* approval = approvals[i];
+  for (size_t i = 0; i < approvals_.size(); ++i) {
+    WebstoreInstaller::Approval* approval = approvals_[i];
     if (approval->extension_id == id &&
         profile->IsSameProfile(approval->profile)) {
-      approvals.weak_erase(approvals.begin() + i);
+      approvals_.weak_erase(approvals_.begin() + i);
       return scoped_ptr<WebstoreInstaller::Approval>(approval);
     }
   }
   return scoped_ptr<WebstoreInstaller::Approval>(NULL);
 }
 
+// Uniquely holds the profile and extension id of an install between the time we
+// prompt and complete the installs.
+class PendingInstalls {
+ public:
+  PendingInstalls();
+  ~PendingInstalls();
+
+  bool InsertInstall(Profile* profile, const std::string& id);
+  void EraseInstall(Profile* profile, const std::string& id);
+ private:
+  typedef std::pair<Profile*, std::string> ProfileAndExtensionId;
+  typedef std::vector<ProfileAndExtensionId> InstallList;
+
+  InstallList::iterator FindInstall(Profile* profile, const std::string& id);
+
+  InstallList installs_;
+
+  DISALLOW_COPY_AND_ASSIGN(PendingInstalls);
+};
+
+PendingInstalls::PendingInstalls() {}
+PendingInstalls::~PendingInstalls() {}
+
+// Returns true and inserts the profile/id pair if it is not present. Otherwise
+// returns false.
+bool PendingInstalls::InsertInstall(Profile* profile, const std::string& id) {
+  if (FindInstall(profile, id) != installs_.end())
+    return false;
+  installs_.push_back(make_pair(profile, id));
+  return true;
+}
+
+// Removes the given profile/id pair.
+void PendingInstalls::EraseInstall(Profile* profile, const std::string& id) {
+  InstallList::iterator it = FindInstall(profile, id);
+  if (it != installs_.end())
+    installs_.erase(it);
+}
+
+PendingInstalls::InstallList::iterator PendingInstalls::FindInstall(
+    Profile* profile,
+    const std::string& id) {
+  for (size_t i = 0; i < installs_.size(); ++i) {
+    ProfileAndExtensionId install = installs_[i];
+    if (install.second == id &&
+        profile->IsSameProfile(install.first)) {
+      return (installs_.begin() + i);
+    }
+  }
+  return installs_.end();
+}

[asargent_no_longer_on_chrome, 2013/05/21 19:47:24]

Can you get away with not having this class at all, and instead just make the
InstallList typedef to a STL "Unique Associative Container" (eg, hashset) which
already has the "insert if the container doesn't already have the item"
behavior? 

http://www.sgi.com/tech/stl/UniqueAssociativeContainer.html

E.g.


#include "base/hash_tables.h"

...

typedef <base::hash_set<const Profile*, std::string> > InstallSet;

...

static base::LazyInstance<InstallSet> g_pending_installs =
    LAZY_INSTANCE_INITIALIZER;

...

if (service->GetInstalledExtension(id_) ||
    !g_pending_installs.Get().insert(profile_, id_).second) {
  SetResultCode(ALREADY_INSTALLED);
  error_ = kAlreadyInstalledError;
  return false;
}

...

g_pending_installs.Get().erase(profile_, id);




One other point - I wonder if it's worth adding some code somewhere that listens
for Profiles being destructed, and removes any matching elements from the set,
so that you don't end up with invalid Profile object pointers hanging around in
there. 

[calamity, 2013/05/24 00:24:28]

We can't use set because there's no ordering, C++11 has unordered_set but we
can't use that so that really only leaves us with hash_set.

I'm not very familiar with this stuff, but from what I gather, using hash_set
would require writing functor objects for the comparison and hashing. This
wouldn't be a problem really, but since it's not part of the standard, MSVC and
GCC have very different implementations. This would mean a couple of nasty
#ifdefs around webstore_private_api.cc which may complicate the code more than
simplify it.

In fact, I don't think I see any hash_sets in the codebase that use custom
comparators (except for a platform specific library in net/).

Also, base/small_map.h mentions that vector is a good way to go for small sizes.
This is indeed a problem. I think the best way to fix this is for
WebstoreInstaller to listen for Profile destruction and cancel the download. Let
me know what you think.

[asargent_no_longer_on_chrome, 2013/05/28 21:35:33]

Ok, good point - sounds like it wouldn't actually save that much code. 


 
That sounds like a good plan.

[asargent_no_longer_on_chrome, 2013/05/28 23:03:52]

Actually, here's one other possibility for you - your call about whether you
like this more or not:


#include "base/stl_util.h"


...


#include "base/stl_util.h"


typedef std::pair<const Profile*, std::string> PendingInstallItem;

class PendingInstalls {
 public:
   bool MaybeInsert(const Profile* profile, const std::string& id) {
     PendingInstallItem item = std::make_pair(profile, id);
     if (ContainsKey(installs_, item)) {
       return false;
     } else {
       installs_.insert(item);
       return true;
     }
   }

  void Erase(const Profile* profile, const std::string& id) {
    PendingInstallItem item = std::make_pair(profile, id);
    installs_.erase(item);
  }
 private:
  std::set<PendingInstallItem> installs_;
};


static base::LazyInstance<PendingInstalls> g_pending_installs =
    LAZY_INSTANCE_INITIALIZER;

+
 static base::LazyInstance<PendingApprovals> g_pending_approvals =
     LAZY_INSTANCE_INITIALIZER;
+static base::LazyInstance<PendingInstalls> g_pending_installs =
+    LAZY_INSTANCE_INITIALIZER;
 
 const char kAppInstallBubbleKey[] = "appInstallBubble";
 const char kEnableLauncherKey[] = "enableLauncher";
@@ -98,6 +155,7 @@ const char kLocalizedNameKey[] = "localizedName";
 const char kLoginKey[] = "login";
 const char kManifestKey[] = "manifest";
 
+const char kAlreadyInstalledError[] = "This item is already installed";
 const char kCannotSpecifyIconDataAndUrlError[] =
     "You cannot specify both icon data and an icon url";
 const char kInvalidIconUrlError[] = "Invalid icon url";
@@ -223,6 +281,15 @@ bool BeginInstallWithManifestFunction::RunImpl() {
     return false;
   }
 
+  ExtensionService* service =
+    extensions::ExtensionSystem::Get(profile_)->extension_service();
+  if (service->GetInstalledExtension(id_) ||
+      !g_pending_installs.Get().InsertInstall(profile_, id_)) {
+    SetResultCode(ALREADY_INSTALLED);
+    error_ = kAlreadyInstalledError;
+    return false;
+  }
+
   EXTENSION_FUNCTION_VALIDATE(details->GetString(kManifestKey, &manifest_));
 
   if (details->HasKey(kIconDataKey) && details->HasKey(kIconUrlKey)) {
@@ -307,6 +374,9 @@ void BeginInstallWithManifestFunction::SetResultCode(ResultCode code) {
     case SIGNIN_FAILED:
       SetResult(Value::CreateStringValue("signin_failed"));
       break;
+    case ALREADY_INSTALLED:
+      SetResult(Value::CreateStringValue("already_installed"));
+      break;
     default:
       CHECK(false);
   }
@@ -532,6 +602,8 @@ void CompleteInstallFunction::OnExtensionInstallSuccess(
   if (test_webstore_installer_delegate)
     test_webstore_installer_delegate->OnExtensionInstallSuccess(id);
 
+  g_pending_installs.Get().EraseInstall(profile_, id);
+
   LOG(INFO) << "Install success, sending response";
   SendResponse(true);
 
@@ -551,6 +623,8 @@ void CompleteInstallFunction::OnExtensionInstallFailure(
         id, error, reason);
   }
 
+  g_pending_installs.Get().EraseInstall(profile_, id);
+
   error_ = error;
   LOG(INFO) << "Install failed, sending response";
   SendResponse(false);