Add crash logging to BrowserContext destructor.

Adding crash logging to the BrowserContext destructor to help diagnose
cases where a RenderProcessHost object still references a
BrowserContext that is being destroyed. These situations can potentially
lead to UAF bugs so this logging should help us detect potential issues
earlier. This change will make it easier to debug the crashes that
caused http://crrev.com/c/2305674 to get reverted without causing
user visible crashes.

This change also fixes a few tests that were accidentally keeping around
objects that kept a RenderProcessHost alive beyond the lifetime of its
BrowserContext.

Bug: 1099998
Change-Id: I45f9e2e281807b9364f560e675447a9f5d3a69c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2324408
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794668}

content/browser/browser_context.cc

@@ -464,10 +464,45 @@ BrowserContext::~BrowserContext() {
     base::debug::DumpWithoutCrashing();
   }
 
-  // Clean up any isolated origins and other security state associated with this
-  // BrowserContext.
+  // Verify that there are no outstanding RenderProcessHosts that reference
+  // this context. Trigger a crash report if there are still references so
+  // we can detect/diagnose potential UAFs.
+  std::string rph_crash_key_value;
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
+
+  for (RenderProcessHost::iterator host_iterator =
+           RenderProcessHost::AllHostsIterator();
+       !host_iterator.IsAtEnd(); host_iterator.Advance()) {
+    RenderProcessHost* host = host_iterator.GetCurrentValue();
+    if (host->GetBrowserContext() == this) {
+      rph_crash_key_value += "{";
+
+      rph_crash_key_value += " process_lock='" +
+                             policy->GetProcessLock(host->GetID()).ToString() +
+                             "'";
+
+      if (host->HostHasNotBeenUsed())
+        rph_crash_key_value += " has_not_been_used ";
+
+      if (RenderProcessHostImpl::IsSpareProcessForCrashReporting(host))
+        rph_crash_key_value += " is_spare";
+
+      rph_crash_key_value += " }";
+    }
+  }
+  if (!rph_crash_key_value.empty()) {
+    NOTREACHED() << "rph_with_bc_reference : " << rph_crash_key_value;
+
+    static auto* crash_key = base::debug::AllocateCrashKeyString(
+        "rph_with_bc_reference", base::debug::CrashKeySize::Size32);
+    base::debug::ScopedCrashKeyString auto_clear(crash_key,
+                                                 rph_crash_key_value);
+    base::debug::DumpWithoutCrashing();
+  }
+
+  // Clean up any isolated origins and other security state associated with this
+  // BrowserContext.
   policy->RemoveStateForBrowserContext(*this);
 
   if (GetUserData(kDownloadManagerKeyName))

content/browser/renderer_host/render_process_host_impl.cc

@@ -3143,6 +3143,13 @@ bool RenderProcessHostImpl::IsSpareProcessKeptAtAllTimes() {
   return true;
 }
 
+// static
+bool RenderProcessHostImpl::IsSpareProcessForCrashReporting(
+    RenderProcessHost* render_process_host) {
+  return render_process_host == SpareRenderProcessHostManager::GetInstance()
+                                    .spare_render_process_host();
+}
+
 bool RenderProcessHostImpl::HostHasNotBeenUsed() {
   return IsUnused() && listeners_.IsEmpty() && keep_alive_ref_count_ == 0 &&
          pending_views_ == 0;

content/browser/renderer_host/render_process_host_impl.h

@@ -528,6 +528,13 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // Returns true if a spare RenderProcessHost should be kept at all times.
   static bool IsSpareProcessKeptAtAllTimes();
 
+  // Helper method that allows crash reporting logic to determine if a
+  // specific RenderProcessHost is the current spare process.
+  // Returns true if |render_process_host| is the current spare
+  // RenderProcessHost.
+  static bool IsSpareProcessForCrashReporting(
+      RenderProcessHost* render_process_host);
+
   PermissionServiceContext& permission_service_context() {
     return *permission_service_context_;
   }

content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

@@ -559,8 +559,13 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
   void TearDownEnvironment() {
     sink_ = nullptr;
     process_host_ = nullptr;
-    if (view_)
+    if (view_) {
       DestroyView(view_);
+    } else if (widget_host_) {
+      // Delete |widget_host_| in cases where |view_| gets destroyed
+      // by its parent, but the host does not get destroyed.
+      delete widget_host_;
+    }
 
     parent_view_->Destroy();
     delete parent_host_;
@@ -6592,6 +6597,8 @@ TEST_P(DelegatedInkPointTest, EventForwardedToCompositor) {
   view_->InitAsChild(nullptr);
   aura_test_helper_->GetTestScreen()->SetDeviceScaleFactor(1.0f);
 
+  ui::Compositor* old_compositor =
+      view_->GetNativeView()->layer()->GetCompositor();
   MockCompositor compositor(aura_test_helper_->GetHost()->compositor());
   view_->GetNativeView()->layer()->SetCompositorForTesting(&compositor);
 
@@ -6713,15 +6720,19 @@ TEST_P(DelegatedInkPointTest, EventForwardedToCompositor) {
 
   EXPECT_FALSE(delegated_ink_point_renderer->HasDelegatedInkPoint());
 
-  widget_host_ = nullptr;
-  view_ = nullptr;
+  // Restore the view's compositor to the old value so it no longer references
+  // the MockCompositor that is about to go out of scope. This also ensures
+  // that the view can be properly destroyed by TearDownEnvironment().
+  view_->GetNativeView()->layer()->SetCompositorForTesting(old_compositor);
 }
 
 // Confirm that the interface is rebound if the receiver disconnects.
 TEST_P(DelegatedInkPointTest, MojoInterfaceReboundOnDisconnect) {
   view_->InitAsChild(nullptr);
   aura_test_helper_->GetTestScreen()->SetDeviceScaleFactor(1.0f);
 
+  ui::Compositor* old_compositor =
+      view_->GetNativeView()->layer()->GetCompositor();
   MockCompositor compositor(aura_test_helper_->GetHost()->compositor());
   view_->GetNativeView()->layer()->SetCompositorForTesting(&compositor);
 
@@ -6777,8 +6788,10 @@ TEST_P(DelegatedInkPointTest, MojoInterfaceReboundOnDisconnect) {
   EXPECT_TRUE(delegated_ink_point_renderer);
   EXPECT_TRUE(delegated_ink_point_renderer->ReceiverIsBound());
 
-  widget_host_ = nullptr;
-  view_ = nullptr;
+  // Restore the view's compositor to the old value so it no longer references
+  // the MockCompositor that is about to go out of scope. This also ensures
+  // that the view can be properly destroyed by TearDownEnvironment().
+  view_->GetNativeView()->layer()->SetCompositorForTesting(old_compositor);
 }
 
 }  // namespace content