Prevent duplicate webstore install requests being serviced.

Prevent duplicate webstore install requests being serviced.

webstore_private_api returns "already_installed" when an install request specifies an extension id that is either already installed or is in the process of installing.

BUG=222735
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203070

[koz (OOO until 15th September), 2013-05-20 23:57:12]

lgtm with nits

https://codereview.chromium.org/15292011/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/15292011/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:91:
struct PendingInstalls {
Make this a class with |installs_| and related typedefs private (and don't
forget DISALLOW_COPY_AND_ASSIGN).

https://codereview.chromium.org/15292011/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:105: if
(FindInstall(profile, id) != installs_.end()) {
nit: no curlies

https://codereview.chromium.org/15292011/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:115: if
(it != installs_.end()) {
nit: no curlies

[calamity, 2013-05-21 00:49:16]

+asargent for webstore api OWNERS

[calamity, 2013-05-21 00:49:41]

https://chromiumcodereview.appspot.com/15292011/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://chromiumcodereview.appspot.com/15292011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:91:
struct PendingInstalls {
On 2013/05/20 23:57:12, koz wrote:
> Make this a class with |installs_| and related typedefs private (and don't
> forget DISALLOW_COPY_AND_ASSIGN).

Done.

https://chromiumcodereview.appspot.com/15292011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:105: if
(FindInstall(profile, id) != installs_.end()) {
On 2013/05/20 23:57:12, koz wrote:
> nit: no curlies

Done.

https://chromiumcodereview.appspot.com/15292011/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:115: if
(it != installs_.end()) {
On 2013/05/20 23:57:12, koz wrote:
> nit: no curlies

Done.

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

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:142: }
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.

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/common/exten...
File chrome/common/extensions/api/webstore_private.json (right):

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/common/exten...
chrome/common/extensions/api/webstore_private.json:127: "description": "A string
result code, which will be empty upon success. The possible values in the case
of errors include 'unknown_error', 'user_cancelled', 'manifest_error',
'icon_error', 'invalid_id', 'permission_denied', 'invalid_icon_url',
'signin_failed', and 'already_installed'."
Can we switch to use an enum here? (I think we didn't have enum support at the
time it was originally written, so a list in the doc string was the best we
could do)

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

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:142: }
On 2013/05/21 19:47:24, Antony Sargent wrote:
> 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);
> 
> 
> 

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.

> 
> 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. 
> 

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.

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/common/exten...
File chrome/common/extensions/api/webstore_private.json (right):

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/common/exten...
chrome/common/extensions/api/webstore_private.json:127: "description": "A string
result code, which will be empty upon success. The possible values in the case
of errors include 'unknown_error', 'user_cancelled', 'manifest_error',
'icon_error', 'invalid_id', 'permission_denied', 'invalid_icon_url',
'signin_failed', and 'already_installed'."
On 2013/05/21 19:47:24, Antony Sargent wrote:
> Can we switch to use an enum here? (I think we didn't have enum support at the
> time it was originally written, so a list in the doc string was the best we
> could do)

Hmm. Doing this causes the webstore to crash on install... I'm not sure why.

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

lgtm

Sorry for delay!

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:142: }
On 2013/05/24 00:24:28, calamity wrote:
> On 2013/05/21 19:47:24, Antony Sargent wrote:
> > 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);
> > 
> > 
> > 
> 
> 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.


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


 
> > 
> > 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. 
> > 
> 
> 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.

That sounds like a good plan.

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/common/exten...
File chrome/common/extensions/api/webstore_private.json (right):

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/common/exten...
chrome/common/extensions/api/webstore_private.json:127: "description": "A string
result code, which will be empty upon success. The possible values in the case
of errors include 'unknown_error', 'user_cancelled', 'manifest_error',
'icon_error', 'invalid_id', 'permission_denied', 'invalid_icon_url',
'signin_failed', and 'already_installed'."
On 2013/05/24 00:24:28, calamity wrote:
> On 2013/05/21 19:47:24, Antony Sargent wrote:
> > Can we switch to use an enum here? (I think we didn't have enum support at
the
> > time it was originally written, so a list in the doc string was the best we
> > could do)
> 
> Hmm. Doing this causes the webstore to crash on install... I'm not sure why.

Maybe a schema validation assertion being hit? With the current version we use
an empty string to signal success, but perhaps that's not compatible with enums?
Anyhow, that's fine to leave out of this CL if you can't get it working.

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

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:142: }
On 2013/05/28 21:35:33, Antony Sargent wrote:
> On 2013/05/24 00:24:28, calamity wrote:
> > On 2013/05/21 19:47:24, Antony Sargent wrote:
> > > 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);
> > > 
> > > 
> > > 
> > 
> > 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.
> 
> 
> Ok, good point - sounds like it wouldn't actually save that much code. 
> 
> 
>  
> > > 
> > > 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. 
> > > 
> > 
> > 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.
> 
> That sounds like a good plan.
> 


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;

[calamity, 2013-05-29 01:29:16]

On 2013/05/28 23:03:52, Antony Sargent wrote:
>
https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
> File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/15292011/diff/5001/chrome/browser/exte...
> chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:142: }
> On 2013/05/28 21:35:33, Antony Sargent wrote:
> > On 2013/05/24 00:24:28, calamity wrote:
> > > On 2013/05/21 19:47:24, Antony Sargent wrote:
> > > > 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);
> > > > 
> > > > 
> > > > 
> > > 
> > > 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.
> > 
> > 
> > Ok, good point - sounds like it wouldn't actually save that much code. 
> > 
> > 
> >  
> > > > 
> > > > 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. 
> > > > 
> > > 
> > > 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.
> > 
> > That sounds like a good plan.
> > 
> 
> 
> 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;

I don't think we can use std::set because it relies on sortability through the
less operator which we don't have here.

[commit-bot: I haz the power, 2013-05-29 01:29:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/15292011/31001

[commit-bot: I haz the power, 2013-05-29 04:20:48]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-05-29 09:58:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/15292011/47001

[commit-bot: I haz the power, 2013-05-29 12:26:17]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[asargent_no_longer_on_chrome, 2013-05-29 16:33:04]

> I don't think we can use std::set because it relies on sortability through
> the
> less operator which we don't have here.
>

The std::pair container does provide a default operator< method :
http://www.sgi.com/tech/stl/pair.html

It compiled fine for me on linux, not sure about Win/Mac. Anyhow, no big
deal.

[commit-bot: I haz the power, 2013-05-30 01:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/15292011/47001

[commit-bot: I haz the power, 2013-05-30 04:50:50]

Change committed as 203070