[Backport] Stash BatteryStatusService in SequenceLocalStorageSlot

The Device Service needs to clean up parts of its internal state as part
of browser shutdown, notably, BatteryStatusService. However, it also
needs to run on the UI thread, and embedded services that run on the UI
thread are not guaranteed to be destroyed as part of browser shutdown
(tasks to destroy these services are posted from the IO thread by
ServiceManagerConnectionImpl::ShutDownOnIOThread, but the UI thread is
typically shut down before these posted tasks are run).

To solve this issue we discussed adding plumbing wherein embedded
services could inform //content that they wanted to be notified when
shutdown was occurring on the main thread. However, on investigation
this plumbing would be painful to implement: it is only
EmbeddedInstanceManager that has direct information of these service
instances, and that object lives far away from //content's
ServiceManagerContext, the object that knows when shutdown is occurring
on the main thread.

This CL takes an alternative approach of having the BatteryStatusService
object be stashed in a SequenceLocalStorageSlot to ensure that it is
destroyed on browser shutdown. I have verified that its destruction is
triggered on shutdown of content_shell.

This CL is conceptually a reland. The original CL (which had the Device
Service directly observe destruction of the UI thread) was reverted for
the following reason:

"crbug.com/856771 presents a crash that it introduced on ChromeOS: the
Device Service instance is now shut down after the DBusThreadManager
global instance, on which it implicitly depends."

However, crbug.com/794105 has continued to be a problem on Windows,
and there aren't active engineering resources for an alternative solution
to this one.  Hence, this CL relands an approach that is similar to the
original fix with the shutdown not being applied on ChromeOS to avoid
triggering crbug.com/856771. As I wrote in the revert, "this is all
sad-making, but c'est la vie."

The usage of SequenceLocalStorageSlot rather than direct observation of
MessageLoop is thanks to a suggestion from rockot@.

Bug: 794105, 856771
Change-Id: I700470b12bc8c4d41406657923c25324e0ec05f4
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#665919}
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

3 files changed, 40 insertions, 12 deletions

diff --git a/chromium/base/win/message_window.cc b/chromium/base/win/message_window.cc
index fe71e29deb6..7fb5039298a 100644
--- a/chromium/base/win/message_window.cc
+++ b/chromium/base/win/message_window.cc
@@ -65,9 +65,7 @@ MessageWindow::WindowClass::~WindowClass() {
     // leaked. For example not calling
     // ui::Clipboard::DestroyClipboardForCurrentThread() results in a leaked
     // MessageWindow.
-    //DCHECK(result);
-    if (!result)
-        DPLOG(WARNING) << "Some MessageWindow objects were leaked at shutdown";
+    DCHECK(result);
   }
 }
 
diff --git a/chromium/services/device/battery/battery_status_service.cc b/chromium/services/device/battery/battery_status_service.cc
index 8b0e8c4cb28..0aef81fe995 100644
--- a/chromium/services/device/battery/battery_status_service.cc
+++ b/chromium/services/device/battery/battery_status_service.cc
@@ -8,7 +8,9 @@
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/no_destructor.h"
 #include "base/single_thread_task_runner.h"
+#include "base/threading/sequence_local_storage_slot.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "services/device/battery/battery_monitor_impl.h"
 #include "services/device/battery/battery_status_manager.h"
@@ -25,12 +27,39 @@ BatteryStatusService::BatteryStatusService()
       &BatteryStatusService::ConsumersChanged, base::Unretained(this)));
 }
 
-BatteryStatusService::~BatteryStatusService() {}
+BatteryStatusService::~BatteryStatusService() {
+    Shutdown();
+}
 
 BatteryStatusService* BatteryStatusService::GetInstance() {
-  return base::Singleton<
-      BatteryStatusService,
-      base::LeakySingletonTraits<BatteryStatusService>>::get();
+  // On embedder teardown, the BatteryStatusService object needs to shut down
+  // certain parts of its state to avoid DCHECKs going off on Windows (see.
+  // https://crbug.com/794105 for details). However, when used in the context of
+  // the browser the Device Service is not guaranteed to have its destructor
+  // run, as the sequence on which it runs is stopped before the task posted to
+  // run the Device Service destructor is run. Hence, stash the
+  // BatteryStatusService singleton in a SequenceLocalStorageSlot to ensure that
+  // its destructor (and consequently Shutdown() method) are run on embedder
+  // teardown. Unfortunately, this shutdown is currently not possible on
+  // ChromeOS: crbug.com/856771 presents a crash that performing this shutdown
+  // introduces on that platform, as it causes the BatteryStatusService instance
+  // to be shut down after the DBusThreadManager global instance, on which it
+  // implicitly depends.
+#if defined(OS_CHROMEOS)
+  static base::NoDestructor<BatteryStatusService> service_wrapper;
+  return service_wrapper.get();
+#else
+  static base::NoDestructor<
+      base::SequenceLocalStorageSlot<std::unique_ptr<BatteryStatusService>>>
+      service_local_storage_slot_wrapper;
+
+  auto& service_local_storage_slot = *service_local_storage_slot_wrapper;
+
+  if (!service_local_storage_slot)
+    service_local_storage_slot.emplace(new BatteryStatusService());
+
+  return service_local_storage_slot->get();
+#endif
 }
 
 std::unique_ptr<BatteryStatusService::BatteryUpdateSubscription>
diff --git a/chromium/services/device/battery/battery_status_service.h b/chromium/services/device/battery/battery_status_service.h
index bb6171bc830..4992788fada 100644
--- a/chromium/services/device/battery/battery_status_service.h
+++ b/chromium/services/device/battery/battery_status_service.h
@@ -9,7 +9,6 @@
 
 #include "base/callback_list.h"
 #include "base/macros.h"
-#include "base/memory/singleton.h"
 #include "services/device/public/mojom/battery_status.mojom.h"
 
 namespace base {
@@ -30,6 +29,12 @@ class BatteryStatusService {
   // Returns the BatteryStatusService singleton.
   static BatteryStatusService* GetInstance();
 
+  // NOTE: These must be public due to internal stashing of the global
+  // BatteryStatusService object in an std::unique_ptr. Clients should use only
+  // the static GetInstance() method above.
+  BatteryStatusService();
+  virtual ~BatteryStatusService();
+
   // Adds a callback to receive battery status updates.  Must be called on the
   // main thread. The callback itself will be called on the main thread as well.
   // NOTE: The callback may be run before AddCallback returns!
@@ -47,12 +52,8 @@ class BatteryStatusService {
   const BatteryUpdateCallback& GetUpdateCallbackForTesting() const;
 
  private:
-  friend struct base::DefaultSingletonTraits<BatteryStatusService>;
   friend class BatteryStatusServiceTest;
 
-  BatteryStatusService();
-  virtual ~BatteryStatusService();
-
   // Updates current battery status and sends new status to interested
   // render processes. Can be called on any thread via a callback.
   void NotifyConsumers(const mojom::BatteryStatus& status);