Fix a crash on browser exit after opening TaskManager.

TaskManager is a singleton, so it's destroyed by AtExitManager. At the time
of destruction it cannot register AtExit callbacks (AtExitManager requires
that). It turns out that some Windows view code wants to register an AtExit
callback during destruction.

For more info about view code, see http://src.chromium.org/viewvc/chrome?view=rev&revision=9161

The fix is to destroy the view early, using EnsureShutdown static method
of TaskManager. It was also necessary to delete child views a bit earlier
to avoid another crashed.

Added a regression browser_test and verified that it's broken without this fix.

http://crbug.com/11180

Review URL: http://codereview.chromium.org/114031

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@16474 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 61 insertions, 0 deletions

diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc
index 18e3a83..bf97e56 100644
--- a/chrome/browser/browser_process_impl.cc
+++ b/chrome/browser/browser_process_impl.cc
@@ -26,6 +26,7 @@
 #include "chrome/browser/renderer_host/render_process_host.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/task_manager.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/notification_service.h"
@@ -212,6 +213,13 @@ BrowserProcessImpl::~BrowserProcessImpl() {
   print_job_manager_->OnQuit();
   print_job_manager_.reset();
 
+#if defined(OS_WIN) || defined(OS_LINUX)
+  // TODO(port): Remove #ifdef when TaskManager is ported to Mac.
+  // Make sure that TaskManager destroys things that can't be safely destroyed
+  // by AtExitManager. Does nothing if TaskManager has not been initialized.
+  TaskManager::EnsureShutdown();
+#endif  // defined(OS_WIN) || defined(OS_LINUX)
+
   // Now OK to destroy NotificationService.
   main_notification_service_.reset();
 
diff --git a/chrome/browser/task_manager.cc b/chrome/browser/task_manager.cc
index 7a7a252..c4e186b 100644
--- a/chrome/browser/task_manager.cc
+++ b/chrome/browser/task_manager.cc
@@ -688,6 +688,9 @@ bool TaskManagerModel::GetProcessMetricsForRows(
 ////////////////////////////////////////////////////////////////////////////////
 
 // static
+bool TaskManager::initialized_ = false;
+
+// static
 void TaskManager::RegisterPrefs(PrefService* prefs) {
   prefs->RegisterDictionaryPref(prefs::kTaskManagerWindowPlacement);
 }
@@ -695,6 +698,7 @@ void TaskManager::RegisterPrefs(PrefService* prefs) {
 TaskManager::TaskManager()
     : ALLOW_THIS_IN_INITIALIZER_LIST(model_(new TaskManagerModel(this))) {
   Init();
+  initialized_ = true;
 }
 
 TaskManager::~TaskManager() {
@@ -711,6 +715,19 @@ void TaskManager::Close() {
   model_->Clear();
 }
 
+// static
+void TaskManager::EnsureShutdown() {
+  if (!initialized_)
+    return;
+
+  // TaskManager is a singleton, which means it's destroyed by AtExitManager.
+  // At that point it can't register AtExit callbacks etc. It turns out that
+  // view destruction code does it on Windows, so we destroy the view now.
+  TaskManager* task_manager = GetInstance();
+  task_manager->view_.reset();
+  initialized_ = false;
+}
+
 bool TaskManager::BrowserProcessIsSelected() {
   if (!view_.get())
     return false;
diff --git a/chrome/browser/task_manager.h b/chrome/browser/task_manager.h
index d58a21e..a66cb8f 100644
--- a/chrome/browser/task_manager.h
+++ b/chrome/browser/task_manager.h
@@ -98,6 +98,10 @@ class TaskManager {
   // Close the task manager.
   void Close();
 
+  // Do the cleanup that can't be done during singleton destruction by
+  // AtExitManager. Does nothing if the TaskManager hasn't been initialized.
+  static void EnsureShutdown();
+
   // Returns true if the current selection includes the browser process.
   bool BrowserProcessIsSelected();
 
@@ -133,6 +137,9 @@ class TaskManager {
   // Returns the singleton instance (and initializes it if necessary).
   static TaskManager* GetInstance();
 
+  // Flag set to true when TaskManager is initialized.
+  static bool initialized_;
+
   // The model used for gathering and processing task data. It is ref counted
   // because it is passed as a parameter to MessageLoop::InvokeLater().
   scoped_refptr<TaskManagerModel> model_;
diff --git a/chrome/browser/task_manager_browsertest.cc b/chrome/browser/task_manager_browsertest.cc
new file mode 100644
index 0000000..6eccc6b
--- /dev/null
+++ b/chrome/browser/task_manager_browsertest.cc
@@ -0,0 +1,19 @@
+// Copyright (c) 2009 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 "chrome/browser/task_manager.h"
+
+#include "chrome/test/in_process_browser_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class TaskManagerBrowserTest : public InProcessBrowserTest {
+};
+
+// Regression test for http://crbug.com/11180
+IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, ShutdownWhileOpen) {
+  TaskManager::Open();
+}
+
+// TODO(phajdan.jr): Write another test which explicitly closes TaskManager.
+// This requires a small refactoring.
diff --git a/chrome/browser/task_manager_win.cc b/chrome/browser/task_manager_win.cc
index 9b0db00..197cde6 100644
--- a/chrome/browser/task_manager_win.cc
+++ b/chrome/browser/task_manager_win.cc
@@ -260,6 +260,8 @@ TaskManagerViewImpl::TaskManagerViewImpl(TaskManager* task_manager,
 }
 
 TaskManagerViewImpl::~TaskManagerViewImpl() {
+  // Delete child views now, while our table model still exists.
+  RemoveAllChildViews(true);
 }
 
 void TaskManagerViewImpl::Init() {
diff --git a/chrome/test/browser/browser_tests_dll.vcproj b/chrome/test/browser/browser_tests_dll.vcproj
index 4013695..9f9a321 100644
--- a/chrome/test/browser/browser_tests_dll.vcproj
+++ b/chrome/test/browser/browser_tests_dll.vcproj
@@ -203,6 +203,14 @@
 			</File>

 		</Filter>

 		<Filter

+			Name="TestTaskManager"

+			>

+			<File

+				RelativePath="..\..\browser\task_manager_browsertest.cc"

+				>

+			</File>

+		</Filter>

+		<Filter

 			Name="TestSSL"

 			>

 			<File