Fix shutdown histograms on browser closing.

Shutdown histograms were not properly written because of several issues:
1) There is an incorrect calculation of current open browsers which
have not yet started to close. This leads to the fact that
browser_shutdown::OnShutdownStarting() is not called in case of
several browsers shutdown.
2) During the closure of the browser we are trying to find if there is any
background Chrome applications with help of KeepAliveRegistry::IsKeepingAlive()
function call, which is actually tracking running Browsers also (i.e. it can
return true even if there is no background application). So it is needed to
introduce new function to track background applications only.

R=sky@chromium.org
BUG=707147
BUG=707144

Change-Id: If4f7c080965c95e2c0b810817e94a09f3a52ba51
Reviewed-on: https://chromium-review.googlesource.com/760356
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Nicolas Dossou-Gbété <dgn@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519810}

chrome/browser/browser_shutdown_browsertest.cc

+// Copyright 2017 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 "base/test/histogram_tester.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/browser_shutdown.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/notification_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::AtLeast;
+
+class BrowserShutdownBrowserTest : public InProcessBrowserTest {
+ public:
+  BrowserShutdownBrowserTest() {}
+  ~BrowserShutdownBrowserTest() override {}
+
+ protected:
+  base::HistogramTester histogram_tester_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BrowserShutdownBrowserTest);
+};
+
+class BrowserClosingObserver : public chrome::BrowserListObserver {
+ public:
+  BrowserClosingObserver() {}
+  MOCK_METHOD1(OnBrowserClosing, void(Browser* browser));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BrowserClosingObserver);
+};
+
+// ChromeOS has the different shutdown flow on user initiated exit process.
+// See the comment for chrome::AttemptUserExit() function declaration.
+#if !defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_F(BrowserShutdownBrowserTest,
+                       PRE_TwoBrowsersClosingShutdownHistograms) {
+  ui_test_utils::NavigateToURL(browser(), GURL("browser://version"));
+  Browser* browser2 = CreateBrowser(browser()->profile());
+  ui_test_utils::NavigateToURL(browser2, GURL("browser://help"));
+
+  BrowserClosingObserver closing_observer;
+  BrowserList::AddObserver(&closing_observer);
+  EXPECT_CALL(closing_observer, OnBrowserClosing(_)).Times(AtLeast(1));
+
+  content::WindowedNotificationObserver terminate_observer(
+      chrome::NOTIFICATION_APP_TERMINATING,
+      content::NotificationService::AllSources());
+  chrome::ExecuteCommand(browser(), IDC_EXIT);
+  terminate_observer.Wait();
+
+  EXPECT_TRUE(browser_shutdown::IsTryingToQuit());
+  EXPECT_TRUE(BrowserList::GetInstance()->empty());
+  EXPECT_EQ(browser_shutdown::GetShutdownType(),
+            browser_shutdown::WINDOW_CLOSE);
+  BrowserList::RemoveObserver(&closing_observer);
+}
+
+IN_PROC_BROWSER_TEST_F(BrowserShutdownBrowserTest,
+                       TwoBrowsersClosingShutdownHistograms) {
+  histogram_tester_.ExpectUniqueSample("Shutdown.ShutdownType",
+                                       browser_shutdown::WINDOW_CLOSE, 1);
+  histogram_tester_.ExpectTotalCount("Shutdown.renderers.total", 1);
+  histogram_tester_.ExpectTotalCount("Shutdown.window_close.time2", 1);
+  histogram_tester_.ExpectTotalCount("Shutdown.window_close.time_per_process",
+                                     1);
+}
+#endif  // !defined(OS_CHROMEOS)

chrome/browser/ui/browser.cc

@@ -735,7 +735,7 @@ void Browser::OnWindowClosing() {
   // pages).
   bool should_quit_if_last_browser =
       browser_shutdown::IsTryingToQuit() ||
-      !KeepAliveRegistry::GetInstance()->IsKeepingAlive();
+      KeepAliveRegistry::GetInstance()->IsKeepingAliveOnlyByBrowserOrigin();
 
   if (should_quit_if_last_browser && ShouldStartShutdown())
     browser_shutdown::OnShutdownStarting(browser_shutdown::WINDOW_CLOSE);
@@ -2464,8 +2464,20 @@ bool Browser::ShouldHideUIForFullscreen() const {
   return window_ && window_->ShouldHideUIForFullscreen();
 }
 
+bool Browser::IsBrowserClosing() const {
+  const BrowserList::BrowserSet& closing_browsers =
+      BrowserList::GetInstance()->currently_closing_browsers();
+
+  return base::ContainsKey(closing_browsers, this);
+}
+
 bool Browser::ShouldStartShutdown() const {
-  return BrowserList::GetInstance()->size() <= 1;
+  if (IsBrowserClosing())
+    return false;
+
+  const size_t closing_browsers_count =
+      BrowserList::GetInstance()->currently_closing_browsers().size();
+  return BrowserList::GetInstance()->size() == closing_browsers_count + 1u;
 }
 
 bool Browser::MaybeCreateBackgroundContents(

chrome/browser/ui/browser.h

@@ -828,6 +828,10 @@ class Browser : public TabStripModelObserver,
 
   bool ShouldHideUIForFullscreen() const;
 
+  // Indicates if we have called BrowserList::NotifyBrowserCloseStarted for the
+  // browser.
+  bool IsBrowserClosing() const;
+
   // Returns true if we can start the shutdown sequence for the browser, i.e.
   // the last browser window is being closed.
   bool ShouldStartShutdown() const;

chrome/browser/ui/browser_list.cc

@@ -84,6 +84,7 @@ void BrowserList::RemoveBrowser(Browser* browser) {
   // Remove |browser| from the appropriate list instance.
   BrowserList* browser_list = GetInstance();
   RemoveBrowserFrom(browser, &browser_list->last_active_browsers_);
+  browser_list->currently_closing_browsers_.erase(browser);
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_BROWSER_CLOSED,
@@ -253,6 +254,8 @@ void BrowserList::NotifyBrowserNoLongerActive(Browser* browser) {
 
 // static
 void BrowserList::NotifyBrowserCloseStarted(Browser* browser) {
+  GetInstance()->currently_closing_browsers_.insert(browser);
+
   for (chrome::BrowserListObserver& observer : observers_.Get())
     observer.OnBrowserClosing(browser);
 }

chrome/browser/ui/browser_list.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/callback_forward.h"
+#include "base/containers/flat_set.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
@@ -28,6 +29,7 @@ class BrowserListObserver;
 // Maintains a list of Browser objects.
 class BrowserList {
  public:
+  using BrowserSet = base::flat_set<Browser*>;
   using BrowserVector = std::vector<Browser*>;
   using CloseCallback = base::Callback<void(const base::FilePath&)>;
   using const_iterator = BrowserVector::const_iterator;
@@ -57,6 +59,11 @@ class BrowserList {
     return last_active_browsers_.rend();
   }
 
+  // Returns the set of browsers that are currently in the closing state.
+  const BrowserSet& currently_closing_browsers() const {
+    return currently_closing_browsers_;
+  }
+
   static BrowserList* GetInstance();
 
   // Adds or removes |browser| from the list it is associated with. The browser
@@ -153,6 +160,8 @@ class BrowserList {
   // A vector of the browsers in this list that have been activated, in the
   // reverse order in which they were last activated.
   BrowserVector last_active_browsers_;
+  // A vector of the browsers that are currently in the closing state.
+  BrowserSet currently_closing_browsers_;
 
   // A list of observers which will be notified of every browser addition and
   // removal across all BrowserLists.

chrome/test/BUILD.gn

@@ -435,6 +435,7 @@ test("browser_tests") {
       "../browser/banners/app_banner_manager_browsertest.cc",
       "../browser/bitmap_fetcher/bitmap_fetcher_browsertest.cc",
       "../browser/browser_encoding_browsertest.cc",
+      "../browser/browser_shutdown_browsertest.cc",
       "../browser/browsing_data/autofill_counter_browsertest.cc",
       "../browser/browsing_data/browsing_data_cache_storage_helper_browsertest.cc",
       "../browser/browsing_data/browsing_data_database_helper_browsertest.cc",

components/keep_alive_registry/keep_alive_registry.cc

@@ -26,6 +26,14 @@ bool KeepAliveRegistry::IsKeepingAlive() const {
   return registered_count_ > 0;
 }
 
+bool KeepAliveRegistry::IsKeepingAliveOnlyByBrowserOrigin() const {
+  for (const auto& value : registered_keep_alives_) {
+    if (value.first != KeepAliveOrigin::BROWSER)
+      return false;
+  }
+  return true;
+}
+
 bool KeepAliveRegistry::IsRestartAllowed() const {
   return registered_count_ == restart_allowed_count_;
 }

components/keep_alive_registry/keep_alive_registry.h

@@ -32,6 +32,7 @@ class KeepAliveRegistry {
 
   // Methods to query the state of the registry.
   bool IsKeepingAlive() const;
+  bool IsKeepingAliveOnlyByBrowserOrigin() const;
   bool IsRestartAllowed() const;
   bool IsOriginRegistered(KeepAliveOrigin origin) const;