Make LoadMonitoringExtensionHostQueue remove itself as an ExtensionHost observer at the correct tim…

extensions/browser/load_monitoring_extension_host_queue_unittest.cc

Index: extensions/browser/load_monitoring_extension_host_queue_unittest.cc
diff --git a/extensions/browser/load_monitoring_extension_host_queue_unittest.cc b/extensions/browser/load_monitoring_extension_host_queue_unittest.cc
index 1157ecda46825b09a3f0b19c3abd2eea71850201..f452e7eba1f5172f9a2bf8d9750d88d861c7167b 100644
--- a/extensions/browser/load_monitoring_extension_host_queue_unittest.cc
+++ b/extensions/browser/load_monitoring_extension_host_queue_unittest.cc
@@ -16,12 +16,25 @@ namespace extensions {
 namespace {
 
 class StubDeferredStartRenderHost : public DeferredStartRenderHost {
+ public:
+  // Returns true if this host is being observed by |observer|.
+  bool IsObservedBy(DeferredStartRenderHostObserver* observer) {
+    return observers_.count(observer) > 0;
+  }
+
  private:
+  // DeferredStartRenderHost:
   void AddDeferredStartRenderHostObserver(
-      DeferredStartRenderHostObserver* observer) override {}
+      DeferredStartRenderHostObserver* observer) override {
+    observers_.insert(observer);
+  }
   void RemoveDeferredStartRenderHostObserver(
-      DeferredStartRenderHostObserver* observer) override {}
+      DeferredStartRenderHostObserver* observer) override {
+    observers_.erase(observer);
+  }
   void CreateRenderViewNow() override {}
+
+  std::set<DeferredStartRenderHostObserver*> observers_;
 };
 
 const size_t g_invalid_size_t = std::numeric_limits<size_t>::max();
@@ -35,7 +48,7 @@ class LoadMonitoringExtensionHostQueueTest : public ExtensionsTest {
         // Arbitrary choice of an invalid size_t.
         num_queued_(g_invalid_size_t),
         num_loaded_(g_invalid_size_t),
-        max_in_queue_(g_invalid_size_t),
+        max_waiting_loading_(g_invalid_size_t),
         max_active_loading_(g_invalid_size_t) {}
 
   void SetUp() override {
@@ -50,7 +63,7 @@ class LoadMonitoringExtensionHostQueueTest : public ExtensionsTest {
  protected:
   // Creates a new DeferredStartRenderHost. Ownership is held by this class,
   // not passed to caller.
-  DeferredStartRenderHost* CreateHost() {
+  StubDeferredStartRenderHost* CreateHost() {
     StubDeferredStartRenderHost* stub = new StubDeferredStartRenderHost();
     stubs_.push_back(stub);
     return stub;
@@ -66,19 +79,19 @@ class LoadMonitoringExtensionHostQueueTest : public ExtensionsTest {
   // will return true).
   size_t num_queued() { return num_queued_; }
   size_t num_loaded() { return num_loaded_; }
-  size_t max_in_queue() { return max_in_queue_; }
+  size_t max_waiting_loading() { return max_waiting_loading_; }
   size_t max_active_loading() { return max_active_loading_; }
 
  private:
   // Callback when queue has finished monitoring.
   void Finished(size_t num_queued,
                 size_t num_loaded,
-                size_t max_in_queue,
+                size_t max_waiting_loading,
                 size_t max_active_loading) {
     finished_ = true;
     num_queued_ = num_queued;
     num_loaded_ = num_loaded;
-    max_in_queue_ = max_in_queue;
+    max_waiting_loading_ = max_waiting_loading;
     max_active_loading_ = max_active_loading;
   }
 
@@ -90,7 +103,7 @@ class LoadMonitoringExtensionHostQueueTest : public ExtensionsTest {
   bool finished_;
   size_t num_queued_;
   size_t num_loaded_;
-  size_t max_in_queue_;
+  size_t max_waiting_loading_;
   size_t max_active_loading_;
 };
 
@@ -109,7 +122,7 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, NoHosts) {
   ASSERT_TRUE(finished());
   EXPECT_EQ(0u, num_queued());
   EXPECT_EQ(0u, num_loaded());
-  EXPECT_EQ(0u, max_in_queue());
+  EXPECT_EQ(0u, max_waiting_loading());
   EXPECT_EQ(0u, max_active_loading());
 }
 
@@ -121,7 +134,7 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, AddOneHost) {
   ASSERT_TRUE(finished());
   EXPECT_EQ(1u, num_queued());
   EXPECT_EQ(0u, num_loaded());
-  EXPECT_EQ(1u, max_in_queue());
+  EXPECT_EQ(1u, max_waiting_loading());
   EXPECT_EQ(0u, max_active_loading());
 }
 
@@ -136,7 +149,7 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, AddAndRemoveOneHost) {
   ASSERT_TRUE(finished());
   EXPECT_EQ(1u, num_queued());
   EXPECT_EQ(0u, num_loaded());
-  EXPECT_EQ(1u, max_in_queue());
+  EXPECT_EQ(1u, max_waiting_loading());
   EXPECT_EQ(0u, max_active_loading());
 }
 
@@ -150,7 +163,7 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, AddAndStartOneHost) {
   ASSERT_TRUE(finished());
   EXPECT_EQ(1u, num_queued());
   EXPECT_EQ(0u, num_loaded());
-  EXPECT_EQ(1u, max_in_queue());
+  EXPECT_EQ(1u, max_waiting_loading());
   EXPECT_EQ(1u, max_active_loading());
 
   // Sanity check: stopping/destroying at this point doesn't crash.
@@ -168,7 +181,7 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, AddAndDestroyOneHost) {
   ASSERT_TRUE(finished());
   EXPECT_EQ(1u, num_queued());
   EXPECT_EQ(0u, num_loaded());
-  EXPECT_EQ(1u, max_in_queue());
+  EXPECT_EQ(1u, max_waiting_loading());
   EXPECT_EQ(0u, max_active_loading());
 
   // Sanity check: stopping/destroying at this point doesn't crash.
@@ -187,7 +200,7 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, AddAndStartAndStopOneHost) {
   ASSERT_TRUE(finished());
   EXPECT_EQ(1u, num_queued());
   EXPECT_EQ(1u, num_loaded());
-  EXPECT_EQ(1u, max_in_queue());
+  EXPECT_EQ(1u, max_waiting_loading());
   EXPECT_EQ(1u, max_active_loading());
 
   // Sanity check: destroying at this point doesn't crash.
@@ -205,7 +218,7 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, AddAndStartAndDestroyOneHost) {
   ASSERT_TRUE(finished());
   EXPECT_EQ(1u, num_queued());
   EXPECT_EQ(1u, num_loaded());
-  EXPECT_EQ(1u, max_in_queue());
+  EXPECT_EQ(1u, max_waiting_loading());
   EXPECT_EQ(1u, max_active_loading());
 }
 
@@ -220,7 +233,7 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, AddAndStartAndRemoveOneHost) {
   ASSERT_TRUE(finished());
   EXPECT_EQ(1u, num_queued());
   EXPECT_EQ(0u, num_loaded());
-  EXPECT_EQ(1u, max_in_queue());
+  EXPECT_EQ(1u, max_waiting_loading());
   EXPECT_EQ(1u, max_active_loading());
 }
 
@@ -258,7 +271,7 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, Sequence) {
   ASSERT_TRUE(finished());
   EXPECT_EQ(6u, num_queued());
   EXPECT_EQ(2u, num_loaded());
-  EXPECT_EQ(4u, max_in_queue());
+  EXPECT_EQ(4u, max_waiting_loading());
   EXPECT_EQ(3u, max_active_loading());
 
   // Sanity check: complete a realistic sequence by stopping and/or destroying
@@ -273,4 +286,58 @@ TEST_F(LoadMonitoringExtensionHostQueueTest, Sequence) {
   queue()->OnDeferredStartRenderHostDestroyed(host6);  // never started/stopped
 }
 
+// Tests that the queue is observing Hosts from adding them through to being
+// removed - that the load sequence itself is irrelevant.
+//
+// This is an unfortunate implementation-style test, but it used to be a bug
+// and difficult to catch outside of a proper test framework - one in which we
+// don't have to trigger events by hand.
+TEST_F(LoadMonitoringExtensionHostQueueTest, ObserverLifetime) {
+  StubDeferredStartRenderHost* host1 = CreateHost();
+  StubDeferredStartRenderHost* host2 = CreateHost();
+  StubDeferredStartRenderHost* host3 = CreateHost();
+  StubDeferredStartRenderHost* host4 = CreateHost();
+
+  EXPECT_FALSE(host1->IsObservedBy(queue()));
+  EXPECT_FALSE(host2->IsObservedBy(queue()));
+  EXPECT_FALSE(host3->IsObservedBy(queue()));
+  EXPECT_FALSE(host4->IsObservedBy(queue()));
+
+  queue()->Add(host1);
+  queue()->Add(host2);
+  queue()->Add(host3);
+  queue()->Add(host4);
+
+  EXPECT_TRUE(host1->IsObservedBy(queue()));
+  EXPECT_TRUE(host2->IsObservedBy(queue()));
+  EXPECT_TRUE(host3->IsObservedBy(queue()));
+  EXPECT_TRUE(host4->IsObservedBy(queue()));
+
+  queue()->OnDeferredStartRenderHostDidStartLoading(host1);
+  queue()->OnDeferredStartRenderHostDidStartLoading(host2);
+  queue()->OnDeferredStartRenderHostDidStartLoading(host3);
+  // host4 will test that we Remove before Starting - so don't start.
+
+  EXPECT_TRUE(host1->IsObservedBy(queue()));
+  EXPECT_TRUE(host2->IsObservedBy(queue()));
+  EXPECT_TRUE(host3->IsObservedBy(queue()));
+  EXPECT_TRUE(host4->IsObservedBy(queue()));
+
+  queue()->OnDeferredStartRenderHostDidStopLoading(host1);
+  queue()->OnDeferredStartRenderHostDestroyed(host2);
+
+  EXPECT_TRUE(host1->IsObservedBy(queue()));
+  EXPECT_TRUE(host2->IsObservedBy(queue()));
+
+  queue()->Remove(host1);
+  queue()->Remove(host2);
+  queue()->Remove(host3);
+  queue()->Remove(host4);
+
+  EXPECT_FALSE(host1->IsObservedBy(queue()));
+  EXPECT_FALSE(host2->IsObservedBy(queue()));
+  EXPECT_FALSE(host3->IsObservedBy(queue()));
+  EXPECT_FALSE(host4->IsObservedBy(queue()));
+}
+
 }  // namespace extensions