Fix WebstoreInlineInstaller ASAN bugs

Fix WebstoreInlineInstaller ASAN bugs

Certain code paths which called back onto a raw pointer to a
WebstoreInlineInstaller could not guarantee the object's existence at
callback time.

Additional ref counts have been added to ensure that this is no longer the case.

BUG=236513, 241431, 241432, 247774, 263004
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228239

[Ken Rockot(use gerrit already), 2013-09-26 21:16:01]

PTAL!

[Ken Rockot(use gerrit already), 2013-09-26 21:26:42]

https://codereview.chromium.org/24839002/diff/1/chrome/browser/extensions/web...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/24839002/diff/1/chrome/browser/extensions/web...
chrome/browser/extensions/webstore_standalone_installer.cc:225: 
FYI: I realize it is redundant to Add and Release side by side. I feel it makes
the intentions clearer, but I suppose this could just be addressed with comments
instead.

[asargent_no_longer_on_chrome, 2013-09-27 22:00:19]

The code changes seem fine. Can you add a browser test that would crash if this
code change wasn't in place? I was thinking maybe we could start up an inline
install and then use the browser test utils stuff to kill the tab (and possibly 
close the confirmation dialog too?).

https://codereview.chromium.org/24839002/diff/1/chrome/browser/extensions/web...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/24839002/diff/1/chrome/browser/extensions/web...
chrome/browser/extensions/webstore_standalone_installer.cc:225: 
On 2013/09/26 21:26:42, rockot wrote:
> FYI: I realize it is redundant to Add and Release side by side. I feel it
makes
> the intentions clearer, but I suppose this could just be addressed with
comments
> instead.

I think I have a slight preference for not calling AddRef/Release in this
function, and putting a comment just above this block (or the call to
installer->Start) along the lines of:

"Control flow will finish in either OnExtensionInstallSuccess or 
OnExtensionInstallFailure"

My reasoning is that I found it a little easier to think about things from the
perspective of "ShowInstallUI starts some asynchronous work to display a dialog
and *if* the user clicks ok, continues to try doing the install". That is, at
the start of a set of operations we AddRef once to keep ourself alive, and then
only Release in leaf execution nodes when we know we're done with ourself, as
opposed to AddRef/Release pairs for each step of sub-operations. 

I don't think it's that big of a deal though - if you find yourself still liking
it the current way you have it here, I'm ok with it. (Maybe consider running it
past one other person to get a tiebreaker opinion?)

[Ken Rockot(use gerrit already), 2013-10-08 00:45:52]

Added a test which will crash when ref count management is broken as it was on
WebstoreStandaloneInstaller.

A great way to repro the original bug was to close a tab while it had an inline
install prompt showing, then respond to the dialog. Unfortunately triggering a
dialog response after closing the WebContents would have been incredibly
difficult due to implementation details of the prompt we have.

The test code can be cleaned up further, just looking for LG/no-LG on the
general approach of adding a WebstoreInlineInstaller factory to customize
implementation for testing.

https://codereview.chromium.org/24839002/diff/1/chrome/browser/extensions/web...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/24839002/diff/1/chrome/browser/extensions/web...
chrome/browser/extensions/webstore_standalone_installer.cc:225: 
It's OK, I already kind of convinced myself to take them out and comment things
appropriately.

[asargent_no_longer_on_chrome, 2013-10-08 22:08:34]

I think this approach is ok. Here's another possibility that came to mind though
- give it some thought and see if you like it or not: 

Instead of having a WebstoreInstallerFactory, we could have WebstoreInstaller
take an optional CreateInstallUIDelegate. In WebstoreInstaller::CreateInstallUI,
if that delegate has been set, then delegate the creation of the install UI to
it. 

It seems like this might result in slightly less code to write in your test
class - you would still have ProgrammableInstallPrompt, but would not need
WebstoreInstallerFactory or WebstoreInlineInstallerForTestFactory, and
CreateInstallUIDelegate interface would be really simple:

class CreateInstallUIDelegate {
 public:
  virtual ~CreateInstallUIDelegate() {}
  virtual scoped_ptr<ExtensionInstallPrompt> CreateInstallUI() = 0    
};

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/tab_helper.cc:114: 
Any reason not to put this in the initializer list before the start of the
method?

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/tab_helper.h:162: void
SetWebstoreInlineInstallerFactory(
nit: add the suffix "ForTests" to the method name - this both serves as a hint
to people not to call it in normal code, but also gets checked by a presubmit
script that it isn't being called in any .cc file which doesn't have "test" in
it's name (eg _unittest.cc, _browsertest.cc, etc.)

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:93: };
Some of the code in here seems sort of familiar - any chance it was cribbed from
another InProcessBrowserTest or something? If so is there any easy opportunity
for extracting out a common intermediate class that multiple uses can inherit
from?

[Ken Rockot(use gerrit already), 2013-10-09 17:12:48]

To persist what we discussed offline, giving a UI-creation-callback to the
inline installer would still require adding something like
TabHelper::SetWebstoreInlineInstallerCreateUICallbackForTests since TabHelper
actually constructs the installer. The installer factory seemed like a better
option.

At the suggestion of asargent, +CC mpcomplete to solicit any alternative
approach for this.

To clarify, we want a fake install confirmation dialog for testing so it can be
programmatically accepted/rejected at a later time. The existing machinery for
tests is only capable of bypassing the prompts altogether, which is not
sufficient.

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/tab_helper.cc:114: 
On 2013/10/08 22:08:34, Antony Sargent wrote:
> Any reason not to put this in the initializer list before the start of the
> method? 

Nope. Done.

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/tab_helper.h:162: void
SetWebstoreInlineInstallerFactory(
On 2013/10/08 22:08:34, Antony Sargent wrote:
> nit: add the suffix "ForTests" to the method name - this both serves as a hint
> to people not to call it in normal code, but also gets checked by a presubmit
> script that it isn't being called in any .cc file which doesn't have "test" in
> it's name (eg _unittest.cc, _browsertest.cc, etc.)

Done.

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/24839002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:93: };
On 2013/10/08 22:08:34, Antony Sargent wrote:
> Some of the code in here seems sort of familiar - any chance it was cribbed
from
> another InProcessBrowserTest or something? If so is there any easy opportunity
> for extracting out a common intermediate class that multiple uses can inherit
> from?

Done.

[Matt Perry, 2013-10-09 21:01:26]

The factory approach SGTM. I don't have any better ideas. The AddRef/Release
tangle is a bit scary, though.

https://codereview.chromium.org/24839002/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/24839002/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/webstore_standalone_installer.cc:240: Release();  //
Balanced in InstallUIProceed.
This comment doesn't seem accurate.

[Ken Rockot(use gerrit already), 2013-10-09 21:10:16]

On 2013/10/09 21:01:26, Matt Perry wrote:
> The factory approach SGTM. I don't have any better ideas. The AddRef/Release
> tangle is a bit scary, though.
> 

Thanks. Leaving the test as-is for now.

The tangle is scary. Working now, but obviously fragile. I will take a pass at
getting rid of all the raw-pointer-as-delegate usage riddling the code and
hopefully there is a world without manual ref management on the other side.

>
https://codereview.chromium.org/24839002/diff/18001/chrome/browser/extensions...
> File chrome/browser/extensions/webstore_standalone_installer.cc (right):
> 
>
https://codereview.chromium.org/24839002/diff/18001/chrome/browser/extensions...
> chrome/browser/extensions/webstore_standalone_installer.cc:240: Release();  //
> Balanced in InstallUIProceed.
> This comment doesn't seem accurate.

You're right, it's outdated.

[asargent_no_longer_on_chrome, 2013-10-10 05:27:38]

lgtm

https://codereview.chromium.org/24839002/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/24839002/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/tab_helper.h:18: #include
"chrome/browser/extensions/webstore_inline_installer_factory.h"
nit: can you move this include to the .cc file and just fwd declare
WebstoreInlineInstallerFactory in this header?

[commit-bot: I haz the power, 2013-10-11 05:53:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/24839002/28001

[commit-bot: I haz the power, 2013-10-11 06:34:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-11 14:36:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/24839002/52001

[commit-bot: I haz the power, 2013-10-11 20:12:03]

Change committed as 228239