Linux native notifications: Add initial unit test

chrome/browser/notifications/notification_platform_bridge_linux.cc

Index: chrome/browser/notifications/notification_platform_bridge_linux.cc
diff --git a/chrome/browser/notifications/notification_platform_bridge_linux.cc b/chrome/browser/notifications/notification_platform_bridge_linux.cc
index cf71db8869d0a8a1c7c5f89085a7fd8338243d6c..f0f34a0742f86ea63d03baad46e7acf98ac218ad 100644
--- a/chrome/browser/notifications/notification_platform_bridge_linux.cc
+++ b/chrome/browser/notifications/notification_platform_bridge_linux.cc
@@ -127,12 +127,22 @@ class NotificationPlatformBridgeLinuxImpl
       public content::NotificationObserver,
       public base::RefCountedThreadSafe<NotificationPlatformBridgeLinuxImpl> {
  public:
-  NotificationPlatformBridgeLinuxImpl()
-      : task_runner_(base::CreateSingleThreadTaskRunnerWithTraits(
-            base::TaskTraits().MayBlock().WithPriority(
-                base::TaskPriority::USER_BLOCKING))) {
+  explicit NotificationPlatformBridgeLinuxImpl(scoped_refptr<dbus::Bus> bus)
+      : bus_(bus) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    task_runner_ = base::CreateSingleThreadTaskRunnerWithTraits(
+        base::TaskTraits().MayBlock().WithPriority(
+            base::TaskPriority::USER_BLOCKING));
     registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
                    content::NotificationService::AllSources());
+  }
+
+  // Init() cannot be called from within the constructor because of a
+  // race condition.  The reference count for |this| starts out as 0.
+  // Posting the Init task would increment the count to 1.  If the
+  // task finishes before the constructor returns, the count will go
+  // to 0 and the object would be prematurely destructed.

[Peter Beverloo, 2017/05/03 00:31:26]

Thank you for the detailed comment!

+  void InitAfterConstructor() {

[Lei Zhang, 2017/05/03 01:10:45]

This can be Init(), and Init() can be InitOnTaskRunner()? To be more consistent
with Display/DisplayOnTaskRunner?

[Tom (Use chromium acct), 2017/05/03 01:22:17]

Done.

     PostTaskToTaskRunnerThread(
         base::BindOnce(&NotificationPlatformBridgeLinuxImpl::Init, this));
   }
@@ -188,6 +198,12 @@ class NotificationPlatformBridgeLinuxImpl
     }
   }
 
+  void CleanUp() {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    PostTaskToTaskRunnerThread(base::BindOnce(
+        &NotificationPlatformBridgeLinuxImpl::CleanUpOnTaskRunner, this));
+  }
+
  private:
   friend class base::RefCountedThreadSafe<NotificationPlatformBridgeLinuxImpl>;
 
@@ -239,6 +255,7 @@ class NotificationPlatformBridgeLinuxImpl
 
   ~NotificationPlatformBridgeLinuxImpl() override {
     DCHECK(!bus_);
+    DCHECK(!notification_proxy_);
     DCHECK(notifications_.empty());
   }
 
@@ -269,11 +286,13 @@ class NotificationPlatformBridgeLinuxImpl
   // Sets up the D-Bus connection.
   void Init() {
     DCHECK(task_runner_->RunsTasksOnCurrentThread());
-    dbus::Bus::Options bus_options;
-    bus_options.bus_type = dbus::Bus::SESSION;
-    bus_options.connection_type = dbus::Bus::PRIVATE;
-    bus_options.dbus_task_runner = task_runner_;
-    bus_ = make_scoped_refptr(new dbus::Bus(bus_options));
+    if (!bus_) {

[Lei Zhang, 2017/05/03 01:10:46]

May be good to mention this happens under normal conditions, and we only skip
this if a test provided |bus_| already.

[Tom (Use chromium acct), 2017/05/03 01:22:17]

Done.

+      dbus::Bus::Options bus_options;
+      bus_options.bus_type = dbus::Bus::SESSION;
+      bus_options.connection_type = dbus::Bus::PRIVATE;
+      bus_options.dbus_task_runner = task_runner_;
+      bus_ = make_scoped_refptr(new dbus::Bus(bus_options));
+    }
 
     notification_proxy_ =
         bus_->GetObjectProxy(kFreedesktopNotificationsName,
@@ -300,18 +319,10 @@ class NotificationPlatformBridgeLinuxImpl
                    this));
   }
 
-  void CleanUp() {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    if (cleanup_posted_)
-      return;
-    PostTaskToTaskRunnerThread(base::BindOnce(
-        &NotificationPlatformBridgeLinuxImpl::CleanUpOnTaskRunner, this));
-    cleanup_posted_ = true;
-  }
-
   void CleanUpOnTaskRunner() {
     DCHECK(task_runner_->RunsTasksOnCurrentThread());
-    bus_->ShutdownAndBlock();
+    if (bus_)
+      bus_->ShutdownAndBlock();
     bus_ = nullptr;
     notification_proxy_ = nullptr;
     notifications_.clear();
@@ -586,8 +597,6 @@ class NotificationPlatformBridgeLinuxImpl
   base::Optional<bool> connected_;
   std::vector<NotificationBridgeReadyCallback> on_connected_callbacks_;
 
-  bool cleanup_posted_ = false;
-
   //////////////////////////////////////////////////////////////////////////////
   // Members used only on the task runner thread.
 
@@ -609,7 +618,13 @@ class NotificationPlatformBridgeLinuxImpl
 };
 
 NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux()
-    : impl_(new NotificationPlatformBridgeLinuxImpl()) {}
+    : NotificationPlatformBridgeLinux(nullptr) {}
+
+NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux(
+    scoped_refptr<dbus::Bus> bus)
+    : impl_(new NotificationPlatformBridgeLinuxImpl(bus)) {
+  impl_->InitAfterConstructor();
+}
 
 NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() = default;
 
@@ -640,3 +655,7 @@ void NotificationPlatformBridgeLinux::SetReadyCallback(
     NotificationBridgeReadyCallback callback) {
   impl_->SetReadyCallback(std::move(callback));
 }
+
+void NotificationPlatformBridgeLinux::CleanUp() {
+  impl_->CleanUp();
+}