Fixes ungraceful handling of destroyed GlobalError GlobalErrorBubbleView.

chrome/browser/ui/views/global_error_bubble_view.cc

 // Copyright (c) 2012 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/global_error_bubble_view.h"
 
 #include <stddef.h>
 
 #include <vector>
 
@@ skipping 56 matching lines @@
     : BubbleDialogDelegateView(anchor_view, arrow),
       browser_(browser),
       error_(error) {
   if (!anchor_view)
     SetAnchorRect(gfx::Rect(anchor_point, gfx::Size()));
 }
 
 GlobalErrorBubbleView::~GlobalErrorBubbleView() {}
 
 base::string16 GlobalErrorBubbleView::GetWindowTitle() const {
-  return error_->GetBubbleViewTitle();
+  if (error_) {

[Wez, 2017/01/24 01:45:55]

nit: I'd recommend using an early-exit pattern for these, i.e. if |error_| is
invalid then return a dummy value:

if (error_)
  return base::string16();
return error_->GetBubbleViewTitle();

in this case.

[CJ, 2017/01/25 01:11:21]

Done. you meant !error right?

[Wez, 2017/01/26 18:34:09]

D'oh! Yes :D

+    return error_->GetBubbleViewTitle();
+  }
+  return base::string16();
 }
 
 gfx::ImageSkia GlobalErrorBubbleView::GetWindowIcon() {
-  gfx::Image image = error_->GetBubbleViewIcon();
+  gfx::Image image;
+  if (error_) {
+    image = error_->GetBubbleViewIcon();
+  }
   DCHECK(!image.IsEmpty());

[Wez, 2017/01/24 01:45:55]

This DCHECK will trivially fail if |error_| is null, so suggest moving it inside
the if body.

[CJ, 2017/01/25 01:11:21]

Done.

   return *image.ToImageSkia();
 }
 
 bool GlobalErrorBubbleView::ShouldShowWindowIcon() const {
   return true;
 }
 
 void GlobalErrorBubbleView::WindowClosing() {
   if (error_)
     error_->BubbleViewDidClose(browser_);
 }
 
 void GlobalErrorBubbleView::Init() {
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(
       GetLayoutConstant(LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET), 0));
 
+  DCHECK(error_);

[Wez, 2017/01/24 01:45:55]

Ooooh, this is a fun one :)

It's good to document the expectation that |error_| be non-null here, however
the de-references below will each DCHECK the same thing, for you - so I'd
suggest not bothering with the DCHECKs, and perhaps just add a one-line comment
at the top of Init() saying e.g. "|error_| is assumed to be valid, and stay
valid, at least until Init() returns."

[CJ, 2017/01/25 01:11:20]

Done.

   std::vector<base::string16> message_strings(error_->GetBubbleViewMessages());
   std::vector<views::Label*> message_labels;
   for (size_t i = 0; i < message_strings.size(); ++i) {
     views::Label* message_label = new views::Label(message_strings[i]);
     message_label->SetMultiLine(true);
     message_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     message_labels.push_back(message_label);
   }
 
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
   // First row, message labels.
   views::ColumnSet* cs = layout->AddColumnSet(0);
   cs->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
                 views::GridLayout::FIXED, kMaxBubbleViewWidth, 0);
 
   for (size_t i = 0; i < message_labels.size(); ++i) {
     layout->StartRow(1, 0);
     layout->AddView(message_labels[i]);
     if (i < message_labels.size() - 1)
       layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
   }
 
   // These bubbles show at times where activation is sporadic (like at startup,
   // or a new window opening). Make sure the bubble doesn't disappear before the
   // user sees it, if the bubble needs to be acknowledged.
+  DCHECK(error_);

[Wez, 2017/01/24 01:45:55]

See above :)

[CJ, 2017/01/25 01:11:21]

Done.

   set_close_on_deactivate(error_->ShouldCloseOnDeactivate());
 }
 
 void GlobalErrorBubbleView::UpdateButton(views::LabelButton* button,
                                          ui::DialogButton type) {
-  BubbleDialogDelegateView::UpdateButton(button, type);
-  if (type == ui::DIALOG_BUTTON_OK &&
-      error_->ShouldAddElevationIconToAcceptButton()) {
-    elevation_icon_setter_.reset(new ElevationIconSetter(
-        button, base::Bind(&GlobalErrorBubbleView::SizeToContents,
-                           base::Unretained(this))));
+  if (error_) {
+    BubbleDialogDelegateView::UpdateButton(button, type);

[Wez, 2017/01/24 01:45:55]

nit: Does UpdateButton reference |error_| in some way?

[CJ, 2017/01/25 01:11:21]

Seems to. Found that through my tests as the StrictMock kept failing because of
unexpected calls to |error_| during UpdateButton.
GetBubbleViewAcceptButtonLabel() and GetBubbleViewCancelButtonLabel() get called
sometime in UpdateButton.

[Wez, 2017/01/26 18:34:09]

Ahhh! That makes sense - UpdateButton() is causing the BubbleDialogdelegateView
to need to know what the button text should be, and which button(s) to show.

Suggest adding a brief comment to state that UpdateButton() can result in calls
back in to the GlobalErrorBubbleView.

[CJ, 2017/01/27 00:44:05]

Done.

+    if (type == ui::DIALOG_BUTTON_OK &&
+        error_->ShouldAddElevationIconToAcceptButton()) {
+      elevation_icon_setter_.reset(new ElevationIconSetter(
+          button, base::Bind(&GlobalErrorBubbleView::SizeToContents,
+                             base::Unretained(this))));
+    }
   }
 }
 
 bool GlobalErrorBubbleView::ShouldShowCloseButton() const {
   return error_ && error_->ShouldShowCloseButton();
 }
 
 bool GlobalErrorBubbleView::ShouldDefaultButtonBeBlue() const {
   return true;
 }
 
 base::string16 GlobalErrorBubbleView::GetDialogButtonLabel(
     ui::DialogButton button) const {
+  DCHECK(error_);

[Wez, 2017/01/24 01:45:55]

As noted above, the de-references below will already DCHECK this.

What is it that guarantees that |error_| is valid here, though? Are these
called-back synchronously from within Init, for example?

If there is no such guarantee then I'd suggest having GetDialogButtons() return
ui::DIALOG_BUTTON_NONE and this return an empty string if |error_| is invalid.

[CJ, 2017/01/25 01:11:21]

Done.

   return button == ui::DIALOG_BUTTON_OK
              ? error_->GetBubbleViewAcceptButtonLabel()
              : error_->GetBubbleViewCancelButtonLabel();
 }
 
 int GlobalErrorBubbleView::GetDialogButtons() const {
+  DCHECK(error_);

[Wez, 2017/01/24 01:45:55]

See GetDialogButtonLabel comment above.

[CJ, 2017/01/25 01:11:21]

Done.

   return ui::DIALOG_BUTTON_OK |
          (error_->GetBubbleViewCancelButtonLabel().empty()
               ? 0
               : ui::DIALOG_BUTTON_CANCEL);
 }
 
 bool GlobalErrorBubbleView::Cancel() {
-  error_->BubbleViewCancelButtonPressed(browser_);
+  if (error_)
+    error_->BubbleViewCancelButtonPressed(browser_);
   return true;
 }
 
 bool GlobalErrorBubbleView::Accept() {
-  error_->BubbleViewAcceptButtonPressed(browser_);
+  if (error_)
+    error_->BubbleViewAcceptButtonPressed(browser_);
   return true;
 }
 
 bool GlobalErrorBubbleView::Close() {
   // Don't fall through to either Cancel() or Accept().
   return true;
 }
 
 void GlobalErrorBubbleView::CloseBubbleView() {
   GetWidget()->Close();
 }