Don't allow inline install if frame is deleted before user accepts

Don't allow inline install if frame is deleted before user accepts

If the frame that called the chrome.webstore.install method to begin an
inline install gets deleted before the user accepts from the dialog, we
don't want the install to continue because a navigation could make it
look like the install request was coming from some unrelated site.

One downside of this approach is that the dialog stays around even after
the frame is deleted, and hitting either accept or cancel buttons both
just cancel the install. It would be better if the dialog is
automatically cancelled, but doing that would involve a lot more
refactoring. The approach in this CL was easier and is probably worth
getting out, and we can improve on it in the future.

BUG=550047
Committed: https://crrev.com/bbe84115d3dc969bfcf6ca87bebd1f5608db6ecf
Cr-Commit-Position: refs/heads/master@{#361218}

[asargent_no_longer_on_chrome, 2015-11-05 01:17:02]

asargent@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[asargent_no_longer_on_chrome, 2015-11-05 01:17:02]

PTAL

[Devlin, 2015-11-05 17:40:40]

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer.cc:180: void
WebstoreInlineInstaller::FrameDeleted(
There's been a ton of confusion over which [Render]Frame methods on
WebContentsObserver to use in which situations, and there's still some bugs/open
questions.  Luckily, I think there's an easier solution in this case, since we
don't care about the frames' existence, but rather whether or not they're
navigated.  Could we instead just do
void WebstoreInlineInstaller::DidNavigateAnyFrame(
    RenderFrameHost* render_frame_host,
    const LoadCommittedDetails& details,
    const FrameNavigateParams& params)) {
  if (!details.is_in_page && (render_frame_host == host_ ||
details.is_main_frame))
    host_ == nullptr;
}

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:41: class
InstallerFactoryObserver {
nit: I think this is a little more than we need.  Can the test factory itself
just store the last installer, and we can keep a raw ptr to the factory in the
test?

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:124: return
WebstoreInlineInstaller::CheckRequestorAlive();
Is this needed?

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:199:
EXPECT_FALSE(last_installer_->CheckRequestorAlive());
Can we also test inline installation from a frame, and navigating the frame? 
(If it seems like it'll be a pain, don't worry about it.)

[asargent_no_longer_on_chrome, 2015-11-18 21:42:44]

ok, this is ready for another look

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer.cc:180: void
WebstoreInlineInstaller::FrameDeleted(
On 2015/11/05 17:40:40, Devlin wrote:
> There's been a ton of confusion over which [Render]Frame methods on
> WebContentsObserver to use in which situations, and there's still some
bugs/open
> questions.  Luckily, I think there's an easier solution in this case, since we
> don't care about the frames' existence, but rather whether or not they're
> navigated.  Could we instead just do
> void WebstoreInlineInstaller::DidNavigateAnyFrame(
>     RenderFrameHost* render_frame_host,
>     const LoadCommittedDetails& details,
>     const FrameNavigateParams& params)) {
>   if (!details.is_in_page && (render_frame_host == host_ ||
> details.is_main_frame))
>     host_ == nullptr;
> }

Yep, that works.

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:41: class
InstallerFactoryObserver {
On 2015/11/05 17:40:40, Devlin wrote:
> nit: I think this is a little more than we need.  Can the test factory itself
> just store the last installer, and we can keep a raw ptr to the factory in the
> test?

Much simpler, I like it. Done.

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:124: return
WebstoreInlineInstaller::CheckRequestorAlive();
On 2015/11/05 17:40:40, Devlin wrote:
> Is this needed?

It's just needed to make the method publicly visible so the test can call it
(it's a protected method of WebstoreInlineInstaller). 

Added a comment to this effect.

https://codereview.chromium.org/1403293008/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:199:
EXPECT_FALSE(last_installer_->CheckRequestorAlive());
On 2015/11/05 17:40:40, Devlin wrote:
> Can we also test inline installation from a frame, and navigating the frame? 
> (If it seems like it'll be a pain, don't worry about it.)

I wrote up a test for it, and in the process of trying to get the test working I
found out that we don't allow calling the API from frames! =)

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

[Devlin, 2015-11-18 21:48:26]

lgtm

https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:131: //
Returns a pointer to the last installer that was created.
nitty nit: I think it's more common to put the description above the member, and
just not document the accessor.

https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:145:
protected:
nit: Let's do private (unless there's a reason for protected).

https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:184:
EXPECT_FALSE(factory->last_installer()->CheckRequestorAlive());
Since we don't cancel the installation here, can we also test calling
ProgrammableInstallPrompt::Accept() and ensuring the extension doesn't get
installed?

[asargent_no_longer_on_chrome, 2015-11-20 01:11:48]

Addressed comments - the last one required adding a bit of extra code to the
test - mind having another quick look at
webstore_inline_installer_browsertest.cc to double-check the sanity of what I've
done?

https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:131: //
Returns a pointer to the last installer that was created.
On 2015/11/18 21:48:26, Devlin wrote:
> nitty nit: I think it's more common to put the description above the member,
and
> just not document the accessor.

Done.

https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:145:
protected:
On 2015/11/18 21:48:26, Devlin wrote:
> nit: Let's do private (unless there's a reason for protected).

Done.

https://codereview.chromium.org/1403293008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:184:
EXPECT_FALSE(factory->last_installer()->CheckRequestorAlive());
On 2015/11/18 21:48:26, Devlin wrote:
> Since we don't cancel the installation here, can we also test calling
> ProgrammableInstallPrompt::Accept() and ensuring the extension doesn't get
> installed?

Good suggestion - I ended up having to add a bit of extra code to the test
framework to accomplish this.

[Devlin, 2015-11-20 01:27:19]

still lgtm

https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:116: bool
success;
nitty nit: please initialize these (feel free to use default member
initialization to make it fewer lines).

https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:137: static
InstallResult* g_last_install_result_;
nitty nit: This isn't entirely POD (because of the string), and is never garbage
collected.  Does it matter in a test?  Probably not.  But is there a reason not
to have it as a non-static member on the test itself, either a scoped_ptr or
just a default-initialized struct?

[asargent_no_longer_on_chrome, 2015-11-20 23:56:46]

thanks for review, sending to CQ now

https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:116: bool
success;
On 2015/11/20 01:27:19, Devlin wrote:
> nitty nit: please initialize these (feel free to use default member
> initialization to make it fewer lines).

Done.

https://codereview.chromium.org/1403293008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:137: static
InstallResult* g_last_install_result_;
On 2015/11/20 01:27:19, Devlin wrote:
> nitty nit: This isn't entirely POD (because of the string), and is never
garbage
> collected.  Does it matter in a test?  Probably not.  But is there a reason
not
> to have it as a non-static member on the test itself, either a scoped_ptr or
> just a default-initialized struct?

Yeah, that was probably a little too fast and loose. I've switched it to let
consumers of WebstoreInlineInstallerForTest specify a scoped_ptr to write this
into.

[asargent_no_longer_on_chrome, 2015-11-20 23:57:32]

Description was changed from

==========
Don't allow inline install if frame is deleted before user accepts

If the frame that called the chrome.webstore.install method to begin an
inline install gets deleted before the user accepts from the dialog, we
don't want the install to continue because a navigation could make it
look like the install request was coming from some unrelated site.

One downside of this approach is that the dialog stays around even after
the frame is deleted, and hitting either accept or cancel buttons both
just cancel the install. It would be better if the dialog is
automatically cancelled, but doing that would involve a lot more
refactoring. The approach in this CL was easier and is probably worth
getting out, and we can improve on it in the future.

BUG=550047
==========

to

==========
Don't allow inline install if frame is deleted before user accepts

If the frame that called the chrome.webstore.install method to begin an
inline install gets deleted before the user accepts from the dialog, we
don't want the install to continue because a navigation could make it
look like the install request was coming from some unrelated site.

One downside of this approach is that the dialog stays around even after
the frame is deleted, and hitting either accept or cancel buttons both
just cancel the install. It would be better if the dialog is
automatically cancelled, but doing that would involve a lot more
refactoring. The approach in this CL was easier and is probably worth
getting out, and we can improve on it in the future.

BUG=550047
==========

[asargent_no_longer_on_chrome, 2015-11-20 23:57:40]

The CQ bit was checked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2015-11-20 23:57:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1403293008/#ps60001
(title: "better way of handling install result checking")

[commit-bot: I haz the power, 2015-11-20 23:58:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403293008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403293008/60001

[commit-bot: I haz the power, 2015-11-21 00:15:20]

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

[commit-bot: I haz the power, 2015-11-21 00:15:21]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[asargent_no_longer_on_chrome, 2015-11-23 21:35:03]

The CQ bit was checked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2015-11-23 21:35:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1403293008/#ps80001
(title: "fix unit test compile failure")

[commit-bot: I haz the power, 2015-11-23 21:39:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403293008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403293008/80001

[commit-bot: I haz the power, 2015-11-23 23:40:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-23 23:41:05]

Patchset 5 (id:??) landed as
https://crrev.com/bbe84115d3dc969bfcf6ca87bebd1f5608db6ecf
Cr-Commit-Position: refs/heads/master@{#361218}