Linux native notifications: Add initial unit test

chrome/browser/notifications/notification_platform_bridge_linux.cc

 // Copyright 2017 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 "chrome/browser/notifications/notification_platform_bridge_linux.h"
 
 #include <algorithm>
 
 #include "base/barrier_closure.h"
 #include "base/files/file_util.h"
@@ skipping 109 matching lines @@
 // static
 NotificationPlatformBridge* NotificationPlatformBridge::Create() {
   return new NotificationPlatformBridgeLinux();
 }
 
 class NotificationPlatformBridgeLinuxImpl
     : public NotificationPlatformBridge,
       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));
   }
 
   void Display(NotificationCommon::Type notification_type,
                const std::string& notification_id,
                const std::string& profile_id,
                bool is_incognito,
                const Notification& notification) override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ skipping 35 matching lines @@
 
   void SetReadyCallback(NotificationBridgeReadyCallback callback) override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     if (connected_.has_value()) {
       std::move(callback).Run(connected_.value());
     } else {
       on_connected_callbacks_.push_back(std::move(callback));
     }
   }
 
+  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>;
 
   struct ResourceFile {
     explicit ResourceFile(const base::FilePath& file_path)
         : file_path(file_path) {}
     ~ResourceFile() { base::DeleteFile(file_path, false); }
     const base::FilePath file_path;
   };
 
@@ skipping 30 matching lines @@
     size_t action_start = 0;
     size_t action_end = 0;
 
     // Temporary resource files associated with the notification that
     // should be cleaned up when the notification is closed or on
     // shutdown.
     std::vector<std::unique_ptr<ResourceFile>> resource_files;
   };
 
   ~NotificationPlatformBridgeLinuxImpl() override {
+    DCHECK(cleanup_posted_);
     DCHECK(!bus_);
+    DCHECK(!notification_proxy_);
     DCHECK(notifications_.empty());
   }
 
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     DCHECK_EQ(chrome::NOTIFICATION_APP_TERMINATING, type);
     // The browser process is about to exit.  Post the CleanUp() task
     // while we still can.
@@ skipping 10 matching lines @@
   void PostTaskToTaskRunnerThread(base::OnceClosure closure) const {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     DCHECK(task_runner_);
     bool success = task_runner_->PostTask(FROM_HERE, std::move(closure));
     DCHECK(success);
   }
 
   // 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,
                              dbus::ObjectPath(kFreedesktopNotificationsPath));
     if (!notification_proxy_) {
       OnConnectionInitializationFinishedOnTaskRunner(false);
       return;
     }
 
     connected_signals_barrier_ = base::BarrierClosure(
         2, base::Bind(&NotificationPlatformBridgeLinuxImpl::
                           OnConnectionInitializationFinishedOnTaskRunner,
                       this, true));
     notification_proxy_->ConnectToSignal(
         kFreedesktopNotificationsName, "ActionInvoked",
         base::Bind(&NotificationPlatformBridgeLinuxImpl::OnActionInvoked, this),
         base::Bind(&NotificationPlatformBridgeLinuxImpl::OnSignalConnected,
                    this));
     notification_proxy_->ConnectToSignal(
         kFreedesktopNotificationsName, "NotificationClosed",
         base::Bind(&NotificationPlatformBridgeLinuxImpl::OnNotificationClosed,
                    this),
         base::Bind(&NotificationPlatformBridgeLinuxImpl::OnSignalConnected,
                    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();
     bus_ = nullptr;
     notification_proxy_ = nullptr;
     notifications_.clear();
   }
 
   // Makes the "Notify" call to D-Bus.
   void DisplayOnTaskRunner(NotificationCommon::Type notification_type,
@@ skipping 280 matching lines @@
   // argument, so the data would get double-destructed.
   template <typename T>
   using UnorderedUniqueSet = std::unordered_map<T*, std::unique_ptr<T>>;
 
   UnorderedUniqueSet<NotificationData> notifications_;
 
   DISALLOW_COPY_AND_ASSIGN(NotificationPlatformBridgeLinuxImpl);
 };
 
 NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux()
-    : impl_(new NotificationPlatformBridgeLinuxImpl()) {}
+    : NotificationPlatformBridgeLinux(nullptr) {}
+
+NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux(
+    scoped_refptr<dbus::Bus> bus)
+    : impl_(new NotificationPlatformBridgeLinuxImpl(bus)) {
+  impl_->InitAfterConstructor();
+}
 
 NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() = default;
 
 void NotificationPlatformBridgeLinux::Display(
     NotificationCommon::Type notification_type,
     const std::string& notification_id,
     const std::string& profile_id,
     bool is_incognito,
     const Notification& notification) {
   impl_->Display(notification_type, notification_id, profile_id, is_incognito,
@@ skipping 10 matching lines @@
     const std::string& profile_id,
     bool incognito,
     const GetDisplayedNotificationsCallback& callback) const {
   impl_->GetDisplayed(profile_id, incognito, callback);
 }
 
 void NotificationPlatformBridgeLinux::SetReadyCallback(
     NotificationBridgeReadyCallback callback) {
   impl_->SetReadyCallback(std::move(callback));
 }
+
+void NotificationPlatformBridgeLinux::CleanUp() {
+  impl_->CleanUp();
+}