Fix crash under ExclusiveAccessBubbleViews::AnimationProgressed().

Fix crash under ExclusiveAccessBubbleViews::AnimationProgressed().

The crash stacks suggest that |Widget::widget_delegate_| has been set to
null. That only happens in Widget::OnNativeWidgetDestroyed(), but that
should only be triggered by a `delete popup_;` fired asynchronously from
the ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

What appears to be happening is that the bubble is being closed before
the ExclusiveAccessBubbleViews destructor is invoked. Observing
OnWidgetDestroyed() revealed some shutdown codepaths where this is
possible in existing tests.

Likely these crashes happen in the wild because of a system logoff while
the bubble is showing that causes some uncommon shutdown codepaths, or
window close events directly from the OS, which can't be ignored.

To fix, observe OnWidgetDestroyed() and ask the owner of
ExclusiveAccessBubbleViews to delete it. This will cancel any animation
timers and ensure nothing references the null widget_delegate_.

BUG=650882
Review-Url: https://codereview.chromium.org/2850403002
Cr-Commit-Position: refs/heads/master@{#469922}
Committed: https://chromium.googlesource.com/chromium/src/+/d62f8027170674b8d057dce514673fe23b5d2a29

[tapted, 2017-05-02 07:16:55]

Description was changed from

==========
Add tracing to diagnose crashes in
ExclusiveAccessBubbleViews::AnimationProgressed()

The stacks suggest that |Widget::widget_delegate_| has been set to null.
That only happens in Widget::OnNativeWidgetDestroyed(), but that should
only be triggered by a `delete popup_;` fired from the
ExclusiveAccessBubbleViews destructor. And when that same destructor completes,
it should also be resetting the |animation_| member, which stops and cancels any
animation timers.

First, try to see whether a call to Widget::OnNativeWidgetDestroyed() is
somehow getting through before the ~ExclusiveAccessBubbleViews destructor
by observing it.

BUG=650882
==========

to

==========
Add tracing to diagnose crashes in
ExclusiveAccessBubbleViews::AnimationProgressed()

The stacks suggest that |Widget::widget_delegate_| has been set to null.
That only happens in Widget::OnNativeWidgetDestroyed(), but that should
only be triggered by a `delete popup_;` fired from the
ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

First, try to see whether a call to Widget::OnNativeWidgetDestroyed() is
somehow getting through before the ~ExclusiveAccessBubbleViews
destructor by observing it.

BUG=650882
==========

[tapted, 2017-05-02 07:16:56]

tapted@chromium.org changed reviewers:
+ msw@chromium.org

[tapted, 2017-05-02 07:17:11]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 07:17:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-02 07:18:04]

Description was changed from

==========
Add tracing to diagnose crashes in
ExclusiveAccessBubbleViews::AnimationProgressed()

The stacks suggest that |Widget::widget_delegate_| has been set to null.
That only happens in Widget::OnNativeWidgetDestroyed(), but that should
only be triggered by a `delete popup_;` fired from the
ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

First, try to see whether a call to Widget::OnNativeWidgetDestroyed() is
somehow getting through before the ~ExclusiveAccessBubbleViews
destructor by observing it.

BUG=650882
==========

to

==========
Add tracing to diagnose crashes in
ExclusiveAccessBubbleViews::AnimationProgressed()

The stacks suggest that |Widget::widget_delegate_| has been set to null.
That only happens in Widget::OnNativeWidgetDestroyed(), but that should
only be triggered by a `delete popup_;` fired from the
ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

First, try to see whether a call to Widget::OnNativeWidgetDestroyed() is
somehow getting through before the ~ExclusiveAccessBubbleViews
destructor by observing OnWidgetDestroyed().

BUG=650882
==========

[tapted, 2017-05-02 07:18:34]

Description was changed from

==========
Add tracing to diagnose crashes in
ExclusiveAccessBubbleViews::AnimationProgressed()

The stacks suggest that |Widget::widget_delegate_| has been set to null.
That only happens in Widget::OnNativeWidgetDestroyed(), but that should
only be triggered by a `delete popup_;` fired from the
ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

First, try to see whether a call to Widget::OnNativeWidgetDestroyed() is
somehow getting through before the ~ExclusiveAccessBubbleViews
destructor by observing OnWidgetDestroyed().

BUG=650882
==========

to

==========
Add tracing to diagnose crashes in
ExclusiveAccessBubbleViews::AnimationProgressed()

The stacks suggest that |Widget::widget_delegate_| has been set to null.
That only happens in Widget::OnNativeWidgetDestroyed(), but that should
only be triggered by a `delete popup_;` fired asynchronously from the
ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

First, try to see whether a call to Widget::OnNativeWidgetDestroyed() is
somehow getting through before the ~ExclusiveAccessBubbleViews
destructor by observing OnWidgetDestroyed().

BUG=650882
==========

[commit-bot: I haz the power, 2017-05-02 08:03:16]

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

[commit-bot: I haz the power, 2017-05-02 08:03:17]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tapted, 2017-05-03 07:21:42]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 07:21:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-03 07:27:16]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 07:27:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 07:44:53]

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

[commit-bot: I haz the power, 2017-05-03 07:44:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-05-03 09:12:39]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 09:12:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 10:25:25]

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

[commit-bot: I haz the power, 2017-05-03 10:25:26]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-03 11:39:13]

Description was changed from

==========
Add tracing to diagnose crashes in
ExclusiveAccessBubbleViews::AnimationProgressed()

The stacks suggest that |Widget::widget_delegate_| has been set to null.
That only happens in Widget::OnNativeWidgetDestroyed(), but that should
only be triggered by a `delete popup_;` fired asynchronously from the
ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

First, try to see whether a call to Widget::OnNativeWidgetDestroyed() is
somehow getting through before the ~ExclusiveAccessBubbleViews
destructor by observing OnWidgetDestroyed().

BUG=650882
==========

to

==========
Fix crash under ExclusiveAccessBubbleViews::AnimationProgressed.

The crash stacks suggest that |Widget::widget_delegate_| has been set to
null. That only happens in Widget::OnNativeWidgetDestroyed(), but that
should only be triggered by a `delete popup_;` fired asynchronously from
the ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

What appears to be happening is that the bubble is being closed before
the ExclusiveAccessBubbleViews destructor is invoked. Observing
OnWidgetDestroyed() revealed some shutdown codepaths where this is
possible in existing tests.

Likely these crashes happen in the wild because of a system logoff while
the bubble is showing that causes some uncommon shutdown codepaths, or
window close events directly from the OS, which can't be ignored.

To fix, observe OnWidgetDestroyed() and ask the owner of
ExclusiveAccessBubbleViews to delete it. This will cancel any animation
timers and ensure nothing references the null widget_delegate_.

BUG=650882
==========

[tapted, 2017-05-03 11:39:27]

Description was changed from

==========
Fix crash under ExclusiveAccessBubbleViews::AnimationProgressed.

The crash stacks suggest that |Widget::widget_delegate_| has been set to
null. That only happens in Widget::OnNativeWidgetDestroyed(), but that
should only be triggered by a `delete popup_;` fired asynchronously from
the ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

What appears to be happening is that the bubble is being closed before
the ExclusiveAccessBubbleViews destructor is invoked. Observing
OnWidgetDestroyed() revealed some shutdown codepaths where this is
possible in existing tests.

Likely these crashes happen in the wild because of a system logoff while
the bubble is showing that causes some uncommon shutdown codepaths, or
window close events directly from the OS, which can't be ignored.

To fix, observe OnWidgetDestroyed() and ask the owner of
ExclusiveAccessBubbleViews to delete it. This will cancel any animation
timers and ensure nothing references the null widget_delegate_.

BUG=650882
==========

to

==========
Fix crash under ExclusiveAccessBubbleViews::AnimationProgressed().

The crash stacks suggest that |Widget::widget_delegate_| has been set to
null. That only happens in Widget::OnNativeWidgetDestroyed(), but that
should only be triggered by a `delete popup_;` fired asynchronously from
the ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

What appears to be happening is that the bubble is being closed before
the ExclusiveAccessBubbleViews destructor is invoked. Observing
OnWidgetDestroyed() revealed some shutdown codepaths where this is
possible in existing tests.

Likely these crashes happen in the wild because of a system logoff while
the bubble is showing that causes some uncommon shutdown codepaths, or
window close events directly from the OS, which can't be ignored.

To fix, observe OnWidgetDestroyed() and ask the owner of
ExclusiveAccessBubbleViews to delete it. This will cancel any animation
timers and ensure nothing references the null widget_delegate_.

BUG=650882
==========

[tapted, 2017-05-03 11:47:28]

tapted@chromium.org changed reviewers:
+ mgiuca@chromium.org
- msw@chromium.org

[tapted, 2017-05-03 11:48:06]

tapted@chromium.org changed reviewers:
+ msw@chromium.org
- mgiuca@chromium.org

[tapted, 2017-05-03 11:58:11]

Hi Mike, please take a look. These crashes have been around for a while on Mac
and Windows (I noticed them on Mac because I was scrutinising things with
ui/views on the crash server for Mac). It was a tricky one, but I think I
figured it out. Thanks!

(cc matt/scheib FYI)

[msw, 2017-05-03 18:48:34]

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right):

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:286: // Although
SubtleNotificationView uses WIDGET_OWNS_NATIVE_WIDGET, a close can
Would using the default NATIVE_WIDGET_OWNS_WIDGET codepath simplify things at
this point? (I'm not sure, just asking)

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:298: // invoked. Note
this is safe to do since |popup_| is deleted via a posted
optional nit: remove "Note" to avoid an extra line wrap :)

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views_context.h (right):

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_context.h:51: // Destroy
any exclusive active bubble.
nit: "access", maybe describe why this is needed akin to your comment in
exclusive_access_bubble_views.cc?

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc
(right):

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:28:
widget->RemoveObserver(this);
optional nit: move this into OnWidgetDestroying?

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:39:
// Simulate obscure codepaths resulting in the bubble Widget being Closed before
nit: "closed"?

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:40:
// the ExclusiveAccessBubbleViews destructor will tear down the bubble and any
nit: this sentence is a bit run-on and could be clearer

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:42:
IN_PROC_BROWSER_TEST_F(ExclusiveAccessBubbleViewsTest, NativeClose) {
nit: maybe make a test for the normal close codepath? (perhaps that exists)

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:49:
// It's hard to nail down a codepath that results in the bubble Widget being
Maybe this comment should just say "Simulate the bubble being closed out from
under its controller, which seems to happen in some odd corner cases, like
system log-off while the bubble is showing."

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:50:
// destroyed before the call to the ExclusiveAccessBubbleViewsTest destructor.
q: Did you mean ExclusiveAccessBubbleViewsTest? (I'm not sure why the test dtor
is relevant here, it seems like you mean some other bubble/controller component)

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:68:
bubble()->GetView()->GetWidget()->CloseNow();
aside: you could also potentially delete the bubble's NativeWidget?

[tapted, 2017-05-04 05:16:43]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 05:16:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-05-04 06:42:08]

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right):

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:286: // Although
SubtleNotificationView uses WIDGET_OWNS_NATIVE_WIDGET, a close can
On 2017/05/03 18:48:33, msw wrote:
> Would using the default NATIVE_WIDGET_OWNS_WIDGET codepath simplify things at
> this point? (I'm not sure, just asking)

I think this added complexity would remain. That is, we'd still need both
codepaths. The difference would be that the destructor calls popup_->Close()
rather than `delete popup_;` (and.. I think that's all). But there's also a
comment in ~ExclusiveAccessBubbleViews(): "We also can't set the popup's
ownership model to NATIVE_WIDGET_OWNS_WIDGET because if the user closed the last
tab while in fullscreen mode, Windows has already destroyed the popup HWND"

Although I suspect now that by watching OnWidgetDestroyed() we cater for that
"gotcha" where the popup HWND has already been closed as well. That is, the
workaround to avoid NATIVE_WIDGET_OWNS_WIDGET works but only so long as an
animation timer didn't get into the run queue before |this| got destroyed.

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:298: // invoked. Note
this is safe to do since |popup_| is deleted via a posted
On 2017/05/03 18:48:33, msw wrote:
> optional nit: remove "Note" to avoid an extra line wrap :)

Done.

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views_context.h (right):

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_context.h:51: // Destroy
any exclusive active bubble.
On 2017/05/03 18:48:33, msw wrote:
> nit: "access", maybe describe why this is needed akin to your comment in
> exclusive_access_bubble_views.cc?

Done.

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc
(right):

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:28:
widget->RemoveObserver(this);
On 2017/05/03 18:48:33, msw wrote:
> optional nit: move this into OnWidgetDestroying?

Done.

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:39:
// Simulate obscure codepaths resulting in the bubble Widget being Closed before
On 2017/05/03 18:48:34, msw wrote:
> nit: "closed"?

Done.

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:40:
// the ExclusiveAccessBubbleViews destructor will tear down the bubble and any
On 2017/05/03 18:48:34, msw wrote:
> nit: this sentence is a bit run-on and could be clearer

Done.

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:42:
IN_PROC_BROWSER_TEST_F(ExclusiveAccessBubbleViewsTest, NativeClose) {
On 2017/05/03 18:48:34, msw wrote:
> nit: maybe make a test for the normal close codepath? (perhaps that exists)

Yup - most of the tests in FullscreenControllerInteractiveTest exercise the
regular codepaths. E.g. those doing
ASSERT_{TRUE,FALSE}(IsFullscreenBubbleDisplayed());
 
(and.. at least 4 of those tests are not disabled for being flaky... perhaps
this bug I'm fixing is even the cause of that flakiness...)

I've been checking up on them occasionally to ensure the disabled tests still
pass. E.g. http://crbug.com/651272 . (and I just ran them again and all 19 are
passing.. at least on Mac).

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:49:
// It's hard to nail down a codepath that results in the bubble Widget being
On 2017/05/03 18:48:34, msw wrote:
> Maybe this comment should just say "Simulate the bubble being closed out from
> under its controller, which seems to happen in some odd corner cases, like
> system log-off while the bubble is showing."

Done. Yeah is probably too much info.

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:50:
// destroyed before the call to the ExclusiveAccessBubbleViewsTest destructor.
On 2017/05/03 18:48:34, msw wrote:
> q: Did you mean ExclusiveAccessBubbleViewsTest? (I'm not sure why the test
dtor
> is relevant here, it seems like you mean some other bubble/controller
component)

Ahh yep - I meant the ExclusiveAccessBubbleViews destructor.

https://codereview.chromium.org/2850403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc:68:
bubble()->GetView()->GetWidget()->CloseNow();
On 2017/05/03 18:48:34, msw wrote:
> aside: you could also potentially delete the bubble's NativeWidget?

That works for NativeWidgetAura, and
views::PlatformTestHelper::SimulateNativeDestroy() does that for
NativeWidgetAura and does an equivalent thing for Mac. However,
DesktopNativeWidgetAura (i.e. DesktopWindowTreeHostWin) is a bit different. But
the way that CloseNow() is implemented turns out to do a reasonable thing for
all platforms.

[commit-bot: I haz the power, 2017-05-04 06:47:32]

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

[commit-bot: I haz the power, 2017-05-04 06:47:33]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-05 19:27:09]

lgtm

[tapted, 2017-05-07 12:31:50]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-05-07 12:32:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-07 13:47:44]

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

[commit-bot: I haz the power, 2017-05-07 13:47:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-05-07 23:46:36]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-05-07 23:46:51]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 01:03:15]

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

[commit-bot: I haz the power, 2017-05-08 01:03:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-05-08 06:50:25]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-05-08 06:51:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 08:33:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494226225872160,
"parent_rev": "ec747d5400f2930853e1f90c8365916393803606", "commit_rev":
"d62f8027170674b8d057dce514673fe23b5d2a29"}

[commit-bot: I haz the power, 2017-05-08 08:33:58]

Description was changed from

==========
Fix crash under ExclusiveAccessBubbleViews::AnimationProgressed().

The crash stacks suggest that |Widget::widget_delegate_| has been set to
null. That only happens in Widget::OnNativeWidgetDestroyed(), but that
should only be triggered by a `delete popup_;` fired asynchronously from
the ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

What appears to be happening is that the bubble is being closed before
the ExclusiveAccessBubbleViews destructor is invoked. Observing
OnWidgetDestroyed() revealed some shutdown codepaths where this is
possible in existing tests.

Likely these crashes happen in the wild because of a system logoff while
the bubble is showing that causes some uncommon shutdown codepaths, or
window close events directly from the OS, which can't be ignored.

To fix, observe OnWidgetDestroyed() and ask the owner of
ExclusiveAccessBubbleViews to delete it. This will cancel any animation
timers and ensure nothing references the null widget_delegate_.

BUG=650882
==========

to

==========
Fix crash under ExclusiveAccessBubbleViews::AnimationProgressed().

The crash stacks suggest that |Widget::widget_delegate_| has been set to
null. That only happens in Widget::OnNativeWidgetDestroyed(), but that
should only be triggered by a `delete popup_;` fired asynchronously from
the ExclusiveAccessBubbleViews destructor. And when that same destructor
completes, it should also be resetting the |animation_| member, which
stops and cancels any animation timers.

What appears to be happening is that the bubble is being closed before
the ExclusiveAccessBubbleViews destructor is invoked. Observing
OnWidgetDestroyed() revealed some shutdown codepaths where this is
possible in existing tests.

Likely these crashes happen in the wild because of a system logoff while
the bubble is showing that causes some uncommon shutdown codepaths, or
window close events directly from the OS, which can't be ignored.

To fix, observe OnWidgetDestroyed() and ask the owner of
ExclusiveAccessBubbleViews to delete it. This will cancel any animation
timers and ensure nothing references the null widget_delegate_.

BUG=650882

Review-Url: https://codereview.chromium.org/2850403002
Cr-Commit-Position: refs/heads/master@{#469922}
Committed:
https://chromium.googlesource.com/chromium/src/+/d62f8027170674b8d057dce51467...
==========

[commit-bot: I haz the power, 2017-05-08 08:33:58]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d62f8027170674b8d057dce51467...