Prevent duplicate concurrent installs of the same extension

Prevent duplicate concurrent installs of the same extension

This patch stores the state of active installs in the InstallTracker
for global access. This allows various install initiators to check for
duplicate installs before proceeding.

Install progress bars in the app launcher can be initialized with the
state of an active install.

BUG=393854
TEST=unit_tests, browser_tests
For manual testing steps, please see bug.

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

[tmdiep, 2014-07-15 05:18:35]

webstore_private_api.cc had a registry of pending installs to prevent duplicate
installs from the webstore, but now that we have inline installs, install from
the app launcher and ephemeral installs, I've been able to induce it more
easily. The installs clobber each other and progress bars are erratic
(reflecting the status of multiple separate installs). Occasionally I've seen
crashes or DCHECK failures.

Probably best to review install_tracker first. The rest follows. Let me know if
you have a better solution. Thanks in advance.

[tmdiep, 2014-07-15 06:14:06]

Do you think we need to handle duplicate installs in the bundle installer?

[asargent_no_longer_on_chrome, 2014-07-16 18:48:45]

On 2014/07/15 06:14:06, tmdiep wrote:
> Do you think we need to handle duplicate installs in the bundle installer?

No, in fact we can actually rip out all of the bundle installer code because it
isn't needed (the feature was experimental and never launched) -
https://code.google.com/p/chromium/issues/detail?id=243879

[asargent_no_longer_on_chrome, 2014-07-16 18:49:07]

https://codereview.chromium.org/389613006/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_launcher.h (right):

https://codereview.chromium.org/389613006/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_launcher.h:120: install_data) const OVERRIDE;
nit: formatting looks weird, maybe > 80 chars? Pls ignore if you're already run
'git cl format' and this is what it came up with

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

https://codereview.chromium.org/389613006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:420:
RemoveActiveInstall(GetProfile(), params_->details.id);
It would be cool if we had something like a "ScopedActiveInstall" object for the
install tracker, so that we wouldn't have to remember to call
RemoveActiveInstall everywhere - it would automatically get removed when the
ScopedActiveInstall was deleted.

[tmdiep, 2014-07-17 01:01:52]

https://codereview.chromium.org/389613006/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_launcher.h (right):

https://codereview.chromium.org/389613006/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_launcher.h:120: install_data) const OVERRIDE;
On 2014/07/16 18:49:07, Antony Sargent wrote:
> nit: formatting looks weird, maybe > 80 chars? Pls ignore if you're already
run
> 'git cl format' and this is what it came up with
 
Yes, this was the work of `git cl format`, but has been avoided now.

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

https://codereview.chromium.org/389613006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:420:
RemoveActiveInstall(GetProfile(), params_->details.id);
On 2014/07/16 18:49:07, Antony Sargent wrote:
> It would be cool if we had something like a "ScopedActiveInstall" object for
the
> install tracker, so that we wouldn't have to remember to call
> RemoveActiveInstall everywhere - it would automatically get removed when the
> ScopedActiveInstall was deleted. 

That's a great idea. Done.

The WebstorePrivateAPI install is split between beginInstall and
completeInstall, so that explains ScopedActiveInstall::CancelDeregister().

[tmdiep, 2014-07-17 01:03:40]

tapted: Please review app_list/webstore_result. Thanks.

[tapted, 2014-07-17 02:03:55]

On 2014/07/17 01:03:40, tmdiep wrote:
> tapted: Please review app_list/webstore_result. Thanks.

nice change - I think you're fixing quite a few corner cases that weren't
previously considered here.

mine is really just a nit

https://codereview.chromium.org/389613006/diff/50001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right):

https://codereview.chromium.org/389613006/diff/50001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/webstore/webstore_result.cc:129: void
WebstoreResult::SetInitialState() {
I like that this is located in the file closer to the constructor -- (it's
currently a bit subtle that StartObserving() is only called from the
constructor). But this and StartObserving are really closely related - i.e. it's
nice seeing { /* get current state */; /* start observing changes to that state
*/; } together.

I think we should move the contents of 'StartObserving' into this function, and
rename SetInitialState->StartObserving so it's symmetric with the Stop*
functions.

i.e. essentially move the call to `StartObserving();` in the constructor to
occur before UpdateActions, but also move the declaration of StartObserving in
the header file up so we can put it closer to the constructor and the file still
reads in the order that stuff happens.

then, with all that, i'd keep the `install_tracker_ =
InstallTrackerFactory::Get...` in the function rather than in an initializer.
(in an initializer, it suggests that the lifetimes are in step, but
install_tracker_ can still be removed early if `OnShutdown` is triggered before
the destructor.)

[tmdiep, 2014-07-17 03:37:52]

https://codereview.chromium.org/389613006/diff/50001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right):

https://codereview.chromium.org/389613006/diff/50001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/webstore/webstore_result.cc:129: void
WebstoreResult::SetInitialState() {
On 2014/07/17 02:03:55, tapted wrote:
> I like that this is located in the file closer to the constructor -- (it's
> currently a bit subtle that StartObserving() is only called from the
> constructor). But this and StartObserving are really closely related - i.e.
it's
> nice seeing { /* get current state */; /* start observing changes to that
state
> */; } together.
> 
> I think we should move the contents of 'StartObserving' into this function,
and
> rename SetInitialState->StartObserving so it's symmetric with the Stop*
> functions.
> 
> i.e. essentially move the call to `StartObserving();` in the constructor to
> occur before UpdateActions, but also move the declaration of StartObserving in
> the header file up so we can put it closer to the constructor and the file
still
> reads in the order that stuff happens.
> 
> then, with all that, i'd keep the `install_tracker_ =
> InstallTrackerFactory::Get...` in the function rather than in an initializer.
> (in an initializer, it suggests that the lifetimes are in step, but
> install_tracker_ can still be removed early if `OnShutdown` is triggered
before
> the destructor.)

Done. Hopefully I understood you correctly :)
I renamed StartObserving to InitAndStartObserving to more accurately describe
its responsibilities.

[tapted, 2014-07-17 03:47:30]

On 2014/07/17 03:37:52, tmdiep wrote:
> Done. Hopefully I understood you correctly :)

yep! app_list stuff lgtm.

[asargent_no_longer_on_chrome, 2014-07-17 06:24:51]

https://codereview.chromium.org/389613006/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/active_install_data.cc (right):

https://codereview.chromium.org/389613006/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/active_install_data.cc:44: InstallTracker* tracker =
InstallTracker::Get(profile_);
I'm a little concerned about this class holding a pointer to a Profile - it
seems like it might be possible for us to accidentally hold on to the Profile*
past its destruction during profile shutdown.  What do you think about one of
these alternatives to holding a Profile*?

A) Hold a WeakPtr to the InstallTracker (look it up from the Profile* inside the
ScopedActiveInstall constructor)

or

B) Hold a naked pointer to the InstallTracker, and have some sort of
InstallTrackerShutdownObserver interface that this object implements so that it
can register itself with the InstallTracker and at shutdown time the
InstallTracker can call a method in the interface that tells clients "I'm going
away now; you should drop your pointer to me now". I guess this is sort of
equivalent to a hand-rolled WeakPtr.

[tmdiep, 2014-07-17 08:32:40]

On 2014/07/17 06:24:51, Antony Sargent wrote:
>
https://codereview.chromium.org/389613006/diff/120001/chrome/browser/extensio...
> File chrome/browser/extensions/active_install_data.cc (right):
> 
>
https://codereview.chromium.org/389613006/diff/120001/chrome/browser/extensio...
> chrome/browser/extensions/active_install_data.cc:44: InstallTracker* tracker =
> InstallTracker::Get(profile_);
> I'm a little concerned about this class holding a pointer to a Profile - it
> seems like it might be possible for us to accidentally hold on to the Profile*
> past its destruction during profile shutdown.  What do you think about one of
> these alternatives to holding a Profile*?
> 
> A) Hold a WeakPtr to the InstallTracker (look it up from the Profile* inside
the
> ScopedActiveInstall constructor)
> 
> or
> 
> B) Hold a naked pointer to the InstallTracker, and have some sort of
> InstallTrackerShutdownObserver interface that this object implements so that
it
> can register itself with the InstallTracker and at shutdown time the
> InstallTracker can call a method in the interface that tells clients "I'm
going
> away now; you should drop your pointer to me now". I guess this is sort of
> equivalent to a hand-rolled WeakPtr.

Thanks for spotting this, that's a really good point. The InstallObserver
interface already has an OnShutdown() that we can use, so I opted for B).
Updated in patches 7 & 8.

[asargent_no_longer_on_chrome, 2014-07-17 17:02:25]

lgtm

[tmdiep, 2014-07-17 22:16:08]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-07-17 22:19:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/389613006/200001

[commit-bot: I haz the power, 2014-07-18 02:15:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-18 05:48:41]

Change committed as 284014