Subscription lifetime management in TabLifecycleUnitSource is improved.

TabLifecycleUnitSource checked for an instance of PageSignalReceiver
both when subscribing and unsubscribing to/from it.  The
PageSignalReceiver::GetInstance() called its static IsEnabled()
method first; the latter returned false if
content::ServiceManagerConnection::GetForProcess() returned nullptr.

Normally, PageSignalReceiver is a static singleton with NoDestructor
wrapper, and TabLifecycleUnitSource is owned by the browser process, so
the above chain might result in that TabLifecycleUnitSource would not
unsubscribe because ServiceManagerConnection for process was already
destroyed.

The bug is subtle and fires sporadically in random unit tests.

R=haraken@chromium.org

Change-Id: I34827b3a598f39f38a05c8d68cb3953009454aac
Reviewed-on: https://chromium-review.googlesource.com/1180216Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Alexander Dunaev <voodoo@yandex-team.ru>
Cr-Commit-Position: refs/heads/master@{#584377}

chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc

@@ -56,6 +56,7 @@ TabLifecycleUnitSource::TabLifecycleUnitSource(
     InterventionPolicyDatabase* intervention_policy_database,
     UsageClock* usage_clock)
     : browser_tab_strip_tracker_(this, nullptr, this),
+      page_signal_receiver_observer_(this),
       intervention_policy_database_(intervention_policy_database),
       usage_clock_(usage_clock) {
   DCHECK(!instance_);
@@ -66,15 +67,11 @@ TabLifecycleUnitSource::TabLifecycleUnitSource(
   DCHECK(intervention_policy_database_);
   browser_tab_strip_tracker_.Init();
   instance_ = this;
-  // TODO(chrisha): Create a ScopedPageSignalObserver helper class to clean up
-  // this manual lifetime management.
   if (auto* page_signal_receiver = PageSignalReceiver::GetInstance())
-    page_signal_receiver->AddObserver(this);
+    page_signal_receiver_observer_.Add(page_signal_receiver);
 }
 
 TabLifecycleUnitSource::~TabLifecycleUnitSource() {
-  if (auto* page_signal_receiver = PageSignalReceiver::GetInstance())
-    page_signal_receiver->RemoveObserver(this);
   DCHECK_EQ(instance_, this);
   instance_ = nullptr;
 }

chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/observer_list.h"
+#include "base/scoped_observer.h"
 #include "chrome/browser/resource_coordinator/lifecycle_unit_source_base.h"
 #include "chrome/browser/resource_coordinator/page_signal_receiver.h"
 #include "chrome/browser/ui/browser_list_observer.h"
@@ -154,6 +155,10 @@ class TabLifecycleUnitSource : public BrowserListObserver,
   // changes.
   base::ObserverList<TabLifecycleObserver>::Unchecked tab_lifecycle_observers_;
 
+  // PageSignalReceiver is a static singleton so it outlives this object.
+  ScopedObserver<PageSignalReceiver, PageSignalObserver>
+      page_signal_receiver_observer_;
+
   // The intervention policy database used to assist freezing/discarding
   // decisions.
   InterventionPolicyDatabase* intervention_policy_database_;