Fix crash on updating notification before attaching

ui/arc/notification/arc_notification_content_view.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 "ui/arc/notification/arc_notification_content_view.h"
 
 #include "ash/wm/window_util.h"
 #include "base/auto_reset.h"
 #include "base/memory/ptr_util.h"
 #include "components/exo/notification_surface.h"
@@ skipping 342 matching lines @@
     surface_->window()->RemoveObserver(this);
     surface_->window()->RemovePreTargetHandler(event_forwarder_.get());
   }
 
   surface_ = surface;
 
   if (surface_ && surface_->window()) {
     surface_->window()->AddObserver(this);
     surface_->window()->AddPreTargetHandler(event_forwarder_.get());
 
-    MaybeCreateFloatingControlButtons();
-
     if (GetWidget())
       AttachSurface();
   }
 }
 
 void ArcNotificationContentView::UpdatePreferredSize() {
   gfx::Size preferred_size;
   if (surface_)
     preferred_size = surface_->GetSize();
   else if (item_)
     preferred_size = item_->GetSnapshot().size();
 
   if (preferred_size.IsEmpty())
     return;
 
   if (preferred_size.width() != message_center::kNotificationWidth) {
     const float scale = static_cast<float>(message_center::kNotificationWidth) /
                         preferred_size.width();
     preferred_size.SetSize(message_center::kNotificationWidth,
                            preferred_size.height() * scale);
   }
 
   SetPreferredSize(preferred_size);
 }
 
 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());
   if (target_visiblity == floating_control_buttons_widget_->IsVisible())
     return;
 
   if (target_visiblity)
     floating_control_buttons_widget_->Show();
   else
     floating_control_buttons_widget_->Hide();
 }
 
 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_) {
     control_buttons_view_->RemoveChildView(close_button_.get());
     close_button_.reset();
     Layout();
   } else if (!item_->GetPinned() && !close_button_) {
     CreateCloseButton();
     Layout();
   }
@@ skipping 19 matching lines @@
   // with fractional scale factor. Force to align it at the pixel
   // boundary here, and when layout is updated in Layout().
   ash::wm::SnapWindowToPixelBoundary(surface_->window());
 
   // Creates slide helper after this view is added to its parent.
   slide_helper_.reset(new SlideHelper(this));
 
   // 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() {
   if (control_button_color_animation_)
     control_button_color_animation_->End();
   control_button_color_animation_.reset(new gfx::LinearAnimation(this));
   control_button_color_animation_->SetDuration(kBackgroundColorChangeDuration);
   control_button_color_animation_->Start();
 }
 
@@ skipping 252 matching lines @@
     }
     if (close_button_) {
       close_button_->set_background(
           views::Background::CreateSolidBackground(current_color));
       close_button_->SchedulePaint();
     }
   }
 }
 
 }  // namespace arc