Simplify ExtensionPopup widget closing behavior.

Simplify ExtensionPopup widget closing behavior.

Fix ExtensionPopup::OnWidgetActivationChanged crashes.
(Issue 327776 shows GetWidget() may be NULL in rare cases)

Simply close extension popups on anchor window activation.
(the previous complex logic was just trying to achieve this)

Cleanup, remove USE_AURA preprocessor conditions.

Add BrowserActionInteractiveTest.DestroyHWNDDoesNotCrash.
Add some helper functions and do minor cleanup.

BUG=327776, 320889, 179786, 106723
TEST=No Extension popup behavior changes. Extension popup bubbles close when clicking the browser window in Windows classic, glass, and Metro Ash. (no close on JS alerts from extension popups, no close on inspect popup, expected close on clicking away from the bubble, etc. on Win Classic/Aero/Ash, ChromeOS, Linux Desktop Aura).

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

[msw, 2014-05-07 03:03:34]

Hey Scott, please take a look; thanks!

[sky, 2014-05-07 13:24:47]

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_popup.cc:150: if (GetWidget() &&
!inspect_with_devtools_ &&
If GetWidget() returns NULL, who ends up deleting this class?

[msw, 2014-05-07 18:12:49]

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_popup.cc:150: if (GetWidget() &&
!inspect_with_devtools_ &&
On 2014/05/07 13:24:47, sky wrote:
> If GetWidget() returns NULL, who ends up deleting this class?

Good question, I assume the bubble is being closed in another manner, and as a
WidgetDelegateView subclass, OnNativeWidgetDestroyed will call DeleteDelegate to
delete this instance.

I suspect that both activation functions are being called here, one after the
other, or perhaps one function is called twice. The first Widget::Close removes
this View and then the second activation function call finds that GetWidget()
returns NULL. I could guard against that more rigorously by removing this
instance as observers before calling Close and restoring CHECK(GetWidget()),
then hope crash rates drop (from the 14 reports on stable). I don't think it's
worthwhile, but would appreciate your advice.

[sky, 2014-05-07 19:38:30]

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_popup.cc:150: if (GetWidget() &&
!inspect_with_devtools_ &&
On 2014/05/07 18:12:49, msw wrote:
> On 2014/05/07 13:24:47, sky wrote:
> > If GetWidget() returns NULL, who ends up deleting this class?
> 
> Good question, I assume the bubble is being closed in another manner, and as a
> WidgetDelegateView subclass, OnNativeWidgetDestroyed will call DeleteDelegate
to
> delete this instance.
> 
> I suspect that both activation functions are being called here, one after the
> other, or perhaps one function is called twice. The first Widget::Close
removes
> this View

This class should only be removed when the widget is destroyed, right? After
right after that this is deleted. So, I guess I don't see how we could end up
with no parent for any length of time that would trigger the crash.

> and then the second activation function call finds that GetWidget()
> returns NULL. I could guard against that more rigorously by removing this
> instance as observers before calling Close and restoring CHECK(GetWidget()),
> then hope crash rates drop (from the 14 reports on stable). I don't think it's
> worthwhile, but would appreciate your advice.

[msw, 2014-05-07 22:14:56]

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_popup.cc:150: if (GetWidget() &&
!inspect_with_devtools_ &&
On 2014/05/07 19:38:30, sky wrote:
> On 2014/05/07 18:12:49, msw wrote:
> > On 2014/05/07 13:24:47, sky wrote:
> > > If GetWidget() returns NULL, who ends up deleting this class?
> > 
> > Good question, I assume the bubble is being closed in another manner, and as
a
> > WidgetDelegateView subclass, OnNativeWidgetDestroyed will call
DeleteDelegate
> to
> > delete this instance.
> > 
> > I suspect that both activation functions are being called here, one after
the
> > other, or perhaps one function is called twice. The first Widget::Close
> removes
> > this View
> 
> This class should only be removed when the widget is destroyed, right? After
> right after that this is deleted. So, I guess I don't see how we could end up
> with no parent for any length of time that would trigger the crash.
> 
> > and then the second activation function call finds that GetWidget()
> > returns NULL. I could guard against that more rigorously by removing this
> > instance as observers before calling Close and restoring CHECK(GetWidget()),
> > then hope crash rates drop (from the 14 reports on stable). I don't think
it's
> > worthwhile, but would appreciate your advice.
> 

Widget::Close posts a callback to actually destroy the widget, but GetWidget()
is valid even through the ExtensionPopup dtor. You're right, it doesn't seem
like an activation change could occur during the synchronous CloseNow process
where the WidgetDelegate is actually deleted. It also doesn't seem possible for
an activation event to be handled by the class before the widget exists, so
consider me equally confused :-/

I could leave the CHECKs in place for now (instead of the conditional) to
continue tracking crashes, but their low report volume doesn't seem to warrant
significant additional investigation at the moment.

[sky, 2014-05-07 22:38:44]

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_popup.cc:150: if (GetWidget() &&
!inspect_with_devtools_ &&
On 2014/05/07 22:14:56, msw wrote:
> On 2014/05/07 19:38:30, sky wrote:
> > On 2014/05/07 18:12:49, msw wrote:
> > > On 2014/05/07 13:24:47, sky wrote:
> > > > If GetWidget() returns NULL, who ends up deleting this class?
> > > 
> > > Good question, I assume the bubble is being closed in another manner, and
as
> a
> > > WidgetDelegateView subclass, OnNativeWidgetDestroyed will call
> DeleteDelegate
> > to
> > > delete this instance.
> > > 
> > > I suspect that both activation functions are being called here, one after
> the
> > > other, or perhaps one function is called twice. The first Widget::Close
> > removes
> > > this View
> > 
> > This class should only be removed when the widget is destroyed, right? After
> > right after that this is deleted. So, I guess I don't see how we could end
up
> > with no parent for any length of time that would trigger the crash.
> > 
> > > and then the second activation function call finds that GetWidget()
> > > returns NULL. I could guard against that more rigorously by removing this
> > > instance as observers before calling Close and restoring
CHECK(GetWidget()),
> > > then hope crash rates drop (from the 14 reports on stable). I don't think
> it's
> > > worthwhile, but would appreciate your advice.
> > 
> 
> Widget::Close posts a callback to actually destroy the widget, but GetWidget()
> is valid even through the ExtensionPopup dtor. You're right, it doesn't seem
> like an activation change could occur during the synchronous CloseNow process
> where the WidgetDelegate is actually deleted. It also doesn't seem possible
for
> an activation event to be handled by the class before the widget exists, so
> consider me equally confused :-/
> 
> I could leave the CHECKs in place for now (instead of the conditional) to
> continue tracking crashes, but their low report volume doesn't seem to warrant
> significant additional investigation at the moment.

If we agree the widget should never be NULL, what about adding a
CHECK(GetWidget()) in ViewHierarchyChanged? Maybe that would tell us how we're
ending up with a NULL widget. The CHECK may need to be conditional on having
been added to a widget first.

Other thought, do we do the right thing if the NativeWidget is deleted out from
the under the widget?

[msw, 2014-05-13 22:44:21]

Please take another look; thanks!

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

https://codereview.chromium.org/268143007/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_popup.cc:150: if (GetWidget() &&
!inspect_with_devtools_ &&
On 2014/05/07 22:38:44, sky wrote:
> On 2014/05/07 22:14:56, msw wrote:
> > On 2014/05/07 19:38:30, sky wrote:
> > > On 2014/05/07 18:12:49, msw wrote:
> > > > On 2014/05/07 13:24:47, sky wrote:
> > > > > If GetWidget() returns NULL, who ends up deleting this class?
> > > > 
> > > > Good question, I assume the bubble is being closed in another manner,
and
> as
> > a
> > > > WidgetDelegateView subclass, OnNativeWidgetDestroyed will call
> > DeleteDelegate
> > > to
> > > > delete this instance.
> > > > 
> > > > I suspect that both activation functions are being called here, one
after
> > the
> > > > other, or perhaps one function is called twice. The first Widget::Close
> > > removes
> > > > this View
> > > 
> > > This class should only be removed when the widget is destroyed, right?
After
> > > right after that this is deleted. So, I guess I don't see how we could end
> up
> > > with no parent for any length of time that would trigger the crash.
> > > 
> > > > and then the second activation function call finds that GetWidget()
> > > > returns NULL. I could guard against that more rigorously by removing
this
> > > > instance as observers before calling Close and restoring
> CHECK(GetWidget()),
> > > > then hope crash rates drop (from the 14 reports on stable). I don't
think
> > it's
> > > > worthwhile, but would appreciate your advice.
> > > 
> > 
> > Widget::Close posts a callback to actually destroy the widget, but
GetWidget()
> > is valid even through the ExtensionPopup dtor. You're right, it doesn't seem
> > like an activation change could occur during the synchronous CloseNow
process
> > where the WidgetDelegate is actually deleted. It also doesn't seem possible
> for
> > an activation event to be handled by the class before the widget exists, so
> > consider me equally confused :-/
> > 
> > I could leave the CHECKs in place for now (instead of the conditional) to
> > continue tracking crashes, but their low report volume doesn't seem to
warrant
> > significant additional investigation at the moment.
> 
> If we agree the widget should never be NULL, what about adding a
> CHECK(GetWidget()) in ViewHierarchyChanged? Maybe that would tell us how we're
> ending up with a NULL widget. The CHECK may need to be conditional on having
> been added to a widget first.

Done. I added this CHECK and similar for the activation handlers.
(would you rather I simply use GetWidget() in those handlers?)

> Other thought, do we do the right thing if the NativeWidget is deleted out
from
> the under the widget?

Afaict, yes after my recent r269600 and via my new test, which passes even
without the functional changes here.

[sky, 2014-05-13 23:31:00]

LGTM

https://codereview.chromium.org/268143007/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
(right):

https://codereview.chromium.org/268143007/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:24:
#if defined(OS_WIN) && defined(TOOLKIT_VIEWS)
nuke TOOLKIT_VIEWS

https://codereview.chromium.org/268143007/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:288:
#if defined(OS_WIN) && defined(TOOLKIT_VIEWS)
Nuke TOOLKIT_VIEWS.

[msw, 2014-05-13 23:43:27]

Done; landing.

https://codereview.chromium.org/268143007/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
(right):

https://codereview.chromium.org/268143007/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:24:
#if defined(OS_WIN) && defined(TOOLKIT_VIEWS)
On 2014/05/13 23:31:00, sky wrote:
> nuke TOOLKIT_VIEWS

Oh yeah, duh!

https://codereview.chromium.org/268143007/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc:288:
#if defined(OS_WIN) && defined(TOOLKIT_VIEWS)
On 2014/05/13 23:31:00, sky wrote:
> Nuke TOOLKIT_VIEWS.

Done.

[msw, 2014-05-13 23:43:57]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-05-13 23:46:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/268143007/180001

[commit-bot: I haz the power, 2014-05-14 02:59:46]

Change committed as 270310