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..e8ff4dd8a239f9a68077ffadcd7656cdd4abf7f9 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.
+  void InitAfterConstructor() {
     PostTaskToTaskRunnerThread(
         base::BindOnce(&NotificationPlatformBridgeLinuxImpl::Init, this));
   }
@@ -188,6 +198,15 @@ class NotificationPlatformBridgeLinuxImpl
     }
   }
 
+  void CleanUp() {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    if (cleanup_posted_)
+      return;
+    PostTaskToTaskRunnerThread(base::BindOnce(
+        &NotificationPlatformBridgeLinuxImpl::CleanUpOnTaskRunner, this));
+    cleanup_posted_ = true;

[Peter Beverloo, 2017/05/02 13:50:21]

Instead of introducing cleanup_posted_, can we just check whether bus_ is
non-null? Posting the task will keep the instance alive until it runs, so it's a
fairly good approximation.

[Tom (Use chromium acct), 2017/05/02 21:23:16]

Done.

FYI I added |cleanup_posted_| to catch the case where initialization failed and
|bus_| was null, but we still want to be explicit about posting the cleanup
task.  But now that I think about it, if initialization failed I guess we don't
need to cleanup anyway.

Also, |bus_| is only supposed to be used on the task runner.  CleanUp() must now
unconditionally post CleanUpOnTaskRunner() to avoid a race condition where bus_
is null but Init() is in the task runner queue.  I changed CleanUpOnTaskRunner()
so that it is safe to call more than once.

+  }
+
  private:
   friend class base::RefCountedThreadSafe<NotificationPlatformBridgeLinuxImpl>;
 
@@ -238,7 +257,9 @@ class NotificationPlatformBridgeLinuxImpl
   };
 
   ~NotificationPlatformBridgeLinuxImpl() override {
+    DCHECK(cleanup_posted_);
     DCHECK(!bus_);
+    DCHECK(!notification_proxy_);
     DCHECK(notifications_.empty());
   }
 
@@ -269,11 +290,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_) {
+      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,15 +323,6 @@ 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();
@@ -609,7 +623,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 +660,7 @@ void NotificationPlatformBridgeLinux::SetReadyCallback(
     NotificationBridgeReadyCallback callback) {
   impl_->SetReadyCallback(std::move(callback));
 }
+
+void NotificationPlatformBridgeLinux::CleanUp() {
+  impl_->CleanUp();
+}