Android: Fix a bug when closing all incognito tabs for preview tab

This CL takes care of the crash when preview tab is closed by 'close all
incognito tabs' from Android notification UI. There are 2 problems:

1) |destroy| of BottomSheetContent that preview tab is built with needs
   to invoked only by BottomSheetController when the controller finds
   it not used any more. Removes the redundant (and unnecessary) destruction
   in the preview tab coordinator.
2) An incognito profile instance shared by multiple ProfileDestroyers
   was destroyed twice when it is triggered from
   RenderProcessHostObserver::RenderProcessHostDestroyed. This CL delays
   the actual destruction task all the way to the ProfileDestroyer dtor
   (which ensures the pending destroyer list is updated properly before
   the deletion), and uses DestroyOffTheRecordProfileNow to null out
   the pending object's pointer to the profile to be deleted, which
   helps avoid UAF.

Bug: 1029677
Change-Id: I2710bb5ec2c35973da362c850434f71753175a22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1971031
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726631}

chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/ephemeraltab/EphemeralTabCoordinator.java

@@ -113,10 +113,7 @@ public class EphemeralTabCoordinator implements View.OnLayoutChangeListener {
     }
 
     private void destroyContent() {
-        if (mSheetContent != null) {
-            mSheetContent.destroy();
-            mSheetContent = null;
-        }
+        mSheetContent = null; // Will be destroyed by BottomSheet controller.
 
         if (mPanelContent != null) {
             mPanelContent.destroy();

chrome/browser/profiles/profile_destroyer.cc

@@ -113,6 +113,7 @@ void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
 void ProfileDestroyer::DestroyOffTheRecordProfileNow(Profile* const profile) {
   DCHECK(profile);
   DCHECK(profile->IsOffTheRecord());
+  DCHECK(profile->GetOriginalProfile());
   if (pending_destroyers_) {
     for (auto i = pending_destroyers_->begin(); i != pending_destroyers_->end();
          ++i) {
@@ -120,18 +121,23 @@ void ProfileDestroyer::DestroyOffTheRecordProfileNow(Profile* const profile) {
         // We want to signal this in debug builds so that we don't lose sight of
         // these potential leaks, but we handle it in release so that we don't
         // crash or corrupt profile data on disk.
-        NOTREACHED() << "A render process host wasn't destroyed early enough.";
+        LOG(WARNING) << "A render process host wasn't destroyed early enough.";
         (*i)->profile_ = NULL;
         break;
       }
     }
   }
-  DCHECK(profile->GetOriginalProfile());
-  profile->GetOriginalProfile()->DestroyOffTheRecordProfile();
+
+  if (profile->IsIndependentOffTheRecordProfile()) {
+    delete profile;
+  } else {
+    profile->GetOriginalProfile()->DestroyOffTheRecordProfile();
+  }
 }
 
 ProfileDestroyer::ProfileDestroyer(Profile* const profile, HostSet* hosts)
     : num_hosts_(0), profile_(profile) {
+  DCHECK(profile_->IsOffTheRecord());
   if (pending_destroyers_ == NULL)
     pending_destroyers_ = new DestroyerSet;
   pending_destroyers_->insert(this);
@@ -144,19 +150,17 @@ ProfileDestroyer::ProfileDestroyer(Profile* const profile, HostSet* hosts)
   // If we are going to wait for render process hosts, we don't want to do it
   // for longer than kTimerDelaySeconds.
   if (num_hosts_) {
-    timer_.Start(FROM_HERE,
-                 base::TimeDelta::FromSeconds(kTimerDelaySeconds),
-                 base::Bind(&ProfileDestroyer::DestroyProfile,
-                            weak_ptr_factory_.GetWeakPtr()));
+    timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(kTimerDelaySeconds),
+                 base::BindOnce(
+                     [](base::WeakPtr<ProfileDestroyer> ptr) {
+                       if (ptr)
+                         delete ptr.get();
+                     },
+                     weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
 ProfileDestroyer::~ProfileDestroyer() {
-  // Check again, in case other render hosts were added while we were
-  // waiting for the previous ones to go away...
-  if (profile_)
-    DestroyProfileWhenAppropriate(profile_);
-
 #ifdef NDEBUG
   // Don't wait for pending registrations, if any, these hosts are buggy.
   // Note: this can happen, but if so, it's better to crash here than wait
@@ -172,6 +176,11 @@ ProfileDestroyer::~ProfileDestroyer() {
     delete pending_destroyers_;
     pending_destroyers_ = NULL;
   }
+
+  if (profile_) {
+    ProfileDestroyer::DestroyOffTheRecordProfileNow(profile_);
+    profile_ = nullptr;
+  }
 }
 
 void ProfileDestroyer::RenderProcessHostDestroyed(
@@ -182,33 +191,15 @@ void ProfileDestroyer::RenderProcessHostDestroyed(
     // Delay the destruction one step further in case other observers need to
     // look at the profile attached to the host.
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(&ProfileDestroyer::DestroyProfile,
-                                  weak_ptr_factory_.GetWeakPtr()));
+        FROM_HERE, base::BindOnce(
+                       [](base::WeakPtr<ProfileDestroyer> ptr) {
+                         if (ptr)
+                           delete ptr.get();
+                       },
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
-void ProfileDestroyer::DestroyProfile() {
-  // We might have been cancelled externally before the timer expired.
-  if (!profile_) {
-    delete this;
-    return;
-  }
-
-  DCHECK(profile_->IsOffTheRecord());
-  DCHECK(profile_->GetOriginalProfile());
-  if (profile_->IsIndependentOffTheRecordProfile())
-    delete profile_;
-  else
-    profile_->GetOriginalProfile()->DestroyOffTheRecordProfile();
-
-  profile_ = nullptr;
-
-  // And stop the timer so we can be released early too.
-  timer_.Stop();
-
-  delete this;
-}
-
 // static
 ProfileDestroyer::HostSet ProfileDestroyer::GetHostsForProfile(
     void* const profile_ptr) {

chrome/browser/profiles/profile_destroyer.h

@@ -39,9 +39,6 @@ class ProfileDestroyer : public content::RenderProcessHostObserver {
   // content::RenderProcessHostObserver override.
   void RenderProcessHostDestroyed(content::RenderProcessHost* host) override;
 
-  // Called by the timer to cancel the pending destruction and do it now.
-  void DestroyProfile();
-
   // Fetch the list of render process hosts that still point to |profile_ptr|.
   // |profile_ptr| is a void* because the Profile object may be freed. Only
   // pointer comparison is allowed, it will never be dereferenced as a Profile.