Fix crash on updating notification before attaching

ui/arc/notification/arc_notification_content_view.cc

Index: ui/arc/notification/arc_notification_content_view.cc
diff --git a/ui/arc/notification/arc_notification_content_view.cc b/ui/arc/notification/arc_notification_content_view.cc
index 312dcb97f903e63d66ce622d45743429f8169721..c387c386470056e654dd59250bfbb90aca4f2d59 100644
--- a/ui/arc/notification/arc_notification_content_view.cc
+++ b/ui/arc/notification/arc_notification_content_view.cc
@@ -360,8 +360,6 @@ void ArcNotificationContentView::SetSurface(exo::NotificationSurface* surface) {
     surface_->window()->AddObserver(this);
     surface_->window()->AddPreTargetHandler(event_forwarder_.get());
 
-    MaybeCreateFloatingControlButtons();
-
     if (GetWidget())
       AttachSurface();
   }
@@ -388,22 +386,9 @@ void ArcNotificationContentView::UpdatePreferredSize() {
 }
 
 void ArcNotificationContentView::UpdateControlButtonsVisibility() {
-  if (!surface_)
+  if (!floating_control_buttons_widget_)
     return;
 
-  // TODO(edcourtney, yhanada): Creating the floating control widget here is not
-  // correct. This function may be called during the destruction of
-  // |floating_control_buttons_widget_|. This can lead to memory corruption.
-  // Rather than creating it here, we should fix the behaviour of OnMouseExited
-  // and OnMouseEntered for ARC notifications in MessageCenterView. See
-  // crbug.com/714587 and crbug.com/709862.
-  if (!floating_control_buttons_widget_) {

[Eliot Courtney, 2017/06/01 07:37:39]

I think removing this code will cause a regression (crbug.com/709862). WDYT?

If we don't remove it though it may potentially continue causing memory
corruption - not sure if this memory corruption is actually triggered though.

[yoshiki, 2017/06/05 04:59:31]

As I checked, the regression is not happened. (although I found an issue of
close button which is not related with this: crbug.com/728915).

If surface_ == null, MaybeCreateFloatingControlButtons() does nothing.

-    // This may update |floating_control_buttons_widget_|.
-    MaybeCreateFloatingControlButtons();
-    if (!floating_control_buttons_widget_)
-      return;
-  }
-
   const bool target_visiblity =
       IsMouseHovered() || (close_button_ && close_button_->HasFocus()) ||
       (settings_button_ && settings_button_->HasFocus());
@@ -417,7 +402,8 @@ void ArcNotificationContentView::UpdateControlButtonsVisibility() {
 }
 
 void ArcNotificationContentView::UpdatePinnedState() {
-  if (!item_)
+  // Surface is not attached yet.
+  if (!control_buttons_view_)

[Eliot Courtney, 2017/06/01 07:37:39]

After removing the call to UpdatePinnedState from AttachSurface, this is only
called from OnItemUpdated AFAICT, which should mean item_ is non-null, but, if a
call to this is added later that doesn't guarantee item_ is non-null it could
crash. Or, if there is a regression of a previous bug where item_ can become
null while OnItemUpdated is executing it could be bad. So, it might be better to
leave the item_ check in

[yoshiki, 2017/06/05 04:59:31]

Done.

     return;
 
   if (item_->GetPinned() && close_button_) {
@@ -457,10 +443,9 @@ void ArcNotificationContentView::AttachSurface() {
   // Invokes Update() in case surface is attached during a slide.
   slide_helper_->Update();
 
-  // Updates pinned state to create or destroy the floating close button
-  // after |surface_| is attached to a widget.
+  // (Re-)create the floating buttons after |surface_| is attached to a widget.
   if (item_)

[Eliot Courtney, 2017/06/01 07:37:39]

I think item_ is checked inside MaybeCreateFloatingControl buttons, so this line
is now not necessary

[yoshiki, 2017/06/05 04:59:31]

Done.

-    UpdatePinnedState();
+    MaybeCreateFloatingControlButtons();

[Eliot Courtney, 2017/06/01 07:37:39]

Doesn't this mean the pinned state potentially isn't updated? I'm not sure if we
are guaranteed that OnItemUpdated will be called after this has executed.

[yoshiki, 2017/06/05 04:59:31]

The pinned state is updated by this method because the floating widget is
recreated. If the notification is pinned, the close button is not added to the
recreated widget.

 }
 
 void ArcNotificationContentView::StartControlButtonsColorAnimation() {