[Profiles] Only remove kWaitingForBrowserWindow when a Browser is added

As pointed out by rhalavati@, the kWaitingForBrowserWindow ref was
removed when _any_ ScopedProfileKeepAlive was added. This does not match
intended use. This CL only removes kWaitingForBrowserWindow when a
kBrowserWindow ref is added.

Also, record histograms in ~ProfileManager(), to detect leaked
keepalives.

Bug: 88586
Change-Id: Iaec7d54109b7268059c8475261fef56ca1594c9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611050Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841491}

chrome/browser/profiles/profile_keep_alive_types.h

@@ -9,22 +9,29 @@
 
 // Refers to what a ScopedProfileKeepAlive's lifetime is tied to, to help
 // debugging.
+//
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+//
+// Keep this in sync with ProfileKeepAliveOrigin in enums.xml.
 enum class ProfileKeepAliveOrigin {
   // When a Profile gets created by ProfileManager, it initially has this type
   // of keep-alive. This ensures that the Profile has a refcount >=1, at least
   // until RemoveKeepAlive() gets called.
   //
   // When a kBrowserWindow keep-alive gets added, this one gets removed.
-  kWaitingForFirstBrowserWindow,
+  kWaitingForFirstBrowserWindow = 0,
 
   // This Profile has browser windows open.
-  kBrowserWindow,
+  kBrowserWindow = 1,
 
   // This Profile is running extensions with persistent background scripts.
-  kBackgroundMode,
+  kBackgroundMode = 2,
 
   // A child off-the-record profile holds a strong reference to its parent.
-  kOffTheRecordProfile,
+  kOffTheRecordProfile = 3,
+
+  kMaxValue = kOffTheRecordProfile,
 };
 
 std::ostream& operator<<(std::ostream& out,

chrome/browser/profiles/profile_manager.cc

@@ -378,6 +378,20 @@ ProfileManager::ProfileManager(const base::FilePath& user_data_dir)
 
 ProfileManager::~ProfileManager() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  if (base::FeatureList::IsEnabled(features::kDestroyProfileOnBrowserClose)) {
+    // Ideally, all the keepalives should've been cleared already. Report
+    // metrics for incorrect usage of ScopedProfileKeepAlive.
+    for (const auto& path_and_profile_info : profiles_info_) {
+      const ProfileInfo* profile_info = path_and_profile_info.second.get();
+      for (const auto& origin_and_count : profile_info->keep_alives) {
+        ProfileKeepAliveOrigin origin = origin_and_count.first;
+        int count = origin_and_count.second;
+        if (count > 0) {
+          UMA_HISTOGRAM_ENUMERATION("Profile.KeepAliveLeakAtShutdown", origin);
+        }
+      }
+    }
+  }
 }
 
 // static
@@ -1278,8 +1292,10 @@ void ProfileManager::AddKeepAlive(const Profile* profile,
 
   int& waiting_for_first_browser_window =
       info->keep_alives[ProfileKeepAliveOrigin::kWaitingForFirstBrowserWindow];
-  if (waiting_for_first_browser_window != 0)
+  if (origin == ProfileKeepAliveOrigin::kBrowserWindow &&
+      waiting_for_first_browser_window != 0) {
     waiting_for_first_browser_window = 0;
+  }
 }
 
 void ProfileManager::RemoveKeepAlive(const Profile* profile,

tools/metrics/histograms/enums.xml

@@ -61431,6 +61431,13 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
   </int>
 </enum>
 
+<enum name="ProfileKeepAliveOrigin">
+  <int value="0" label="kWaitingForFirstBrowserWindow"/>
+  <int value="1" label="kBrowserWindow"/>
+  <int value="2" label="kBackgroundMode"/>
+  <int value="3" label="kOffTheRecordProfile"/>
+</enum>
+
 <enum name="ProfileMenuActionableItem">
   <int value="0" label="Manage your Google Account button"/>
   <int value="1" label="Passwords button"/>

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

@@ -248,6 +248,17 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="Profile.KeepAliveLeakAtShutdown" enum="ProfileKeepAliveOrigin"
+    expires_after="2021-06-01">
+  <owner>nicolaso@chromium.org</owner>
+  <owner>cbe-eng@google.com</owner>
+  <summary>
+    Recorded during BrowserProcess teardown. Indicates that a Profile still has
+    ScopedProfileKeepAlive objects referencing it, of the given origin. This is
+    a sign of a bug, or incorrect usage of the ScopedProfileKeepAlive API.
+  </summary>
+</histogram>
+
 <histogram name="Profile.Menu.ClickedActionableItem"
     enum="ProfileMenuActionableItem" expires_after="2021-06-30">
   <owner>droger@chromium.org</owner>