Replace base::CallbackList with base::ObserverList in CastConfigDelegate

base::CallbackList::Observer invoked a destructor inside of TrayCast. Due to wonky deinitialization order between the system tray delegate and the status area, this caused a crash.

base::ObserverList deinitialization is more easily controlled than base::CallbackList. Switch to that so we don't deinitialize the observer if it's already been removed.

Fixing the deinitialization order between the system tray delegate and the status area is the correct fix, but it will take significantly more time and needs to be thoroughly tested.

BUG=574874

Review URL: https://codereview.chromium.org/1567103005

Cr-Commit-Position: refs/heads/master@{#369508}

ash/cast_config_delegate.h

@@ -9,8 +9,6 @@
 #include <vector>
 
 #include "ash/ash_export.h"
-#include "base/callback.h"
-#include "base/callback_list.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
@@ -71,12 +69,19 @@ class CastConfigDelegate {
     Activity activity;
   };
 
-  // The key is the receiver id.
   using ReceiversAndActivities = std::vector<ReceiverAndActivity>;
-  using ReceiversAndActivitesCallback =
-      base::Callback<void(const ReceiversAndActivities&)>;
-  using DeviceUpdateSubscription = scoped_ptr<
-      base::CallbackList<void(const ReceiversAndActivities&)>::Subscription>;
+
+  class ASH_EXPORT Observer {
+   public:
+    // Invoked whenever there is new receiver or activity information available.
+    virtual void OnDevicesUpdated(const ReceiversAndActivities& devices) = 0;
+
+   protected:
+    virtual ~Observer() {}
+
+   private:
+    DISALLOW_ASSIGN(Observer);
+  };
 
   virtual ~CastConfigDelegate() {}
 
@@ -85,11 +90,6 @@ class CastConfigDelegate {
   // gone. See crbug.com/551132.
   virtual bool HasCastExtension() const = 0;
 
-  // Adds a listener that will get invoked whenever the receivers or their
-  // associated activites have changed.
-  virtual DeviceUpdateSubscription RegisterDeviceUpdateObserver(
-      const ReceiversAndActivitesCallback& callback) = 0;
-
   // Request fresh data from the backend. When the data is available, all
   // registered observers will get called.
   virtual void RequestDeviceRefresh() = 0;
@@ -111,6 +111,10 @@ class CastConfigDelegate {
   // gone. See crbug.com/551132.
   virtual void LaunchCastOptions() = 0;
 
+  // Add or remove an observer.
+  virtual void AddObserver(Observer* observer) = 0;
+  virtual void RemoveObserver(Observer* observer) = 0;
+
  private:
   DISALLOW_ASSIGN(CastConfigDelegate);
 };

ash/system/cast/tray_cast.cc

@@ -42,6 +42,12 @@ const int kStopButtonRightPadding = 18;
 
 // Returns the active CastConfigDelegate instance.
 ash::CastConfigDelegate* GetCastConfigDelegate() {
+  // When shutting down Chrome, there may not be a shell or a delegate instance.
+  if (!ash::Shell::GetInstance() ||
+      !ash::Shell::GetInstance()->system_tray_delegate()) {
+    return nullptr;
+  }
+
   return ash::Shell::GetInstance()
       ->system_tray_delegate()
       ->GetCastConfigDelegate();
@@ -125,12 +131,11 @@ class CastCastView : public views::View, public views::ButtonListener {
   views::ImageView* icon_;
   views::Label* label_;
   TrayPopupLabelButton* stop_button_;
-  base::WeakPtrFactory<CastCastView> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CastCastView);
 };
 
-CastCastView::CastCastView() : weak_ptr_factory_(this) {
+CastCastView::CastCastView() {
   // We will initialize the primary tray view which shows a stop button here.
 
   set_background(views::Background::CreateSolidBackground(kBackgroundColor));
@@ -412,7 +417,6 @@ class CastDetailedView : public TrayDetailsView, public ViewClickListener {
       receivers_and_activities_;
   // A mapping from the view pointer to the associated activity id.
   std::map<views::View*, std::string> receiver_activity_map_;
-  base::WeakPtrFactory<CastDetailedView> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CastDetailedView);
 };
@@ -421,7 +425,7 @@ CastDetailedView::CastDetailedView(
     SystemTrayItem* owner,
     user::LoginStatus login,
     const CastConfigDelegate::ReceiversAndActivities& receivers_and_activities)
-    : TrayDetailsView(owner), login_(login), weak_ptr_factory_(this) {
+    : TrayDetailsView(owner), login_(login) {
   CreateItems();
   UpdateReceiverList(receivers_and_activities);
 }
@@ -551,14 +555,19 @@ void CastDetailedView::OnViewClicked(views::View* sender) {
 
 }  // namespace tray
 
-TrayCast::TrayCast(SystemTray* system_tray)
-    : SystemTrayItem(system_tray),
-      weak_ptr_factory_(this) {
+TrayCast::TrayCast(SystemTray* system_tray) : SystemTrayItem(system_tray) {
   Shell::GetInstance()->AddShellObserver(this);
 }
 
 TrayCast::~TrayCast() {
-  Shell::GetInstance()->RemoveShellObserver(this);
+  // TODO(jdufault): Remove these if checks (and the ones in
+  // GetCastConfigDelegate) by fixing deinit order. See crbug.com/577413.
+  if (Shell::GetInstance())
+    Shell::GetInstance()->RemoveShellObserver(this);
+
+  ash::CastConfigDelegate* cast_config_delegate = GetCastConfigDelegate();
+  if (added_observer_ && cast_config_delegate)
+    cast_config_delegate->RemoveObserver(this);
 }
 
 void TrayCast::StartCastForTest(const std::string& receiver_id) {
@@ -591,16 +600,16 @@ views::View* TrayCast::CreateDefaultView(user::LoginStatus status) {
   if (HasCastExtension()) {
     ash::CastConfigDelegate* cast_config_delegate = GetCastConfigDelegate();
 
-    // We add the cast listener here instead of in the ctor for two reasons:
+    // Add the cast observer here instead of the ctor for two reasons:
     // - The ctor gets called too early in the initialization cycle (at least
     //   for the tests); the correct profile hasn't been setup yet.
-    // - The listener is only added if there is a cast extension. If the call
-    //   below were in the ctor, then the cast tray item would not appear if the
-    //   user installed the extension in an existing session.
-    if (!device_update_subscription_) {
-      device_update_subscription_ =
-          cast_config_delegate->RegisterDeviceUpdateObserver(base::Bind(
-              &TrayCast::OnReceiversUpdated, weak_ptr_factory_.GetWeakPtr()));
+    // - If we're using the cast extension backend (media router is disabled),
+    //   then the user can install the extension at any point in time. The
+    //   return value of HasCastExtension() can change, so only checking it in
+    //   the ctor isn't enough.
+    if (!added_observer_) {
+      cast_config_delegate->AddObserver(this);
+      added_observer_ = true;
     }
 
     // The extension updates its view model whenever the popup is opened, so we
@@ -645,7 +654,7 @@ bool TrayCast::HasCastExtension() {
          cast_config_delegate->HasCastExtension();
 }
 
-void TrayCast::OnReceiversUpdated(
+void TrayCast::OnDevicesUpdated(
     const CastConfigDelegate::ReceiversAndActivities& receivers_activities) {
   receivers_and_activities_ = receivers_activities;
 

ash/system/cast/tray_cast.h

@@ -9,7 +9,6 @@
 #include "ash/shell_observer.h"
 #include "ash/system/tray/system_tray_item.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 
 namespace ash {
 namespace tray {
@@ -19,7 +18,9 @@ class CastDetailedView;
 class CastDuplexView;
 }  // namespace tray
 
-class ASH_EXPORT TrayCast : public SystemTrayItem, public ShellObserver {
+class ASH_EXPORT TrayCast : public SystemTrayItem,
+                            public ShellObserver,
+                            public CastConfigDelegate::Observer {
  public:
   explicit TrayCast(SystemTray* system_tray);
   ~TrayCast() override;
@@ -47,14 +48,13 @@ class ASH_EXPORT TrayCast : public SystemTrayItem, public ShellObserver {
   // Overridden from ShellObserver.
   void OnCastingSessionStartedOrStopped(bool started) override;
 
+  // Overridden from CastConfigDelegate::Observer.
+  void OnDevicesUpdated(
+      const CastConfigDelegate::ReceiversAndActivities& devices) override;
+
   // Returns true if the cast extension was detected.
   bool HasCastExtension();
 
-  // Callback used to enable/disable the begin casting view depending on
-  // if we have any cast receivers.
-  void OnReceiversUpdated(
-      const CastConfigDelegate::ReceiversAndActivities& receivers_activities);
-
   // This makes sure that the current view displayed in the tray is the correct
   // one, depending on if we are currently casting. If we're casting, then a
   // view with a stop button is displayed; otherwise, a view that links to a
@@ -63,16 +63,15 @@ class ASH_EXPORT TrayCast : public SystemTrayItem, public ShellObserver {
   void UpdatePrimaryView();
 
   CastConfigDelegate::ReceiversAndActivities receivers_and_activities_;
-  CastConfigDelegate::DeviceUpdateSubscription device_update_subscription_;
   bool is_casting_ = false;
 
+  bool added_observer_ = false;
+
   // Not owned.
   tray::CastTrayView* tray_ = nullptr;
   tray::CastDuplexView* default_ = nullptr;
   tray::CastDetailedView* detailed_ = nullptr;
 
-  base::WeakPtrFactory<TrayCast> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(TrayCast);
 };
 

ash/test/tray_cast_test_api.cc

@@ -4,7 +4,10 @@
 
 #include "ash/test/tray_cast_test_api.h"
 
+#include "ash/cast_config_delegate.h"
+#include "ash/shell.h"
 #include "ash/system/tray/system_tray.h"
+#include "ash/system/tray/system_tray_delegate.h"
 #include "ui/views/view.h"
 
 namespace ash {
@@ -46,7 +49,15 @@ void TrayCastTestAPI::OnCastingSessionStartedOrStopped(bool is_casting) {
 }
 
 void TrayCastTestAPI::ReleaseConfigCallbacks() {
-  tray_cast_->device_update_subscription_.reset();
+  tray_cast_->added_observer_ = false;
+
+  if (ash::Shell::GetInstance() &&
+      ash::Shell::GetInstance()->system_tray_delegate()) {
+    ash::Shell::GetInstance()
+        ->system_tray_delegate()
+        ->GetCastConfigDelegate()
+        ->RemoveObserver(tray_cast_);
+  }
 }
 
 bool TrayCastTestAPI::IsViewDrawn(TrayCast::ChildViewId id) const {

chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc

@@ -65,15 +65,20 @@ CastDeviceUpdateListeners::CastDeviceUpdateListeners(
 
 CastDeviceUpdateListeners::~CastDeviceUpdateListeners() {}
 
-ash::CastConfigDelegate::DeviceUpdateSubscription
-CastDeviceUpdateListeners::RegisterCallback(
-    const ash::CastConfigDelegate::ReceiversAndActivitesCallback& callback) {
-  return callback_list_.Add(callback);
+void CastDeviceUpdateListeners::AddObserver(
+    ash::CastConfigDelegate::Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void CastDeviceUpdateListeners::RemoveObserver(
+    ash::CastConfigDelegate::Observer* observer) {
+  observer_list_.RemoveObserver(observer);
 }
 
 void CastDeviceUpdateListeners::NotifyCallbacks(
     const ReceiverAndActivityList& devices) {
-  callback_list_.Notify(devices);
+  FOR_EACH_OBSERVER(ash::CastConfigDelegate::Observer, observer_list_,
+                    OnDevicesUpdated(devices));
 }
 
 CastDevicesPrivateUpdateDevicesFunction::

chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h

@@ -10,6 +10,7 @@
 #include "ash/cast_config_delegate.h"
 #include "base/callback_list.h"
 #include "base/macros.h"
+#include "base/observer_list.h"
 #include "extensions/browser/browser_context_keyed_api_factory.h"
 #include "extensions/browser/extension_function.h"
 
@@ -20,13 +21,12 @@ class CastDeviceUpdateListeners : public BrowserContextKeyedAPI {
   explicit CastDeviceUpdateListeners(content::BrowserContext* context);
   ~CastDeviceUpdateListeners() override;
 
-  // Fetch an instance for the given context.
+  // Fetches an instance for the given context.
   static CastDeviceUpdateListeners* Get(content::BrowserContext* context);
 
-  // Register a function that will be invoked only when a new device update is
-  // available.
-  ash::CastConfigDelegate::DeviceUpdateSubscription RegisterCallback(
-      const ash::CastConfigDelegate::ReceiversAndActivitesCallback& callback);
+  // Adds an observer that will be invoked when new device data is available.
+  void AddObserver(ash::CastConfigDelegate::Observer* observer);
+  void RemoveObserver(ash::CastConfigDelegate::Observer* observer);
 
   // BrowserContextKeyedAPI implementation:
   static BrowserContextKeyedAPIFactory<CastDeviceUpdateListeners>*
@@ -40,7 +40,7 @@ class CastDeviceUpdateListeners : public BrowserContextKeyedAPI {
   friend class CastDevicesPrivateUpdateDevicesFunction;  // For NotifyCallbacks.
   void NotifyCallbacks(const ReceiverAndActivityList& devices);
 
-  base::CallbackList<void(const ReceiverAndActivityList&)> callback_list_;
+  base::ObserverList<ash::CastConfigDelegate::Observer> observer_list_;
 
   friend class BrowserContextKeyedAPIFactory<CastDeviceUpdateListeners>;
 

chrome/browser/ui/ash/cast_config_delegate_chromeos.cc

@@ -65,13 +65,6 @@ bool CastConfigDelegateChromeos::HasCastExtension() const {
   return FindCastExtension() != nullptr;
 }
 
-CastConfigDelegateChromeos::DeviceUpdateSubscription
-CastConfigDelegateChromeos::RegisterDeviceUpdateObserver(
-    const ReceiversAndActivitesCallback& callback) {
-  auto listeners = extensions::CastDeviceUpdateListeners::Get(GetProfile());
-  return listeners->RegisterCallback(callback);
-}
-
 void CastConfigDelegateChromeos::RequestDeviceRefresh() {
   scoped_ptr<base::ListValue> args =
       extensions::api::cast_devices_private::UpdateDevicesRequested::Create();
@@ -120,4 +113,16 @@ void CastConfigDelegateChromeos::LaunchCastOptions() {
   chrome::Navigate(&params);
 }
 
+void CastConfigDelegateChromeos::AddObserver(
+    ash::CastConfigDelegate::Observer* observer) {
+  return extensions::CastDeviceUpdateListeners::Get(GetProfile())
+      ->AddObserver(observer);
+}
+
+void CastConfigDelegateChromeos::RemoveObserver(
+    ash::CastConfigDelegate::Observer* observer) {
+  return extensions::CastDeviceUpdateListeners::Get(GetProfile())
+      ->RemoveObserver(observer);
+}
+
 }  // namespace chromeos

chrome/browser/ui/ash/cast_config_delegate_chromeos.h

@@ -23,13 +23,13 @@ class CastConfigDelegateChromeos : public ash::CastConfigDelegate {
  private:
   // CastConfigDelegate:
   bool HasCastExtension() const override;
-  DeviceUpdateSubscription RegisterDeviceUpdateObserver(
-      const ReceiversAndActivitesCallback& callback) override;
   void RequestDeviceRefresh() override;
   void CastToReceiver(const std::string& receiver_id) override;
   void StopCasting(const std::string& activity_id) override;
   bool HasOptions() const override;
   void LaunchCastOptions() override;
+  void AddObserver(ash::CastConfigDelegate::Observer* observer) override;
+  void RemoveObserver(ash::CastConfigDelegate::Observer* observer) override;
 
   DISALLOW_COPY_AND_ASSIGN(CastConfigDelegateChromeos);
 };

chrome/browser/ui/ash/cast_config_delegate_media_router.cc

@@ -136,12 +136,6 @@ bool CastConfigDelegateMediaRouter::HasCastExtension() const {
   return true;
 }
 
-CastConfigDelegateMediaRouter::DeviceUpdateSubscription
-CastConfigDelegateMediaRouter::RegisterDeviceUpdateObserver(
-    const ReceiversAndActivitesCallback& callback) {
-  return callback_list_.Add(callback);
-}
-
 void CastConfigDelegateMediaRouter::RequestDeviceRefresh() {
   // The media router component isn't ready yet.
   if (!devices())
@@ -192,7 +186,8 @@ void CastConfigDelegateMediaRouter::RequestDeviceRefresh() {
     }
   }
 
-  callback_list_.Notify(items);
+  FOR_EACH_OBSERVER(ash::CastConfigDelegate::Observer, observer_list_,
+                    OnDevicesUpdated(items));
 }
 
 void CastConfigDelegateMediaRouter::CastToReceiver(
@@ -213,3 +208,13 @@ bool CastConfigDelegateMediaRouter::HasOptions() const {
 }
 
 void CastConfigDelegateMediaRouter::LaunchCastOptions() {}
+
+void CastConfigDelegateMediaRouter::AddObserver(
+    ash::CastConfigDelegate::Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void CastConfigDelegateMediaRouter::RemoveObserver(
+    ash::CastConfigDelegate::Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}