Fix a lifetime issue in ToolbarActionsBarBubbleViews

chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc

Index: chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
diff --git a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
index 23396652cda7bb461715cf584df94e793406ab0a..4dc0d68943f09b23a1316ef2830e21b497dd6406 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
@@ -4,7 +4,6 @@
 
 #include "chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h"
 
-#include "chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h"
 #include "chrome/browser/ui/views/harmony/layout_delegate.h"
 #include "chrome/grit/locale_settings.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -28,6 +27,8 @@ ToolbarActionsBarBubbleViews::ToolbarActionsBarBubbleViews(
     : views::BubbleDialogDelegateView(anchor_view,
                                       views::BubbleBorder::TOP_RIGHT),
       delegate_(std::move(delegate)),
+      close_reason_(
+          ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION),
       item_list_(nullptr),
       link_(nullptr),
       anchored_to_action_(anchored_to_action) {
@@ -100,10 +101,7 @@ bool ToolbarActionsBarBubbleViews::Accept() {
 }
 
 bool ToolbarActionsBarBubbleViews::Close() {
-  if (delegate_) {
-    delegate_->OnBubbleClosed(
-        ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION);
-  }
+  delegate_->OnBubbleClosed(close_reason_);
   return true;
 }
 
@@ -163,8 +161,6 @@ base::string16 ToolbarActionsBarBubbleViews::GetDialogButtonLabel(
 
 void ToolbarActionsBarBubbleViews::LinkClicked(views::Link* link,
                                                int event_flags) {
-  delegate_->OnBubbleClosed(ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE);
-  // Reset delegate so we don't send extra OnBubbleClosed()s.
-  delegate_.reset();
+  close_reason_ = ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE;

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

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]

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 ).

   GetWidget()->Close();
 }