Fix use-after-free in PageSignalReceiver::NotifyObserversIfKnownCu.

The cu_id_web_contents_map_ can be mutated by observers and can make
the iterator in NotifyObserversIfKnownCu invalid.

See the comment of the regression test for the description of the
scenario that can happen with tab discarding.

Change-Id: I90a7f1710655573fa059bf3f12bf40013ebc7743
Reviewed-on: https://chromium-review.googlesource.com/1107798
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569205}

chrome/browser/resource_coordinator/page_signal_receiver.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_RESOURCE_COORDINATOR_PAGE_SIGNAL_RECEIVER_H_
 #define CHROME_BROWSER_RESOURCE_COORDINATOR_PAGE_SIGNAL_RECEIVER_H_
 
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "mojo/public/cpp/bindings/binding.h"
@@ -83,6 +84,8 @@ class PageSignalReceiver : public mojom::PageSignalReceiver {
   void RemoveCoordinationUnitID(const CoordinationUnitID& cu_id);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(PageSignalReceiverUnitTest,
+                           NotifyObserversThatCanRemoveCoordinationUnitID);
   template <typename Method, typename... Params>
   void NotifyObserversIfKnownCu(const CoordinationUnitID& page_cu_id,
                                 Method m,
@@ -90,8 +93,11 @@ class PageSignalReceiver : public mojom::PageSignalReceiver {
     auto web_contents_iter = cu_id_web_contents_map_.find(page_cu_id);
     if (web_contents_iter == cu_id_web_contents_map_.end())
       return;
+    // An observer can make web_contents_iter invalid by mutating
+    // the cu_id_web_contents_map_.
+    content::WebContents* web_contents = web_contents_iter->second;
     for (auto& observer : observers_)
-      (observer.*m)(web_contents_iter->second, std::forward<Params>(params)...);
+      (observer.*m)(web_contents, std::forward<Params>(params)...);
   }
 
   mojo::Binding<mojom::PageSignalReceiver> binding_;

chrome/browser/resource_coordinator/page_signal_receiver_unittest.cc

+// Copyright 2018 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/resource_coordinator/page_signal_receiver.h"
+
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "content/public/test/web_contents_tester.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace resource_coordinator {
+
+class PageSignalReceiverUnitTest : public ChromeRenderViewHostTestHarness {
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+    page_cu_id_ = {CoordinationUnitType::kPage, CoordinationUnitID::RANDOM_ID};
+    page_signal_receiver_ = std::make_unique<PageSignalReceiver>();
+  }
+
+  void TearDown() override {
+    page_signal_receiver_.reset();
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
+ protected:
+  CoordinationUnitID page_cu_id_;
+  std::unique_ptr<PageSignalReceiver> page_signal_receiver_;
+};
+
+enum class Action { kObserve, kRemoveCoordinationUnitID };
+class TestPageSignalObserver : public PageSignalObserver {
+ public:
+  TestPageSignalObserver(Action action,
+                         CoordinationUnitID page_cu_id,
+                         PageSignalReceiver* page_signal_receiver)
+      : action_(action),
+        page_cu_id_(page_cu_id),
+        page_signal_receiver_(page_signal_receiver) {
+    page_signal_receiver_->AddObserver(this);
+  }
+  ~TestPageSignalObserver() override {
+    page_signal_receiver_->RemoveObserver(this);
+  }
+  // PageSignalObserver:
+  void OnLifecycleStateChanged(content::WebContents* contents,
+                               mojom::LifecycleState state) override {
+    if (action_ == Action::kRemoveCoordinationUnitID)
+      page_signal_receiver_->RemoveCoordinationUnitID(page_cu_id_);
+  }
+
+ private:
+  Action action_;
+  CoordinationUnitID page_cu_id_;
+  PageSignalReceiver* page_signal_receiver_;
+  DISALLOW_COPY_AND_ASSIGN(TestPageSignalObserver);
+};
+
+// This test models the scenario that can happen with tab discarding.
+// 1) Multiple observers are subscribed to the page signal receiver.
+// 2) The page signal receiver sends OnLifecycleStateChanged to observers.
+// 3) The first observer is TabLifecycleUnitSource, which calls
+//    TabLifecycleUnit::FinishDiscard that destroys the old WebContents.
+// 4) The destructor of the WebContents calls
+//    ResourceCoordinatorTabHelper::WebContentsDestroyed, which deletes the
+//    page_cu_id entry from the page signal receiver map.
+// 5) The next observer is invoked with the deleted entry.
+TEST_F(PageSignalReceiverUnitTest,
+       NotifyObserversThatCanRemoveCoordinationUnitID) {
+  page_signal_receiver_->AssociateCoordinationUnitIDWithWebContents(
+      page_cu_id_, web_contents());
+  TestPageSignalObserver observer1(Action::kObserve, page_cu_id_,
+                                   page_signal_receiver_.get());
+  TestPageSignalObserver observer2(Action::kRemoveCoordinationUnitID,
+                                   page_cu_id_, page_signal_receiver_.get());
+  TestPageSignalObserver observer3(Action::kObserve, page_cu_id_,
+                                   page_signal_receiver_.get());
+  page_signal_receiver_->NotifyObserversIfKnownCu(
+      page_cu_id_, &PageSignalObserver::OnLifecycleStateChanged,
+      mojom::LifecycleState::kDiscarded);
+}
+
+}  // namespace resource_coordinator

chrome/test/BUILD.gn

@@ -2962,6 +2962,7 @@ test("unit_tests") {
       "../browser/resource_coordinator/local_site_characteristics_data_writer_unittest.cc",
       "../browser/resource_coordinator/local_site_characteristics_non_recording_data_store_unittest.cc",
       "../browser/resource_coordinator/local_site_characteristics_webcontents_observer_unittest.cc",
+      "../browser/resource_coordinator/page_signal_receiver_unittest.cc",
       "../browser/resource_coordinator/performance_measurement_manager_unittest.cc",
       "../browser/resource_coordinator/session_restore_policy_unittest.cc",
       "../browser/resource_coordinator/tab_activity_watcher_unittest.cc",