views: Make sure the bubble-delegate removes itself from the anchor-widget's observer list.

views: Make sure the bubble-delegate removes itself from the anchor-widget's observer list.

This fixes a crash that shows up when terminating from the system tray popup.

BUG=105151, 124310
TEST=WidgetObserverTest.DestroyBubble

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

[msw, 2012-04-24 14:49:06]

Is the repro as simple as that? can I see more info (bug# or just a callstack?)

http://codereview.chromium.org/10204014/diff/1/ui/views/bubble/bubble_delegat...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10204014/diff/1/ui/views/bubble/bubble_delegat...
ui/views/bubble/bubble_delegate.cc:143: anchor_widget_->RemoveObserver(this);
Try OnWidgetClosing, but even then, I find this is extremely hard to get right.

[sadrul, 2012-04-24 15:15:53]

The repro is indeed as simple as that. The test case without the change in the
dtor:

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WidgetObserverTest
[ RUN      ] WidgetObserverTest.DestroyBubble
[10693:10693:0424/111135:6132288825790:ERROR:process_util_posix.cc(143)]
Received signal 11
        base::debug::StackTrace::StackTrace() [0x7fe6cce2bc82]
        base::(anonymous namespace)::StackDumpSignalHandler() [0x7fe6cce8b7a5]
        0x7fe6c3578af0
        views::Widget::OnNativeWidgetVisibilityChanged() [0x7fe6c65478c3]
        views::NativeWidgetAura::OnWindowVisibilityChanged() [0x7fe6c6541bd0]
        aura::Window::SetVisible() [0x7fe6cc0a9d96]
        aura::Window::Hide() [0x7fe6cc0a7d31]
        views::NativeWidgetAura::Hide() [0x7fe6c65406dc]
        views::Widget::Hide() [0x7fe6c6546469]
        views::(anonymous
namespace)::WidgetObserverTest_DestroyBubble_Test::TestBody() [0x54937b]
        testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x562929]
        testing::internal::HandleExceptionsInMethodIfSupported<>() [0x55f488]
        testing::Test::Run() [0x553920]
        testing::TestInfo::Run() [0x554144]
        testing::TestCase::Run() [0x55483a]
        testing::internal::UnitTestImpl::RunAllTests() [0x5597c5]
        testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x563b4d]
        testing::internal::HandleExceptionsInMethodIfSupported<>() [0x55feca]
        testing::UnitTest::Run() [0x5582ae]
        base::TestSuite::Run() [0x59c3af]
        main [0x54bcad]
        0x7fe6c3563c4d
        0x447c69

Steps to reproduce the crash: click on the system tray, click on shutdown

http://codereview.chromium.org/10204014/diff/1/ui/views/bubble/bubble_delegat...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10204014/diff/1/ui/views/bubble/bubble_delegat...
ui/views/bubble/bubble_delegate.cc:143: anchor_widget_->RemoveObserver(this);
On 2012/04/24 14:49:06, msw wrote:
> Try OnWidgetClosing, but even then, I find this is extremely hard to get
right.

I am not sure I understand what you mean.

[msw, 2012-04-24 17:01:43]

Sorry about the crash; I thought I specifically tested that.

http://codereview.chromium.org/10204014/diff/1/ui/views/bubble/bubble_delegat...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10204014/diff/1/ui/views/bubble/bubble_delegat...
ui/views/bubble/bubble_delegate.cc:143: anchor_widget_->RemoveObserver(this);
On 2012/04/24 15:15:53, sadrul wrote:
> On 2012/04/24 14:49:06, msw wrote:
> > Try OnWidgetClosing, but even then, I find this is extremely hard to get
> right.
> 
> I am not sure I understand what you mean.

Sorry, I meant to say try putting this in a WidgetDelegate::WindowClosing
override; I've had problems doing this type of work in the dtor before.

That said, if you don't see issues, this may be okay.

[sadrul, 2012-04-24 18:00:48]

http://codereview.chromium.org/10204014/diff/1/ui/views/bubble/bubble_delegat...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10204014/diff/1/ui/views/bubble/bubble_delegat...
ui/views/bubble/bubble_delegate.cc:143: anchor_widget_->RemoveObserver(this);
On 2012/04/24 17:01:43, msw wrote:
> On 2012/04/24 15:15:53, sadrul wrote:
> > On 2012/04/24 14:49:06, msw wrote:
> > > Try OnWidgetClosing, but even then, I find this is extremely hard to get
> > right.
> > 
> > I am not sure I understand what you mean.
> 
> Sorry, I meant to say try putting this in a WidgetDelegate::WindowClosing
> override; I've had problems doing this type of work in the dtor before.
> 
> That said, if you don't see issues, this may be okay.

Done.

[msw, 2012-04-24 18:10:41]

Ah, this crash makes sense, CloseNow would crash because it doesn't hide the
bubble first (which would invoke anchor_widget()->RemoveObserver(this) in
BubbleDelegateView::OnWidgetVisibilityChanged). I'm really sorry that you needed
to make this fix for me :(

http://codereview.chromium.org/10204014/diff/9001/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10204014/diff/9001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.cc:167: if (anchor_widget_) {
nit: use anchor_widget() here and @line168 for r-value access.

http://codereview.chromium.org/10204014/diff/9001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.cc:169: anchor_widget_ = NULL;
Sorry, would you mind also setting anchor_view_ to null here?

[sadrul, 2012-04-24 18:15:42]

http://codereview.chromium.org/10204014/diff/9001/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10204014/diff/9001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.cc:167: if (anchor_widget_) {
On 2012/04/24 18:10:41, msw wrote:
> nit: use anchor_widget() here and @line168 for r-value access.

I think we actually prefer directly using the variable (at least in the rest of
the views code). (see all the uses of parent_ in view.cc for example)

http://codereview.chromium.org/10204014/diff/9001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.cc:169: anchor_widget_ = NULL;
On 2012/04/24 18:10:41, msw wrote:
> Sorry, would you mind also setting anchor_view_ to null here?

Done.

[msw, 2012-04-24 18:24:58]

LGTM; thanks for fixing this!

http://codereview.chromium.org/10204014/diff/9001/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10204014/diff/9001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.cc:167: if (anchor_widget_) {
On 2012/04/24 18:15:42, sadrul wrote:
> On 2012/04/24 18:10:41, msw wrote:
> > nit: use anchor_widget() here and @line168 for r-value access.
> 
> I think we actually prefer directly using the variable (at least in the rest
of
> the views code). (see all the uses of parent_ in view.cc for example)

Fair enough.

[msw, 2012-04-24 18:26:36]

Oh, please set BUG=105151,124310

[sky, 2012-04-24 19:41:44]

LGTM

http://codereview.chromium.org/10204014/diff/12002/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10204014/diff/12002/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.cc:166: void BubbleDelegateView::WindowClosing()
{
Make position match header.

[sadrul, 2012-04-24 19:47:54]

http://codereview.chromium.org/10204014/diff/12002/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10204014/diff/12002/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.cc:166: void BubbleDelegateView::WindowClosing()
{
On 2012/04/24 19:41:44, sky wrote:
> Make position match header.

Done.