[PM] Fix UAF in TabLoadingFrameNavigationScheduler.

Resuming one throttle can cause arbitrarily many other throttles to be
invalidated, which would manifest as a UAF with the incorrect throttle
iteration logic currently in use. Moreover, the code assumes a throttle
that is vended gets attached to a NavigationHandle and eventually closed
only when DidFinishNavigation() is invoked; this fix plugs that hole by
explicit lifetime tracking of throttles.

BUG=1098226

Change-Id: Ic848db05c398526d993ed3725d4fdfd849341f2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2270957
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#784075}

components/performance_manager/BUILD.gn

@@ -204,6 +204,7 @@ source_set("unit_tests") {
     "graph/properties_unittest.cc",
     "graph/system_node_impl_unittest.cc",
     "graph/worker_node_impl_unittest.cc",
+    "mechanisms/tab_loading_frame_navigation_scheduler_unittest.cc",
     "owned_objects_unittest.cc",
     "performance_manager_impl_unittest.cc",
     "performance_manager_registry_impl_unittest.cc",

components/performance_manager/mechanisms/tab_loading_frame_navigation_scheduler.cc

@@ -88,13 +88,21 @@ class MainThreadMechanism : public PerformanceManagerMainThreadMechanism {
 
 }  // namespace
 
-// A very simple throttle that always defers until Resume is called.
+// A very simple throttle that always defers until Resume is called. The
+// scheduler that issued the throttle will always outlive all of the throttles
+// themselves, or explicitly detach from the throttle.
 class TabLoadingFrameNavigationScheduler::Throttle
     : public content::NavigationThrottle {
  public:
-  explicit Throttle(content::NavigationHandle* handle)
-      : content::NavigationThrottle(handle) {}
-  ~Throttle() override = default;
+  Throttle(content::NavigationHandle* handle,
+           TabLoadingFrameNavigationScheduler* scheduler)
+      : content::NavigationThrottle(handle), scheduler_(scheduler) {
+    DCHECK(scheduler);
+  }
+  ~Throttle() override {
+    if (scheduler_)
+      scheduler_->NotifyThrottleDestroyed(this);
+  }
 
   // content::NavigationThrottle implementation
   const char* GetNameForLogging() override {
@@ -106,8 +114,20 @@ class TabLoadingFrameNavigationScheduler::Throttle
     return content::NavigationThrottle::DEFER;
   }
 
-  // Make this public so the scheduler can invoke it.
+  // Make this public so the scheduler can invoke it. Care must be taken in
+  // calling this as it can synchronously destroy this Throttle and others!
   using content::NavigationThrottle::Resume;
+
+  // Detaches this Throttle from its scheduler, so it doesn't callback into it
+  // when it is destroyed.
+  void DetachFromScheduler() {
+    // This should only be called once.
+    DCHECK(scheduler_);
+    scheduler_ = nullptr;
+  }
+
+ private:
+  TabLoadingFrameNavigationScheduler* scheduler_ = nullptr;
 };
 
 WEB_CONTENTS_USER_DATA_KEY_IMPL(TabLoadingFrameNavigationScheduler)
@@ -187,7 +207,7 @@ TabLoadingFrameNavigationScheduler::MaybeCreateThrottleForNavigation(
 
   // Getting here indicates that the navigation is to be throttled. Create a
   // throttle and remember it.
-  std::unique_ptr<Throttle> throttle(new Throttle(handle));
+  std::unique_ptr<Throttle> throttle(new Throttle(handle, scheduler));
   auto result =
       scheduler->throttles_.insert(std::make_pair(handle, throttle.get()));
   DCHECK(result.second);
@@ -267,23 +287,29 @@ TabLoadingFrameNavigationScheduler::TabLoadingFrameNavigationScheduler(
 
 void TabLoadingFrameNavigationScheduler::DidFinishNavigation(
     content::NavigationHandle* handle) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  // If we're throttling a canceled navigation then stop tracking it. The
-  // |handle| becomes invalid shortly after this function returns.
-  throttles_.erase(handle);
+  DCHECK(handle);
+  RemoveThrottleForHandle(handle);
 }
 
 void TabLoadingFrameNavigationScheduler::StopThrottlingImpl() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Release all of the throttles. Note that releasing a throttle will cause
-  // "DidFinishNavigation" to be invoked for the associated NavigationHandle,
-  // which would modify |throttles_|. We instead take the data structure before
-  // iterating.
-  auto throttles = std::move(throttles_);
-  DCHECK(throttles_.empty());
-  for (auto& entry : throttles) {
-    auto* throttle = entry.second;
+  // Release all of the throttles. Note that releasing a throttle may cause
+  // that throttle and others to immediately be invalidated (we'll learn about
+  // it via DidFinishNavigation() or NavigationThrottleDestroyed()).
+  while (!throttles_.empty()) {
+    // Remove the first throttle.
+    auto it = throttles_.end() - 1;
+    auto* throttle = it->second;
+    throttles_.erase(it);
+
+    // We've already erased this throttle and don't need it to notify us via
+    // NotifyThrottleDestroyed.
+    throttle->DetachFromScheduler();
+
+    // Resume the throttle.
+    if (resume_callback_)
+      resume_callback_.Run(throttle);
     throttle->Resume();
   }
 
@@ -293,5 +319,26 @@ void TabLoadingFrameNavigationScheduler::StopThrottlingImpl() {
   web_contents()->RemoveUserData(UserDataKey());
 }
 
+void TabLoadingFrameNavigationScheduler::NotifyThrottleDestroyed(
+    Throttle* throttle) {
+  DCHECK(throttle);
+  DCHECK(throttle->navigation_handle());
+  RemoveThrottleForHandle(throttle->navigation_handle());
+}
+
+void TabLoadingFrameNavigationScheduler::RemoveThrottleForHandle(
+    content::NavigationHandle* handle) {
+  DCHECK(handle);
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  auto it = throttles_.find(handle);
+  if (it == throttles_.end())
+    return;
+  // If we're throttling a canceled navigation then stop tracking it. The
+  // |handle| becomes invalid shortly after this function returns. Explicitly
+  // detach from the throttle so we don't get multiple callbacks from it.
+  it->second->DetachFromScheduler();
+  throttles_.erase(it);
+}
+
 }  // namespace mechanisms
 }  // namespace performance_manager

components/performance_manager/mechanisms/tab_loading_frame_navigation_scheduler_unittest.cc

@@ -0,0 +1,187 @@
+// Copyright 2020 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 "components/performance_manager/public/mechanisms/tab_loading_frame_navigation_scheduler.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/test/bind_test_util.h"
+#include "components/performance_manager/test_support/performance_manager_test_harness.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/navigation_throttle.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/test/mock_navigation_handle.h"
+#include "content/public/test/navigation_simulator.h"
+#include "content/public/test/test_renderer_host.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace performance_manager {
+namespace mechanisms {
+
+namespace {
+
+// A simply policy delegate that always says yes to throttling everything.
+class DummyPolicyDelegate
+    : public TabLoadingFrameNavigationScheduler::PolicyDelegate {
+ public:
+  DummyPolicyDelegate() = default;
+  DummyPolicyDelegate(const DummyPolicyDelegate&) = delete;
+  DummyPolicyDelegate& operator=(const DummyPolicyDelegate&) = delete;
+  ~DummyPolicyDelegate() override = default;
+
+  // PolicyDelegate implementation:
+  bool ShouldThrottleWebContents(content::WebContents* contents) override {
+    return true;
+  }
+  bool ShouldThrottleNavigation(content::NavigationHandle* handle) override {
+    return true;
+  }
+};
+
+class TabLoadingFrameNavigationSchedulerTest
+    : public PerformanceManagerTestHarness {
+ public:
+  using Super = PerformanceManagerTestHarness;
+
+  TabLoadingFrameNavigationSchedulerTest() = default;
+  TabLoadingFrameNavigationSchedulerTest(
+      const TabLoadingFrameNavigationSchedulerTest&) = delete;
+  TabLoadingFrameNavigationSchedulerTest& operator=(
+      const TabLoadingFrameNavigationSchedulerTest&) = delete;
+  ~TabLoadingFrameNavigationSchedulerTest() override = default;
+
+  void SetUp() override {
+    Super::SetUp();
+    TabLoadingFrameNavigationScheduler::SetPolicyDelegateForTesting(
+        &policy_delegate_);
+  }
+
+  void TearDown() override {
+    // Clear the delegate (a static singleton) so we don't affect other tests.
+    TabLoadingFrameNavigationScheduler::SetPolicyDelegateForTesting(nullptr);
+    Super::TearDown();
+  }
+
+ private:
+  DummyPolicyDelegate policy_delegate_;
+};
+
+// A simple WebContentsObserver that attaches a throttle to a navigation
+// when the navigation starts.
+class AttachThrottleHelper : public content::WebContentsObserver {
+ public:
+  explicit AttachThrottleHelper(content::WebContents* contents)
+      : content::WebContentsObserver(contents) {}
+
+  AttachThrottleHelper(const AttachThrottleHelper&) = delete;
+  AttachThrottleHelper& operator=(const AttachThrottleHelper&) = delete;
+
+  ~AttachThrottleHelper() override = default;
+
+ private:
+  // WebContentsObserver implementation:
+  void DidStartNavigation(content::NavigationHandle* handle) override {
+    std::unique_ptr<content::NavigationThrottle> throttle =
+        TabLoadingFrameNavigationScheduler::MaybeCreateThrottleForNavigation(
+            handle);
+    if (throttle)
+      handle->RegisterThrottleForTesting(std::move(throttle));
+  }
+};
+
+}  // namespace
+
+TEST_F(TabLoadingFrameNavigationSchedulerTest, MultipleThrottlesClosed) {
+  TabLoadingFrameNavigationScheduler::SetThrottlingEnabled(true);
+
+  // A helper class for attaching NavigationThrottles to the simulated
+  // navigations.
+  SetContents(CreateTestWebContents());
+  AttachThrottleHelper attach_throttle_helper(web_contents());
+
+  // Start a main frame navigation.
+  content::NavigationSimulator::NavigateAndCommitFromBrowser(
+      web_contents(), GURL("https://www.foo.com/"));
+  content::RenderFrameHost* main_frame = web_contents()->GetMainFrame();
+
+  // A scheduler should now exist, but there should be no throttles.
+  auto* scheduler =
+      TabLoadingFrameNavigationScheduler::FromWebContents(web_contents());
+  ASSERT_TRUE(scheduler);
+  EXPECT_EQ(0u, scheduler->GetThrottleCountForTesting());
+
+  // Create a child frame that we will navigate.
+  auto* child1 =
+      content::RenderFrameHostTester::For(main_frame)->AppendChild("child1");
+
+  // Pretend to navigate the child frame with a MockNavigation, and issue a
+  // throttle for it. We manually delete the throttle before transferring
+  // ownership to a NavigationHandle, testing the throttle deletion notification
+  // codepath.
+  const GURL bar_url("https://www.bar.com/");
+  {
+    content::MockNavigationHandle handle(bar_url, child1);
+    std::unique_ptr<content::NavigationThrottle> throttle =
+        scheduler->MaybeCreateThrottleForNavigation(&handle);
+    EXPECT_TRUE(throttle.get());
+    EXPECT_EQ(1u, scheduler->GetThrottleCountForTesting());
+    throttle.reset();
+    EXPECT_EQ(0u, scheduler->GetThrottleCountForTesting());
+  }
+
+  // Navigate the child frame again. This time we'll use a navigation simulator
+  // and actually attach the throttle to the handle.
+  std::unique_ptr<content::NavigationSimulator> simulator1(
+      content::NavigationSimulator::CreateRendererInitiated(bar_url, child1));
+  simulator1->SetAutoAdvance(false);
+  simulator1->Start();
+  auto* handle1 = simulator1->GetNavigationHandle();
+  EXPECT_EQ(1u, scheduler->GetThrottleCountForTesting());
+  EXPECT_TRUE(simulator1->IsDeferred());
+
+  // Create another frame from a distinct domain. It should be throttled.
+  auto* child2 =
+      content::RenderFrameHostTester::For(main_frame)->AppendChild("child2");
+  std::unique_ptr<content::NavigationSimulator> simulator2(
+      content::NavigationSimulator::CreateRendererInitiated(
+          GURL("https://www.baz.com/"), child2));
+  simulator2->SetAutoAdvance(false);
+  simulator2->Start();
+  auto* handle2 = simulator2->GetNavigationHandle();
+  EXPECT_EQ(2u, scheduler->GetThrottleCountForTesting());
+  EXPECT_TRUE(simulator2->IsDeferred());
+
+  // Set a "Resume" callback. When resume is called for one of the throttles,
+  // reset the other. Don't expect another resume callback for the second
+  // throttle, as it should be synchronously cleaned up in the scope of the
+  // first resume.
+  size_t resume_callback_count = 0;
+  scheduler->SetResumeCallbackForTesting(base::BindLambdaForTesting(
+      [&simulator1, &simulator2, handle1, handle2,
+       &resume_callback_count](content::NavigationThrottle* throttle) {
+        ++resume_callback_count;
+        // Calling AbortFromRenderer will cause the throttle to be destroyed via
+        // a DidFinishNavigation notification.
+        auto* handle = throttle->navigation_handle();
+        if (handle == handle1) {
+          simulator2->AbortFromRenderer();
+        } else if (handle == handle2) {
+          simulator1->AbortFromRenderer();
+        }
+      }));
+
+  // Disable throttling, and expect this to cause the scheduler to tear down.
+  // The resume callback should only be invoked for one of the throttles, and
+  // the scheduler should be torn down.
+  TabLoadingFrameNavigationScheduler::SetThrottlingEnabled(false);
+  DCHECK_EQ(1u, resume_callback_count);
+  EXPECT_FALSE(
+      TabLoadingFrameNavigationScheduler::FromWebContents(web_contents()));
+}
+
+}  // namespace mechanisms
+}  // namespace performance_manager

components/performance_manager/public/mechanisms/tab_loading_frame_navigation_scheduler.h

@@ -37,6 +37,9 @@ class TabLoadingFrameNavigationScheduler
  public:
   class PolicyDelegate;
 
+  using ResumeCallback =
+      base::RepeatingCallback<void(content::NavigationThrottle*)>;
+
   ~TabLoadingFrameNavigationScheduler() override;
 
   // Invoked by the embedder hooks. Depending on policy decisions this may end
@@ -59,18 +62,23 @@ class TabLoadingFrameNavigationScheduler
                              int64_t last_navigation_id);
 
   // Testing seams.
+  static TabLoadingFrameNavigationScheduler* GetRootForTesting();
   static void SetPolicyDelegateForTesting(PolicyDelegate* policy_delegate);
   static bool IsThrottlingEnabledForTesting();
   static bool IsMechanismRegisteredForTesting();
   void StopThrottlingForTesting() { StopThrottlingImpl(); }
   size_t GetThrottleCountForTesting() const { return throttles_.size(); }
   int64_t GetNavigationIdForTesting() const { return navigation_id_; }
+  void SetResumeCallbackForTesting(ResumeCallback resume_callback) {
+    resume_callback_ = resume_callback;
+  }
 
  private:
   friend class content::WebContentsUserData<TabLoadingFrameNavigationScheduler>;
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 
   class Throttle;
+  using ThrottleMap = base::flat_map<content::NavigationHandle*, Throttle*>;
 
   explicit TabLoadingFrameNavigationScheduler(content::WebContents* contents);
 
@@ -83,13 +91,24 @@ class TabLoadingFrameNavigationScheduler
   // be taken in using this.
   void StopThrottlingImpl();
 
+  // Called by instances of Throttle as they tear down. Allows this class to
+  // maintain the list of throttles it has issued, and observe them as they are
+  // destroyed.
+  void NotifyThrottleDestroyed(Throttle* throttle);
+
+  // Called by both "NotifyThrottleDestroyed" and "DidFinishNavigation".
+  void RemoveThrottleForHandle(content::NavigationHandle* handle);
+
   // The navigation ID that this scheduler applies to. Set immediately after
   // object creation.
   int64_t navigation_id_ = 0;
 
   // The set of Throttles that have been created by this object, and the
   // navigation handles to which they are associated.
-  base::flat_map<content::NavigationHandle*, Throttle*> throttles_;
+  ThrottleMap throttles_;
+
+  // Used as a testing seam.
+  ResumeCallback resume_callback_;
 
   // Linked list mechanism for the collection of all mechanism instances. This
   // is used to implement StopThrottlingEverything.