Ensure app install is aborted when the install dialog is closed

Ensure app install is aborted when the install dialog is closed

If the user initiates an app install via the app launcher, it is
possible to close the install dialog by clicking on the background.
The app launcher window and modal dialog are destroyed without
cancelling the dialog, therefore keeping the install active.

This patch ensures that ExtensionInstallPrompt::Delegate::
InstallUIAbort() is called in this scenario to allow the delegate
(WebstoreStandaloneInstaller) to abort the install.

BUG=409616
TEST=browser_tests

Committed: https://crrev.com/d0373dd21f2e1690cf484b4655226a54b3737612
Cr-Commit-Position: refs/heads/master@{#294505}

[tmdiep, 2014-09-07 23:55:24]

tmdiep@chromium.org changed reviewers:
+ sky@chromium.org, tapted@chromium.org

[tmdiep, 2014-09-08 00:02:09]

This is a corner case regression and the bug has a video with steps to
reproduce.

When the user clicks on the background, the app list window and its children
(including the modal dialog) are destroyed without invoking any code paths that
eventually call views::DialogDelegate::Cancel(), Accept(), Close() or
OnClosed().  This patch ensures that views::DialogDelegate::OnClosed() is always
called as many subclasses override this function to implement clean up.

Normally ExtensionInstallDialogView::Cancel() would call
WebstoreStandaloneInstaller::InstallUIAbort(). Since this doesn't occur, the
install is not aborted.

I think the correct behavior is to a) not destroy the app list and the modal
dialog when the user clicks on the background, or b) implement plumbing to call
views::DialogClientView::CanClose().
This patch instead implements a low risk fix, as it may need to be merged into
M38.

tapted, sky: PTAL and let me know if there are any problems with this fix.
Thanks.

[tapted, 2014-09-08 07:42:21]

just one question in dialog_client_view.cc really

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:871: if
(sampling_event_.get())
nit: pretty sure Chrome prefers dropping the `.get()` (I guess
ExperienceSamplingEvent suggests .get(), but probably just because it's easier
to describe in a comment)

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
File
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:74:
ExtensionInstallDialogViewTestBase(
nit: explicit

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:94:
};
nit: DISALLOW_COPY_AND_ASSIGN(..). The two new classes below should have it too

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:218:
static_cast<views::DialogDelegateView*>(dialog.get());
I think you can just assign it rather than static_cast to get at the
made-private overrides. There's also a thing in macros.h that would let you do

  implicit_cast<views::DialogDelegateView*>(dialog.get())->Accept();

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:222:
dialog.reset();
Can you let this just go out of scope?

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:224:
ASSERT_EQ(0, delegate.abort_count());
nit: ASSERT->EXPECT. Typically I see ASSERT just for when later code is
guaranteed to crash, otherwise the extra logging in a failure can be handy.

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:253:
delegate_view->OnClosed();
The test might be more compelling if you can do dialog->GetWidget()->CloseNow();
I .. think all the bits are in the test harness for that to work, but the
lifetime of these things is sometimes odd in tests.

https://codereview.chromium.org/547253002/diff/1/ui/views/window/dialog_clien...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/547253002/diff/1/ui/views/window/dialog_clien...
ui/views/window/dialog_client_view.cc:143: if (!notified_delegate_)
do you need `notified_delegate_ = true` here, and in DialogClientView::Close(),
below?

[tmdiep, 2014-09-08 08:26:08]

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:871: if
(sampling_event_.get())
On 2014/09/08 07:42:20, tapted wrote:
> nit: pretty sure Chrome prefers dropping the `.get()` (I guess
> ExperienceSamplingEvent suggests .get(), but probably just because it's easier
> to describe in a comment)

Done.

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
File
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:74:
ExtensionInstallDialogViewTestBase(
On 2014/09/08 07:42:20, tapted wrote:
> nit: explicit

Done.

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:94:
};
On 2014/09/08 07:42:21, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..). The two new classes below should have it
too

Done.

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:218:
static_cast<views::DialogDelegateView*>(dialog.get());
On 2014/09/08 07:42:20, tapted wrote:
> I think you can just assign it rather than static_cast to get at the
> made-private overrides. There's also a thing in macros.h that would let you do
> 
>   implicit_cast<views::DialogDelegateView*>(dialog.get())->Accept();

Done.

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:222:
dialog.reset();
On 2014/09/08 07:42:20, tapted wrote:
> Can you let this just go out of scope?

I wanted to make sure that the dialog is completely destroyed before checking
the proceed and abort counts (in case something gets inserted in the destructor
in the future).

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:224:
ASSERT_EQ(0, delegate.abort_count());
On 2014/09/08 07:42:20, tapted wrote:
> nit: ASSERT->EXPECT. Typically I see ASSERT just for when later code is
> guaranteed to crash, otherwise the extra logging in a failure can be handy.

Done.

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:253:
delegate_view->OnClosed();
On 2014/09/08 07:42:20, tapted wrote:
> The test might be more compelling if you can do
dialog->GetWidget()->CloseNow();
> I .. think all the bits are in the test harness for that to work, but the
> lifetime of these things is sometimes odd in tests.

Yeah, I agree that it would be a better test of the scenario. But to avoid the
risk of a flaky test, maybe I'll just leave as is.

https://codereview.chromium.org/547253002/diff/1/ui/views/window/dialog_clien...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/547253002/diff/1/ui/views/window/dialog_clien...
ui/views/window/dialog_client_view.cc:143: if (!notified_delegate_)
On 2014/09/08 07:42:21, tapted wrote:
> do you need `notified_delegate_ = true` here, and in
DialogClientView::Close(),
> below?

DialogClientView::Close() is called by AcceptWindow() and CancelWindow(), where
notified_delegate_ is set to true.

I went back and forth between whether to set the flag to true here.
The comment for notified_delegate_ in the header file says: "True if we've
notified the delegate the window is closing and the delegate allowed the close."
In this case the delegate did not allow the close, but the window is closing, so
I opted for this to ensure this flow remains unchanged:
When there is an accept, the order of execution is line 71 -> 429 -> 129
(returns at 130) -> 143 -> 430.

[tapted, 2014-09-08 09:28:56]

lgtm

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
File
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/547253002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:253:
delegate_view->OnClosed();
On 2014/09/08 08:26:08, tmdiep wrote:
> On 2014/09/08 07:42:20, tapted wrote:
> > The test might be more compelling if you can do
> dialog->GetWidget()->CloseNow();
> > I .. think all the bits are in the test harness for that to work, but the
> > lifetime of these things is sometimes odd in tests.
> 
> Yeah, I agree that it would be a better test of the scenario. But to avoid the
> risk of a flaky test, maybe I'll just leave as is.

CloseNow usually avoids being flaky (Widget::Close needs a runloop, so needs
care, but is otherwise safe from flakiness too).

I guess you're not really getting much extra coverage though.

https://codereview.chromium.org/547253002/diff/1/ui/views/window/dialog_clien...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/547253002/diff/1/ui/views/window/dialog_clien...
ui/views/window/dialog_client_view.cc:143: if (!notified_delegate_)
On 2014/09/08 08:26:08, tmdiep wrote:
> On 2014/09/08 07:42:21, tapted wrote:
> > do you need `notified_delegate_ = true` here, and in
> DialogClientView::Close(),
> > below?
> 
> DialogClientView::Close() is called by AcceptWindow() and CancelWindow(),
where
> notified_delegate_ is set to true.
> 
> I went back and forth between whether to set the flag to true here.
> The comment for notified_delegate_ in the header file says: "True if we've
> notified the delegate the window is closing and the delegate allowed the
close."
> In this case the delegate did not allow the close, but the window is closing,
so
> I opted for this to ensure this flow remains unchanged:
> When there is an accept, the order of execution is line 71 -> 429 -> 129
> (returns at 130) -> 143 -> 430. 

sg - so long as Close() is private/non-virtual I think it makes sense

[sky, 2014-09-08 16:19:23]

Are you sure this doesn't require updating any existing subclasses of OnClosed?
I would prefer you separate out the views change into its own patch and add test
coverage.

[littleone691_gmail.com, 2014-09-08 16:21:03]

On Mon, Sep 8, 2014 at 12:19 PM, <sky@chromium.org> wrote:

> Are you sure this doesn't require updating any existing subclasses of
> OnClosed?
> I would prefer you separate out the views change into its own patch and
> add test
> coverage.
>

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

[tmdiep, 2014-09-09 00:17:08]

Patchset #3 (id:40001) has been deleted

[tmdiep, 2014-09-09 00:23:48]

tmdiep@chromium.org changed reviewers:
- littleone691@gmail.com

[tmdiep, 2014-09-09 00:37:57]

On 2014/09/08 16:19:23, sky wrote:
> Are you sure this doesn't require updating any existing subclasses of
OnClosed?
> I would prefer you separate out the views change into its own patch and add
test
> coverage.

I've updated the patch to implement a minimalist fix for this regression (as it
is a stable blocker). The install dialog now ensures the delegate is notified of
the abort in its destructor.

I'll log a bug to decide on the correct solution for dismissing modal dialogs
when the app launcher is deactivated.

[tapted, 2014-09-10 12:55:28]

I think I liked the idea of using WidgetClosing more, but I don't think there's
anything actually wrong with patchset 3.. I can't help with OWNERS approval, but
I'd probably ask the expert anyway :)

Perhaps it's the case that this install dialog really is special, in that it's
shown on the app launcher, which is more likely to close its child dialogs
algorithmically. However, I think there's still a concern that other dialogs
could be similarly afflicted, and would benefit from a general fix.

[tmdiep, 2014-09-10 23:53:14]

tmdiep@chromium.org changed reviewers:
+ finnur@chromium.org

[tmdiep, 2014-09-10 23:56:35]

On 2014/09/10 12:55:28, tapted wrote:
> I think I liked the idea of using WidgetClosing more, but I don't think
there's
> anything actually wrong with patchset 3.. I can't help with OWNERS approval,
but
> I'd probably ask the expert anyway :)
> 
> Perhaps it's the case that this install dialog really is special, in that it's
> shown on the app launcher, which is more likely to close its child dialogs
> algorithmically. However, I think there's still a concern that other dialogs
> could be similarly afflicted, and would benefit from a general fix.

+finnur, who is a direct owner of the install dialog.

I raised crbug.com/413031 for a more general fix for all modal dialogs. I think
it will require some discussion, whereas the regression for the install dialog
is a little more urgent to fix.

[Finnur, 2014-09-11 09:33:52]

As a quick fix this LGTM.

[tmdiep, 2014-09-11 21:15:30]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-09-11 21:31:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/547253002/60001

[commit-bot: I haz the power, 2014-09-12 00:44:08]

Committed patchset #3 (id:60001) as 53bd41732fc06093775e791ecbb0058885f6f6e8

[commit-bot: I haz the power, 2014-09-12 00:51:46]

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