Fixes ungraceful handling of destroyed GlobalError GlobalErrorBubbleView.

chrome/browser/ui/views/global_error_bubble_view.cc

Index: chrome/browser/ui/views/global_error_bubble_view.cc
diff --git a/chrome/browser/ui/views/global_error_bubble_view.cc b/chrome/browser/ui/views/global_error_bubble_view.cc
index 62789d901a1a452384093144da1d94ce2f126c3a..740a5b4d4c15017858c5eb0f9922a6af840b71f5 100644
--- a/chrome/browser/ui/views/global_error_bubble_view.cc
+++ b/chrome/browser/ui/views/global_error_bubble_view.cc
@@ -74,11 +74,17 @@ GlobalErrorBubbleView::GlobalErrorBubbleView(
 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();
 }
@@ -97,6 +103,7 @@ void GlobalErrorBubbleView::Init() {
   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) {
@@ -124,17 +131,20 @@ void GlobalErrorBubbleView::Init() {
   // 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))));
+    }
   }
 }
 
@@ -148,12 +158,14 @@ bool GlobalErrorBubbleView::ShouldDefaultButtonBeBlue() const {
 
 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
@@ -161,12 +173,14 @@ int GlobalErrorBubbleView::GetDialogButtons() const {
 }
 
 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;
 }