Add metrics for OffTheRecord profile destruction.

Metrics are added to measure different code paths in off-the-record
profile destruction, specifically when destruction is delayed due to
delayed RPHs.

Bug: 1033903
Change-Id: Ibc8c82c3f8c4d569c34e57aad1b508eff9eb32c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2440625Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814111}

chrome/browser/profiles/profile.h

@@ -33,6 +33,7 @@ class ChromeZoomLevelPrefs;
 class ExtensionSpecialStoragePolicy;
 class PrefService;
 class PrefStore;
+class ProfileDestroyer;
 class ProfileKey;
 class TestingProfile;
 
@@ -146,6 +147,7 @@ class Profile : public content::BrowserContext {
 #endif
 
    private:
+    friend class ProfileDestroyer;
     friend std::ostream& operator<<(std::ostream& out,
                                     const OTRProfileID& profile_id);
 

chrome/browser/profiles/profile_destroyer.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
@@ -25,6 +26,15 @@ const int64_t kTimerDelaySeconds = 5;
 const int64_t kTimerDelaySeconds = 1;
 #endif
 
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+enum class ProfileDestructionType {
+  kImmediately = 0,
+  kDelayed = 1,
+  kDelayedAndCrashed = 2,
+  kMaxValue = kDelayedAndCrashed,
+};
+
 }  // namespace
 
 ProfileDestroyer::DestroyerSet* ProfileDestroyer::pending_destroyers_ = nullptr;
@@ -38,8 +48,6 @@ void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
   DCHECK(profile);
   profile->MaybeSendDestroyedNotification();
 
-  // TODO(https://crbug.com/1033903): If regular profile has OTRs and they have
-  // hosts, create a |ProfileDestroyer| instead.
   if (!profile->IsOffTheRecord()) {
     DestroyRegularProfileNow(profile);
     return;
@@ -63,8 +71,9 @@ void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
 void ProfileDestroyer::DestroyOffTheRecordProfileNow(Profile* const profile) {
   DCHECK(profile);
   DCHECK(profile->IsOffTheRecord());
-  TRACE_EVENT1("shutdown", "ProfileDestroyer::DestroyOffTheRecordProfileNow",
-               "profile", profile);
+  TRACE_EVENT2("shutdown", "ProfileDestroyer::DestroyOffTheRecordProfileNow",
+               "profile", profile, "OTRProfileID",
+               profile->GetOTRProfileID().ToString());
   if (ResetPendingDestroyers(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
@@ -73,6 +82,8 @@ void ProfileDestroyer::DestroyOffTheRecordProfileNow(Profile* const profile) {
   }
   DCHECK(profile->GetOriginalProfile());
   profile->GetOriginalProfile()->DestroyOffTheRecordProfile(profile);
+  UMA_HISTOGRAM_ENUMERATION("Profile.Destroyer.OffTheRecord",
+                            ProfileDestructionType::kImmediately);
 }
 
 // static
@@ -157,8 +168,8 @@ ProfileDestroyer::ProfileDestroyer(Profile* const profile, HostSet* hosts)
 }
 
 ProfileDestroyer::~ProfileDestroyer() {
-  TRACE_EVENT1("shutdown", "ProfileDestroyer::~ProfileDestroyer", "profile",
-               profile_);
+  TRACE_EVENT2("shutdown", "ProfileDestroyer::~ProfileDestroyer", "profile",
+               profile_, "remaining_hosts", num_hosts_);
 
   // Check again, in case other render hosts were added while we were
   // waiting for the previous ones to go away...
@@ -168,6 +179,10 @@ ProfileDestroyer::~ProfileDestroyer() {
   // 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
   // for the host to dereference a deleted Profile. http://crbug.com/248625
+  UMA_HISTOGRAM_ENUMERATION("Profile.Destroyer.OffTheRecord",
+                            num_hosts_
+                                ? ProfileDestructionType::kDelayedAndCrashed
+                                : ProfileDestructionType::kDelayed);
   CHECK_EQ(0U, num_hosts_) << "Some render process hosts were not "
                            << "destroyed early enough!";
   DCHECK(pending_destroyers_ != NULL);

tools/metrics/histograms/enums.xml

@@ -59737,6 +59737,12 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
   </int>
 </enum>
 
+<enum name="ProfileDestructionType">
+  <int value="0" label="Immediately"/>
+  <int value="1" label="Delayed"/>
+  <int value="2" label="Delayed and Crashed"/>
+</enum>
+
 <enum name="ProfileErrorType">
   <int value="0" label="History error"/>
   <int value="1" label="Preferences error"/>

tools/metrics/histograms/histograms_xml/profile/histograms.xml

@@ -152,6 +152,17 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="Profile.Destroyer.OffTheRecord" enum="ProfileDestructionType"
+    expires_after="M95">
+  <owner>rhalavati@chromium.org</owner>
+  <owner>chrome-privacy-core@google.com</owner>
+  <summary>
+    Track if the off-the-record profile was immidiately destroyed when asked to
+    be destroyed, or was scheduled for a delayed destruction, and possibly was
+    not destroyed properly after the delay and was crashed.
+  </summary>
+</histogram>
+
 <histogram name="Profile.DiceUI.GaiaAccountsStale" enum="BooleanStale"
     expires_after="M80">
   <owner>msarda@chromium.org</owner>