Fixed leaks of WebstoreStandaloneInstallers in some code paths

Fixed leaks of WebstoreStandaloneInstallers in some code paths

Simplifies the sequence of AddRef() and Release() to avoid leaks
due to unbalanced Releases.

BUG=378606
TEST=browser_tests
Manual: Test installing apps via the app launcher and inline
installation to ensure there are no regressions.

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

[tmdiep, 2014-05-30 00:44:08]

I noticed this problem while testing https://codereview.chromium.org/298303002.
Please see the bug for more details. If this is a risky change, I can implement
something safer to solve the problem. But I think this patch will simplify the
code and hopefully avoid a regression in the future.

Thanks in advance.

[asargent_no_longer_on_chrome, 2014-05-30 05:03:03]

https://codereview.chromium.org/298133007/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/298133007/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_standalone_installer.cc:262: this,
Inside OnWebstoreParseSuccess, we can end up calling into InstallUIProceed here,
and it looks like we then pass a naked pointer to ourself as the delegate to
this new WebstoreInstaller. Since you removed the AddRef on line 217, doesn't
that mean that after exiting this method at line 268 that the reference count
will reach 0 and the WebstoreInstaller will be left with a stale pointer?

[tmdiep, 2014-05-30 05:34:32]

https://codereview.chromium.org/298133007/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/298133007/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_standalone_installer.cc:262: this,
On 2014/05/30 05:03:03, Antony Sargent wrote:
> Inside OnWebstoreParseSuccess, we can end up calling into InstallUIProceed
here,
> and it looks like we then pass a naked pointer to ourself as the delegate to
> this new WebstoreInstaller. Since you removed the AddRef on line 217, doesn't
> that mean that after exiting this method at line 268 that the reference count
> will reach 0 and the WebstoreInstaller will be left with a stale pointer?

WebstoreStandaloneInstaller::BeginInstall() starts with AddRef() and I traced
all the paths of execution to CompleteInstall(). As long as use of this object
always starts with BeginInstall() and ends in CompleteInstall(), the ref count
should always be 1 in between. Let me know if I've missed something.

I can add a DCHECK(HasOneRef()) at key locations to ensure this is the case.

[tmdiep, 2014-05-30 06:25:26]

https://codereview.chromium.org/298133007/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/298133007/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_standalone_installer.cc:262: this,
On 2014/05/30 05:34:32, tmdiep wrote:
> I can add a DCHECK(HasOneRef()) at key locations to ensure this is the case.

I added this dcheck before creation of the install prompt, WebstoreInstallHelper
and WebstoreInstaller, and before the Release in CompleteInstall(). It always
holds. I can't check this in though, in case clients or subclasses call
additional AddRefs and the ref count is not exactly one.

[asargent_no_longer_on_chrome, 2014-06-03 23:18:35]

Oops, sorry this fell off my radar. 

lgtm

[tmdiep, 2014-06-04 01:14:51]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-04 01:16:30]

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

[tmdiep, 2014-06-04 04:19:02]

The CQ bit was unchecked by tmdiep@chromium.org

[tmdiep, 2014-06-04 11:24:11]

After analyzing the inline install test failure, I see what you meant earlier -
sorry! If the web contents is destroyed while the dialog is shown,
AbortInstall() will call Release(), but we want to ensure the installer is alive
until the dialog is closed (or while any other async operation is in progress).

I've fixed this case in patch set 3.

[asargent_no_longer_on_chrome, 2014-06-05 05:23:00]

lgtm
On Jun 4, 2014 4:24 AM, <tmdiep@chromium.org> wrote:

> After analyzing the inline install test failure, I see what you meant
> earlier -
> sorry! If the web contents is destroyed while the dialog is shown,
> AbortInstall() will call Release(), but we want to ensure the installer is
> alive
> until the dialog is closed (or while any other async operation is in
> progress).
>
> I've fixed this case in patch set 3.
>
> https://codereview.chromium.org/298133007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[asargent_no_longer_on_chrome, 2014-06-05 05:24:55]

lgtm

[tmdiep, 2014-06-05 22:01:09]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-05 22:03:22]

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

[commit-bot: I haz the power, 2014-06-05 22:54:54]

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

[commit-bot: I haz the power, 2014-06-05 23:19:55]

Change committed as 275270