Create CrxInstaller directly in WebstoreInstaller

Create CrxInstaller directly in WebstoreInstaller

The WebstoreInstaller needs a way to keep track of extensions being
downloaded and installed. The installation is handled by CrxInstaller
but when it used to be created automatically by the
ChromeDownloadManagerDelegate, the WebstoreInstaller had little control
over it and needed the CrxInstaller to keep the original_download_url as
sort of an identifier.

With this change the WebstoreInstaller creates the CrxInstaller itself
and keeps a pointer to it so there is no more need for
CrxInstaller::original_download_url().

This is also more robust because URLs are not unique identifiers (there
could be two installations run simultaneously for one download URL which
would have led to a crash in the old code).

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

[not at google - send to devlin, 2014-04-04 15:39:29]

mind filing a bug containing the detail you give in the CL description and
referencing it?

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (left):

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:627:
installer->set_download_url(download_url);
why not setting download ID? was this always unnecessary?

[msimonides, 2014-04-07 07:23:37]

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (left):

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:627:
installer->set_download_url(download_url);
On 2014/04/04 15:39:30, kalman wrote:
> why not setting download ID? was this always unnecessary?

The download ID is not available here. But most importantly, the UpdateExtension
method isn't used by the WebstoreInstaller so the value isn't necessary and
wouldn't be used.

[asanka, 2014-04-07 15:26:00]

/download/ lgtm

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:547: const GURL& download_url,
Nit: download_url is unused.

[not at google - send to devlin, 2014-04-07 16:51:40]

+Antony for a second opinion. You likely know this code much better than I do
anyway.

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/cr...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/cr...
chrome/browser/extensions/crx_installer.h:312: uint32 download_id_;
This needs to be initialised to something. Is there a value you can set it to
that guarantees it won't accidentally have a value of a real download ID?

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (left):

https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:627:
installer->set_download_url(download_url);
On 2014/04/07 07:23:38, msimonides wrote:
> On 2014/04/04 15:39:30, kalman wrote:
> > why not setting download ID? was this always unnecessary?
> 
> The download ID is not available here. But most importantly, the
UpdateExtension
> method isn't used by the WebstoreInstaller so the value isn't necessary and
> wouldn't be used.

I see. It's weird that the download ID is a concern of CrxInstaller if only
WebstoreInstaller cares about it. Speaking of which I see several other
instantiations of WebstoreInstaller and only once place where the ID is set from
CrxInstaller, which originates from ChromeDownloadManagerDelegate. I'm confused.

[msimonides, 2014-04-08 08:14:17]

On 2014/04/07 16:51:40, kalman wrote:
> +Antony for a second opinion. You likely know this code much better than I do
> anyway.
> 
>
https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/cr...
> File chrome/browser/extensions/crx_installer.h (right):
> 
>
https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/cr...
> chrome/browser/extensions/crx_installer.h:312: uint32 download_id_;
> This needs to be initialised to something. Is there a value you can set it to
> that guarantees it won't accidentally have a value of a real download ID?
> 
>
https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_service.cc (left):
> 
>
https://codereview.chromium.org/226023003/diff/1/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_service.cc:627:
> installer->set_download_url(download_url);
> On 2014/04/07 07:23:38, msimonides wrote:
> > On 2014/04/04 15:39:30, kalman wrote:
> > > why not setting download ID? was this always unnecessary?
> > 
> > The download ID is not available here. But most importantly, the
> UpdateExtension
> > method isn't used by the WebstoreInstaller so the value isn't necessary and
> > wouldn't be used.
> 
> I see. It's weird that the download ID is a concern of CrxInstaller if only
> WebstoreInstaller cares about it. Speaking of which I see several other
> instantiations of WebstoreInstaller and only once place where the ID is set
from
> CrxInstaller, which originates from ChromeDownloadManagerDelegate. I'm
confused.

The download ID (or URL) is used only to match the CrxInstaller with a
particular download process within the WebstoreInstaller. If the
WebstoreInstaller observed the extension download finish itself and created the
CrxInstaller itself (instead of relying on
ChromeDownloadManagerDelegate::ShouldOpenDownload doing it behind the scenes) it
could track everything without the need for the download ID/URL in the
CrxInstaller.

[asargent_no_longer_on_chrome, 2014-04-08 23:55:19]

Similar to kalman, I'm a little concerned about how we're managing the download
ID's here. It seems like it would be really easy for future code changes to
accidentally not keep the download id up to date. It's kind of weird in general
that CrxInstaller has anything to do with downloads, because it's interface is
essentially "please go install the .crx file located at path /foo/bar.crx", and
it gets used to install crx files that were never downloaded (eg external
installs). 

It sounds like the root problem we're trying to address is concurrent attemps to
download and install the same extension. How about this for an alternate
approach that might work better: keep track of the id's of items that
WebstoreInstaller is trying to install (I believe it finds out the id pretty
early on). Then if we get a new instance trying to install the same id, we can
either make that one fair immediately, or perhaps if it is possible, try to
cancel the existing pending one.

[msimonides, 2014-04-09 07:37:49]

On 2014/04/08 23:55:19, Antony Sargent wrote:
> Similar to kalman, I'm a little concerned about how we're managing the
download
> ID's here. It seems like it would be really easy for future code changes to
> accidentally not keep the download id up to date. It's kind of weird in
general
> that CrxInstaller has anything to do with downloads, because it's interface is
> essentially "please go install the .crx file located at path /foo/bar.crx",
and
> it gets used to install crx files that were never downloaded (eg external
> installs). 

I agree. But this change doesn't modify the design, it only switches the
identifier from URL (not really unique) to a download ID (unique).

> It sounds like the root problem we're trying to address is concurrent attemps
to
> download and install the same extension. How about this for an alternate
> approach that might work better: keep track of the id's of items that
> WebstoreInstaller is trying to install (I believe it finds out the id pretty
> early on). Then if we get a new instance trying to install the same id, we can
> either make that one fair immediately, or perhaps if it is possible, try to
> cancel the existing pending one.

This will not change the bad design, we'll still have and need the identifier
(in form of the non-unique URL) in CrxInstaller for matching with the
WebstoreInstaller.

IMO a better approach would be the one I've described above, i.e. have the
WebstoreInstaller create the CrxInstaller explicitly instead of the
ChromeDownloadManagerDelegate doing it behind the scenes.
I'll try to draft an implementation and submit it for review.

[msimonides, 2014-04-09 11:56:07]

On 2014/04/09 07:37:49, msimonides wrote:
> IMO a better approach would be the one I've described above, i.e. have the
> WebstoreInstaller create the CrxInstaller explicitly instead of the
> ChromeDownloadManagerDelegate doing it behind the scenes.
> I'll try to draft an implementation and submit it for review.

I have uploaded a patchset that does it. The CrxInstaller is created by
WebstoreInstaller so it has direct access to it and all the notifications from
can be matched to the CrxInstaller directly. Therefore there is no need to keep
any download IDs or URLs in the CrxInstaller.
The code needs some cleanup (e.g. WebstoreInstaller::StartCrxInstaller should
probably share some code with download_crx_util::OpenChromeExtension) but first
I want to see if you like the approach.

[msimonides, 2014-04-15 08:10:55]

ping! :)

[asargent_no_longer_on_chrome, 2014-04-16 00:15:50]

Apologies for the review latency! I was on vacation for several days. 

I like the way you have things structured now. I think the only remaining thing
to do is what you said in your earlier comment, that it would be nice to clean
up the duplication of code for setting a bunch of flags on the CrxInstaller in
WebstoreInstaller::StartCrxInstaller and download_crx_util::OpenChromeExtension
so that they don't drift out of sync inadvertently in future code changes.

https://codereview.chromium.org/226023003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/226023003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.h:306: uint32 download_id_;
Did you accidentally leave this in? It doesn't look like it's used anywhere.

[msimonides, 2014-04-16 08:42:04]

https://codereview.chromium.org/226023003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/226023003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.h:306: uint32 download_id_;
On 2014/04/16 00:15:51, Antony Sargent wrote:
> Did you accidentally leave this in? It doesn't look like it's used anywhere. 

Done.

[asargent_no_longer_on_chrome, 2014-04-16 18:15:03]

lgtm

[msimonides, 2014-04-17 06:03:45]

The CQ bit was checked by msimonides@opera.com

[commit-bot: I haz the power, 2014-04-17 06:04:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/226023003/60001

[commit-bot: I haz the power, 2014-04-17 07:04:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-17 07:04:39]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[msimonides, 2014-04-17 11:20:24]

The CQ bit was checked by msimonides@opera.com

[commit-bot: I haz the power, 2014-04-17 11:20:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/226023003/80001

[commit-bot: I haz the power, 2014-04-17 12:09:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-17 12:09:12]

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

[msimonides, 2014-04-17 12:13:07]

Need review for a test update in chrome/browser/performance_monitor

[Yoyo Zhou, 2014-04-17 16:09:12]

On 2014/04/17 12:13:07, msimonides wrote:
> Need review for a test update in chrome/browser/performance_monitor

performance_monitor_browsertest LGTM

[msimonides, 2014-04-22 06:22:20]

The CQ bit was checked by msimonides@opera.com

[commit-bot: I haz the power, 2014-04-22 06:22:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/226023003/80001

[commit-bot: I haz the power, 2014-04-22 07:29:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 07:29:48]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[msimonides, 2014-04-22 09:17:18]

The CQ bit was checked by msimonides@opera.com

[commit-bot: I haz the power, 2014-04-22 09:17:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/226023003/100001

[commit-bot: I haz the power, 2014-04-22 10:10:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 10:10:05]

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

[msimonides, 2014-04-23 09:28:03]

The CQ bit was checked by msimonides@opera.com

[commit-bot: I haz the power, 2014-04-23 09:28:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/226023003/140001

[msimonides, 2014-04-23 09:54:00]

The CQ bit was checked by msimonides@opera.com

[commit-bot: I haz the power, 2014-04-23 09:54:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/226023003/160001

[commit-bot: I haz the power, 2014-04-23 12:03:37]

Change committed as 265615

[Devlin, 2014-04-24 19:46:29]

On 2014/04/23 12:03:37, I haz the power (commit-bot) wrote:
> Change committed as 265615

Were the logs in CrxInstaller intentionally left in?  If not, could you remove
them, and if so, could you please document the reason?

Cheers!

[msimonides, 2014-04-25 05:53:57]

On 2014/04/24 19:46:29, D Cronin wrote:
> On 2014/04/23 12:03:37, I haz the power (commit-bot) wrote:
> > Change committed as 265615
> 
> Were the logs in CrxInstaller intentionally left in?  If not, could you remove
> them, and if so, could you please document the reason?

Oops, sorry about that. Removing them in
https://codereview.chromium.org/256593006/