Prevent duplicate webstore install requests being serviced.

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

 // Copyright (c) 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/api/webstore_private/webstore_private_api.h"
 
 #include "apps/app_launcher.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
@@ skipping 34 matching lines @@
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using content::GpuDataManager;
 
 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() {}
 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";
 const char kIconDataKey[] = "iconData";
 const char kIconUrlKey[] = "iconUrl";
 const char kIdKey[] = "id";
 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";
 const char kInvalidIdError[] = "Invalid id";
 const char kInvalidManifestError[] = "Invalid manifest";
 const char kNoPreviousBeginInstallWithManifestError[] =
     "* does not match a previous call to beginInstallWithManifest3";
 const char kUserCancelledError[] = "User cancelled install";
 
 // Helper to create a dictionary with login properties set from the appropriate
@@ skipping 105 matching lines @@
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
   CHECK(details);
 
   EXTENSION_FUNCTION_VALIDATE(details->GetString(kIdKey, &id_));
   if (!extensions::Extension::IdIsValid(id_)) {
     SetResultCode(INVALID_ID);
     error_ = kInvalidIdError;
     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)) {
     SetResultCode(ICON_ERROR);
     error_ = kCannotSpecifyIconDataAndUrlError;
     return false;
   }
 
   if (details->HasKey(kIconDataKey))
     EXTENSION_FUNCTION_VALIDATE(details->GetString(kIconDataKey, &icon_data_));
@@ skipping 64 matching lines @@
       break;
     case PERMISSION_DENIED:
       SetResult(Value::CreateStringValue("permission_denied"));
       break;
     case INVALID_ICON_URL:
       SetResult(Value::CreateStringValue("invalid_icon_url"));
       break;
     case SIGNIN_FAILED:
       SetResult(Value::CreateStringValue("signin_failed"));
       break;
+    case ALREADY_INSTALLED:
+      SetResult(Value::CreateStringValue("already_installed"));
+      break;
     default:
       CHECK(false);
   }
 }
 
 void BeginInstallWithManifestFunction::OnWebstoreParseSuccess(
     const std::string& id,
     const SkBitmap& icon,
     DictionaryValue* parsed_manifest) {
   CHECK_EQ(id_, id);
@@ skipping 205 matching lines @@
       &(dispatcher()->delegate()->GetAssociatedWebContents()->GetController()),
       id, approval_.Pass(), WebstoreInstaller::FLAG_NONE);
   installer->Start();
 }
 
 void CompleteInstallFunction::OnExtensionInstallSuccess(
     const std::string& id) {
   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);
 
   // Matches the AddRef in RunImpl().
   Release();
 }
 
 void CompleteInstallFunction::OnExtensionInstallFailure(
     const std::string& id,
     const std::string& error,
     WebstoreInstaller::FailureReason reason) {
   extensions::InstallTracker* tracker =
       extensions::InstallTrackerFactory::GetForProfile(profile());
   tracker->OnInstallFailure(id);
   if (test_webstore_installer_delegate) {
     test_webstore_installer_delegate->OnExtensionInstallFailure(
         id, error, reason);
   }
 
+  g_pending_installs.Get().EraseInstall(profile_, id);
+
   error_ = error;
   LOG(INFO) << "Install failed, sending response";
   SendResponse(false);
 
   // Matches the AddRef in RunImpl().
   Release();
 }
 
 void CompleteInstallFunction::OnExtensionDownloadProgress(
     const std::string& id,
@@ skipping 76 matching lines @@
       &GetIsLauncherEnabledFunction::OnIsLauncherCheckCompleted, this));
   return true;
 }
 
 void GetIsLauncherEnabledFunction::OnIsLauncherCheckCompleted(bool is_enabled) {
   SetResult(Value::CreateBooleanValue(is_enabled));
   SendResponse(true);
 }
 
 }  // namespace extensions