Fix a lifetime issue in ToolbarActionsBarBubbleViews

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/layout/box_layout.h"
 
 namespace {
 const int kIconSize = 16;
 }
 
 ToolbarActionsBarBubbleViews::ToolbarActionsBarBubbleViews(
     views::View* anchor_view,
     const gfx::Point& anchor_point,
     bool anchored_to_action,
     std::unique_ptr<ToolbarActionsBarBubbleDelegate> delegate)
     : 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) {
   set_close_on_deactivate(delegate_->ShouldCloseOnDeactivate());
   if (!anchor_view)
     SetAnchorRect(gfx::Rect(anchor_point, gfx::Size()));
 }
 
 ToolbarActionsBarBubbleViews::~ToolbarActionsBarBubbleViews() {}
 
@@ skipping 52 matching lines @@
       ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_USER_ACTION);
   return true;
 }
 
 bool ToolbarActionsBarBubbleViews::Accept() {
   delegate_->OnBubbleClosed(ToolbarActionsBarBubbleDelegate::CLOSE_EXECUTE);
   return true;
 }
 
 bool ToolbarActionsBarBubbleViews::Close() {
-  if (delegate_) {
-    delegate_->OnBubbleClosed(
-        ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION);
-  }
+  delegate_->OnBubbleClosed(close_reason_);
   return true;
 }
 
 void ToolbarActionsBarBubbleViews::Init() {
   LayoutDelegate* delegate = LayoutDelegate::Get();
   SetLayoutManager(new views::BoxLayout(
       views::BoxLayout::kVertical, 0, 0,
       delegate->GetMetric(
           LayoutDelegate::Metric::RELATED_CONTROL_VERTICAL_SPACING)));
 
@@ skipping 39 matching lines @@
 }
 
 base::string16 ToolbarActionsBarBubbleViews::GetDialogButtonLabel(
     ui::DialogButton button) const {
   return button == ui::DIALOG_BUTTON_OK ? delegate_->GetActionButtonText()
                                         : delegate_->GetDismissButtonText();
 }
 
 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();
 }