Improve WebstoreInstaller error handling.

Improve WebstoreInstaller error handling.

This addresses the cases where WebstoreInstaller does not hear about failed extension installs, like for download failures (404, etc.) or when the downloaded file is not a CRX.

BUG=118237
TEST=existing, also manual testing.
(Example: http://www.corp.google.com/~jstritar/bundle.html with apps-gallery-update-url set to various values like http://localhost)

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

[jstritar, 2012-03-23 20:23:25]

Mihai, can you take a look?
Randy, FYI and can you sanity check my use of the downloads APIs when you get a
chance?

[Randy Smith (Not in Mondays), 2012-03-23 20:57:19]

The biggest issue is that possibility of the use-after-free, which I think can
be solved by keeping a copy of the DownloadItem* around for use in the
destructor.

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.cc (right):

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:42: using content::DownloadItem;
I thought the style guide generally preferred actually qualifying the name at
point of use?  (Not a request to change if you feel strongly, just raising the
issue.)

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:152: item->RemoveObserver(this);
This scares me.  I'm not certain as to the lifetime of the WebstoreInstaller,
but if the download item completes but is not deleted before the
WebStoreInstaller destructor is called, I think the DownloadItem will still be
around but GetActiveDownloadItem() won't return it, so we won't remove ourselves
as an observer, so our observation interfaces may later be called (i.e.
use-after-free).  

The canonical model is to keep a copy of a pointer to the DownloadItem around,
and be sensitive with an update to the DownloadItem state to REMOVING, which'll
always occur before the DownloadItem is destroyed.

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:222: if (item)
YOu should be able to make this a DCHECK; the DI should be available from
GetActiveDownloadItem at this point.  (And if it's not, that's probably a
problem for you, so we should find out soon :-}.)

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.h (right):

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.h:25: class DownloadItem;
Why is this forward decl needed if you're including the .h file?

[jstritar, 2012-03-23 21:52:53]

Thank you for reviewing!

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.cc (right):

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:42: using content::DownloadItem;
On 2012/03/23 20:57:19, rdsmith wrote:
> I thought the style guide generally preferred actually qualifying the name at
> point of use?  (Not a request to change if you feel strongly, just raising the
> issue.)

I think this is okay in .cc files, but we're not supposed to pull in whole
namespaces like:

using namespace content;

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:152: item->RemoveObserver(this);
On 2012/03/23 20:57:19, rdsmith wrote:
> This scares me.  I'm not certain as to the lifetime of the WebstoreInstaller,
> but if the download item completes but is not deleted before the
> WebStoreInstaller destructor is called, I think the DownloadItem will still be
> around but GetActiveDownloadItem() won't return it, so we won't remove
ourselves
> as an observer, so our observation interfaces may later be called (i.e.
> use-after-free).  
> 
> The canonical model is to keep a copy of a pointer to the DownloadItem around,
> and be sensitive with an update to the DownloadItem state to REMOVING,
which'll
> always occur before the DownloadItem is destroyed.

Ah, I see. A GetDownloadItem method that returned the download regardless of
what state it's in would be useful. 

I updated the WebstoreInstaller to hold a pointer to the DownloadItem and use
that in the destructor. (We already remove the observer when we get these
notifications -- CANCELLED, INTERRUPTED, REMOVING, COMPLETE.)

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:222: if (item)
On 2012/03/23 20:57:19, rdsmith wrote:
> YOu should be able to make this a DCHECK; the DI should be available from
> GetActiveDownloadItem at this point.  (And if it's not, that's probably a
> problem for you, so we should find out soon :-}.)

Done. I removed the if but didn't add a CHECK since we'll crash on the next line
anyways.

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.h (right):

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.h:25: class DownloadItem;
On 2012/03/23 20:57:19, rdsmith wrote:
> Why is this forward decl needed if you're including the .h file?

Done.

[Randy Smith (Not in Mondays), 2012-03-23 22:18:29]

LGTM; thanks!

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.cc (right):

https://chromiumcodereview.appspot.com/9837054/diff/4/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:152: item->RemoveObserver(this);
On 2012/03/23 21:52:53, jstritar wrote:
> On 2012/03/23 20:57:19, rdsmith wrote:
> > This scares me.  I'm not certain as to the lifetime of the
WebstoreInstaller,
> > but if the download item completes but is not deleted before the
> > WebStoreInstaller destructor is called, I think the DownloadItem will still
be
> > around but GetActiveDownloadItem() won't return it, so we won't remove
> ourselves
> > as an observer, so our observation interfaces may later be called (i.e.
> > use-after-free).  
> > 
> > The canonical model is to keep a copy of a pointer to the DownloadItem
around,
> > and be sensitive with an update to the DownloadItem state to REMOVING,
> which'll
> > always occur before the DownloadItem is destroyed.
> 
> Ah, I see. A GetDownloadItem method that returned the download regardless of
> what state it's in would be useful. 

You are completely right.  Mea culpa, and I hope to fix that soon.

> 
> I updated the WebstoreInstaller to hold a pointer to the DownloadItem and use
> that in the destructor. (We already remove the observer when we get these
> notifications -- CANCELLED, INTERRUPTED, REMOVING, COMPLETE.)

[jstritar, 2012-03-26 18:00:08]

+binji: can you take a look at chrome/browser/ui/intents for the OWNERS check?

[Mihai Parparita -not on Chrome, 2012-03-26 18:46:49]

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
File chrome/browser/extensions/webstore_installer.cc (right):

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.cc:175: if (extension == NULL &&
installer->expected_id() == id_)
You may also want to compare installer->profile() and profile_, to make sure
that you're getting the notification from the install that you started (in
theory the same extension could be installed at nearly the same time in multiple
profiles).

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.cc:236: case
DownloadItem::REMOVING:
Based on what Randy said, it seems cleaner/safer to null out the download_item_
pointer when you get the REMOVING state, and only then. Otherwise, you're
relying on catching all of the other error states, and if a new error enum value
is added, you won't be clearing the download_item_ pointer.

If in the REMOVING state, you call download_item_->RemoveObserver, and null out
the pointer, then you can get rid of RemoveObservers altogether.

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.cc:288: RemoveObservers();
This may be obsolete because of the comment above, but this call shouldn't be
necessary, the Release() call below will result in this instance being deleted,
which invokes the destructor, which invokes RemoveObservers.

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.cc:296: RemoveObservers();
Ditto.

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
File chrome/browser/extensions/webstore_installer.h (right):

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.h:101: content::DownloadItem*
download_item_;
Add a comment about who owns this instance and what the expected
lifetime/validity of the pointer is.

[binji, 2012-03-26 19:21:05]

On 2012/03/26 18:00:08, jstritar wrote:
> +binji: can you take a look at chrome/browser/ui/intents for the OWNERS check?

lgtm

[jstritar, 2012-03-26 19:53:36]

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
File chrome/browser/extensions/webstore_installer.cc (right):

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.cc:175: if (extension == NULL &&
installer->expected_id() == id_)
On 2012/03/26 18:46:49, Mihai Parparita wrote:
> You may also want to compare installer->profile() and profile_, to make sure
> that you're getting the notification from the install that you started (in
> theory the same extension could be installed at nearly the same time in
multiple
> profiles).

Done. Also switched this to compare the CrxInstaller's download URL to the
DownloadItem's URL, since CrxInstaller::expected_id() is generally not set.

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.cc:236: case
DownloadItem::REMOVING:
On 2012/03/26 18:46:49, Mihai Parparita wrote:
> Based on what Randy said, it seems cleaner/safer to null out the
download_item_
> pointer when you get the REMOVING state, and only then. Otherwise, you're
> relying on catching all of the other error states, and if a new error enum
value
> is added, you won't be clearing the download_item_ pointer.
> 
> If in the REMOVING state, you call download_item_->RemoveObserver, and null
out
> the pointer, then you can get rid of RemoveObservers altogether.
> 

Done.  I also removed the observer in the destructor since WebstoreInstaller can
complete before the DownloadItem goes into the REMOVING state.

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.cc:288: RemoveObservers();
On 2012/03/26 18:46:49, Mihai Parparita wrote:
> This may be obsolete because of the comment above, but this call shouldn't be
> necessary, the Release() call below will result in this instance being
deleted,
> which invokes the destructor, which invokes RemoveObservers.

Done. I also NULL out the delegat to make sure we don't call it twice if they
hold a reference (e.g. Release wouldn't destruct installer).

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.cc:296: RemoveObservers();
On 2012/03/26 18:46:49, Mihai Parparita wrote:
> Ditto.

Done.

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
File chrome/browser/extensions/webstore_installer.h (right):

http://codereview.chromium.org/9837054/diff/9002/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_installer.h:101: content::DownloadItem*
download_item_;
On 2012/03/26 18:46:49, Mihai Parparita wrote:
> Add a comment about who owns this instance and what the expected
> lifetime/validity of the pointer is.

Done.

[Mihai Parparita -not on Chrome, 2012-03-26 22:30:54]

LGTM

[commit-bot: I haz the power, 2012-03-26 22:37:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9837054/9003

[commit-bot: I haz the power, 2012-03-27 04:24:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9837054/9003