Fix a lifetime issue in ToolbarActionsBarBubbleViews

Fix a lifetime issue in ToolbarActionsBarBubbleViews.

Also re-enable
ExtensionMessageBubbleViewBrowserTest.TestClickingLearnMoreButton on
Mac, which tickled it, and add more coverage.

The test was disabled on Mac in r457730, but the crash reporter has
matching stacks recorded on Windows - see http://crbug.com/702554#c6.

The problem is that clicking a button would trigger an asynchronous
Widget::Close() after setting the view's delegate to null. But OS events
can still arrive while waiting for the asynchronous Close() to complete,
and cause a segfault.

To fix, don't clear the view's delegate: just set state to ensure the
correct close reason is forwarded to it when the dialog is closed via
the usual codepaths.

BUG=702554
Review-Url: https://codereview.chromium.org/2762893002
Cr-Commit-Position: refs/heads/master@{#459382}
Committed: https://chromium.googlesource.com/chromium/src/+/b9fc45d0bb0654521d098aae19e73925e79ac270

[tapted, 2017-03-21 03:08:58]

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

[commit-bot: I haz the power, 2017-03-21 03:09:31]

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-03-21 03:51:59]

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

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

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-21 04:02:44]

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

[commit-bot: I haz the power, 2017-03-21 04:03:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-21 04:07:49]

Description was changed from

==========
cl format


Option 1

BUG=702554
==========

to

==========
Fix a lifetime issue in ToolbarActionsBarBubbleViews.

Also re-enable
ExtensionMessageBubbleViewBrowserTest.TestClickingLearnMoreButton on
Mac, which tickled it, and add more coverage.

The test was disabled on Mac in r457730, but the crash reporter has
matching stacks recorded on Windows - see http://crbug.com/702554#c6.

The problem is that clicking a button would trigger an asynchronous
Widget::Close() after setting the view's delegate to null. But OS events
can still arrive while waiting for the asynchronous Close() to complete,
and cause a segfault.

To fix, don't clear the view's delegate: just set state to ensure the
correct close reason is forwarded to it when the dialog is closed via
the usual codepaths.

BUG=702554
==========

[tapted, 2017-03-21 04:07:49]

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

[tapted, 2017-03-21 04:10:44]

Hi Finnur, please take a look

[commit-bot: I haz the power, 2017-03-21 04:52:32]

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

[commit-bot: I haz the power, 2017-03-21 04:52:32]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-22 10:07:10]

finnur: ping?

[Finnur, 2017-03-22 10:56:04]

Sorry, I overlooked this yesterday. Thanks for fixing. One question:

https://codereview.chromium.org/2762893002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
(right):

https://codereview.chromium.org/2762893002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:164:
close_reason_ = ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE;
Have you verified none of the different ways of closing the bubble result in
multiple OnBubbleClosed messages being sent?

[tapted, 2017-03-23 00:07:06]

https://codereview.chromium.org/2762893002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
(right):

https://codereview.chromium.org/2762893002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:164:
close_reason_ = ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE;
On 2017/03/22 10:56:04, Finnur wrote:
> Have you verified none of the different ways of closing the bubble result in
> multiple OnBubbleClosed messages being sent?

The test coverage for this is pretty good. Eg.
TestToolbarActionsBarBubbleDelegate has a CHECK() -
https://cs.chromium.org/chromium/src/chrome/browser/ui/toolbar/test_toolbar_a...

ExtensionMessageBubbleBrowserTest has tests for the different ways a bubble can
be dismissed.

ExtensionMessageBubbleController has DCHECKs to ensure the user action is a
`BOUNDARY` / sentinel value that isn't set yet -
https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_mess...
-- but note there is a comment there in ExtensionMessageBubbleController that it
will *intentionally* handle multiple OnBubbleDismiss() gracefully.00

But also note this was the only place that would set |delegate_| to null in the
past, so the only way the protection in ::Close() would have been depended upon
would be *after* a call to LinkClicked. That is, there's no way for a new
codepath to OnBubbleClosed() to have opened up due to the change in ::Close()
that isn't also going through LinkClicked()  (and *not* going through
Cancel/Accept since they never checked |delegate_|).

(basically LinkClicked(), Close(), Close() doesn't happen

DialogClientView::CanClose() is the only thing that calls
DialogDelegate::Close() and it can only call it once since
ToolbarActionsBarBubbleViews::Close() return true

and even if it did, ExtensionMessageBubbleController::OnBubbleDismiss() would
DCHECK ).

[Finnur, 2017-03-24 09:54:15]

Thanks. LGTM.

And since you are OOO for the time being, I'll check the CQ for you. Hope that
doesn't mess things up for you. :)

[Finnur, 2017-03-24 09:54:26]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2017-03-24 09:54:41]

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-03-24 10:39:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490349266388020,
"parent_rev": "ee21033fb48f99bb38854310b2705a9040c4116c", "commit_rev":
"b9fc45d0bb0654521d098aae19e73925e79ac270"}

[commit-bot: I haz the power, 2017-03-24 10:40:59]

Description was changed from

==========
Fix a lifetime issue in ToolbarActionsBarBubbleViews.

Also re-enable
ExtensionMessageBubbleViewBrowserTest.TestClickingLearnMoreButton on
Mac, which tickled it, and add more coverage.

The test was disabled on Mac in r457730, but the crash reporter has
matching stacks recorded on Windows - see http://crbug.com/702554#c6.

The problem is that clicking a button would trigger an asynchronous
Widget::Close() after setting the view's delegate to null. But OS events
can still arrive while waiting for the asynchronous Close() to complete,
and cause a segfault.

To fix, don't clear the view's delegate: just set state to ensure the
correct close reason is forwarded to it when the dialog is closed via
the usual codepaths.

BUG=702554
==========

to

==========
Fix a lifetime issue in ToolbarActionsBarBubbleViews.

Also re-enable
ExtensionMessageBubbleViewBrowserTest.TestClickingLearnMoreButton on
Mac, which tickled it, and add more coverage.

The test was disabled on Mac in r457730, but the crash reporter has
matching stacks recorded on Windows - see http://crbug.com/702554#c6.

The problem is that clicking a button would trigger an asynchronous
Widget::Close() after setting the view's delegate to null. But OS events
can still arrive while waiting for the asynchronous Close() to complete,
and cause a segfault.

To fix, don't clear the view's delegate: just set state to ensure the
correct close reason is forwarded to it when the dialog is closed via
the usual codepaths.

BUG=702554

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

[commit-bot: I haz the power, 2017-03-24 10:41:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b9fc45d0bb0654521d098aae19e7...