[Notifications] Fix crashes.

ui/arc/notification/arc_custom_notification_view.cc

Index: ui/arc/notification/arc_custom_notification_view.cc
diff --git a/ui/arc/notification/arc_custom_notification_view.cc b/ui/arc/notification/arc_custom_notification_view.cc
index a09ecb34df63664e6b1aa24cc81cef90d8396cc7..d79a43e8bf71bbf6414d6c60d3346054dbd5423e 100644
--- a/ui/arc/notification/arc_custom_notification_view.cc
+++ b/ui/arc/notification/arc_custom_notification_view.cc
@@ -294,11 +294,12 @@ void ArcCustomNotificationView::CreateSettingsButton() {
   control_buttons_view_->AddChildView(settings_button_);
 }
 
-void ArcCustomNotificationView::CreateFloatingControlButtons() {
+void ArcCustomNotificationView::MaybeCreateFloatingControlButtons() {
   // Floating close button is a transient child of |surface_| and also part
   // of the hosting widget's focus chain. It could only be created when both
-  // are present.
-  if (!surface_ || !GetWidget())
+  // are present. Further, if we are being destroyed (|item_| is null), don't
+  // create the control buttons.
+  if (!surface_ || !GetWidget() || !item_)
     return;
 
   // Creates the control_buttons_view_, which collects all control buttons into
@@ -307,9 +308,9 @@ void ArcCustomNotificationView::CreateFloatingControlButtons() {
   control_buttons_view_->SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0));
 
-  if (item_ && item_->IsOpeningSettingsSupported())
+  if (item_->IsOpeningSettingsSupported())
     CreateSettingsButton();
-  if (item_ && !item_->pinned())
+  if (!item_->pinned())
     CreateCloseButton();
 
   views::Widget::InitParams params(views::Widget::InitParams::TYPE_CONTROL);
@@ -350,13 +351,17 @@ void ArcCustomNotificationView::SetSurface(exo::NotificationSurface* surface) {
     surface_->window()->AddObserver(this);
     surface_->window()->AddPreTargetHandler(event_forwarder_.get());
 
-    CreateFloatingControlButtons();
+    MaybeCreateFloatingControlButtons();
 
     if (GetWidget())
       AttachSurface();
   }
 }
 
+// N.B. |UpdatePreferredSize| calls SetPreferredSize, which may update the
+// notification. If this is during a clear all operation, we may receive an
+// OnItemDestroying event. That is, calling this method may indirectly cause

[yhanada1, 2017/04/21 08:06:49]

method?

[Eliot Courtney, 2017/04/25 04:09:24]

With crrev.com/2833403002 this comment is no longer accurate, so I removed it.

+// item_ to become null.
 void ArcCustomNotificationView::UpdatePreferredSize() {
   gfx::Size preferred_size =
       surface_ ? surface_->GetSize() : item_ ? item_->snapshot().size()
@@ -378,12 +383,10 @@ void ArcCustomNotificationView::UpdateControlButtonsVisibility() {
   if (!surface_)
     return;
 
-  if (!floating_control_buttons_widget_) {
-    if (GetWidget())
-      CreateFloatingControlButtons();
-    else
-      return;
-  }
+  if (!floating_control_buttons_widget_)
+    MaybeCreateFloatingControlButtons();
+  if (!floating_control_buttons_widget_)

[yoshiki, 2017/04/21 15:21:38]

I think the following structure is better for readability.
if (!floating_control_buttons_widget_) {
  MaybeCreateFloatingControlButtons();
  if (!floating_control_buttons_widget_)
    return;
}

Please mention that floating_control_buttons_widget_ may be updated in
MaybeCreateFloatingControlButtons().

[Eliot Courtney, 2017/04/25 04:09:24]

Done.

+    return;
 
   const bool target_visiblity =
       IsMouseHovered() || (close_button_ && close_button_->HasFocus()) ||
@@ -398,7 +401,8 @@ void ArcCustomNotificationView::UpdateControlButtonsVisibility() {
 }
 
 void ArcCustomNotificationView::UpdatePinnedState() {
-  DCHECK(item_);
+  if (!item_)
+    return;
 
   if (item_->pinned() && close_button_) {
     control_buttons_view_->RemoveChildView(close_button_.get());
@@ -452,7 +456,9 @@ void ArcCustomNotificationView::StartControlButtonsColorAnimation() {
 }
 
 bool ArcCustomNotificationView::ShouldUpdateControlButtonsColor() const {
-  DCHECK(item_);
+  // Don't update the control button color when we are about to be destroyed.
+  if (!item_)
+   return false;

[yoshiki, 2017/04/21 15:21:38]

I think it's better to check item_ in the caller instead of here for
consistency. But it's up to you.

[Eliot Courtney, 2017/04/25 04:09:24]

I thought it would be good to create preconditions at the top of every function.
This function accesses item_ so I figured it'd be better to check it close to
the access.

 
   if (settings_button_ &&
       settings_button_->background()->get_color() !=