CPM: revised unclean exit watching, revised renderer crash test

chrome/browser/performance_monitor/performance_monitor_browsertest.cc

Index: chrome/browser/performance_monitor/performance_monitor_browsertest.cc
diff --git a/chrome/browser/performance_monitor/performance_monitor_browsertest.cc b/chrome/browser/performance_monitor/performance_monitor_browsertest.cc
index 59923a81492765bb83bd4914a78837dbe9c417ec..658b53b473f3d298e431b278afd2372bff453036 100644
--- a/chrome/browser/performance_monitor/performance_monitor_browsertest.cc
+++ b/chrome/browser/performance_monitor/performance_monitor_browsertest.cc
@@ -4,10 +4,13 @@
 
 #include "chrome/test/base/in_process_browser_test.h"
 
+#include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/path_service.h"
+#include "base/string_number_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/performance_monitor/constants.h"
 #include "chrome/browser/performance_monitor/database.h"
 #include "chrome/browser/performance_monitor/performance_monitor.h"
@@ -15,10 +18,13 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/unpacked_installer.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/url_constants.h"
@@ -196,6 +202,67 @@ class PerformanceMonitorBrowserTest : public ExtensionBrowserTest {
   PerformanceMonitor* performance_monitor_;
 };
 
+class PerformanceMonitorUncleanExitBrowserTest
+    : public PerformanceMonitorBrowserTest {
+ public:
+  virtual bool SetUpUserDataDirectory() OVERRIDE {
+    FilePath user_data_directory;
+    PathService::Get(chrome::DIR_USER_DATA, &user_data_directory);
+
+    // The default profile directory can be changed on CrOS if we are logged in

[Yoyo Zhou, 2012/07/30 09:01:02]

What does 'can be changed' mean?

[Devlin, 2012/07/30 17:22:25]

I hope this is a bit more descriptive? The entire situation is a bit hazy, and
difficult to describe in a short comment...

+    // (trybots and buildbots are typically logged in as 'user'), so we need
+    // a separate definition.
+#if defined(OS_CHROMEOS)
+    const CommandLine command_line = *CommandLine::ForCurrentProcess();
+    if (command_line.HasSwitch(switches::kLoginProfile)) {

[Yoyo Zhou, 2012/07/30 09:01:02]

What's the default setup for this switch on the ChromeOS bots?

[Devlin, 2012/07/30 17:22:25]

Unfortunately, I couldn't find anything saying which switches were enabled on
the trybot runs - but I'm fairly certain trybots run with something like
--login-profile=user. This is why these tests failed on CrOS before - the tests
only used "Default" before. By sending this through the trybots with various
logs inserted (ugly, but effective), and using
ProfileManager::GetInitialProfileDir (would link to code, but codesearch is
refusing to open the file), I found that the CrOS trybots have a profile name of
"user" and do enter these if-statements.

Also, a side-note - if this configuration changes, then the result is just that
it won't enter this if statement, and will use "Default" again - so it won't
break anything (hopefully).

Does this make sense? If not, I can try to clear it up more in an email (with
trybot logs, etc).

+      first_profile_name_ =
+          command_line.GetSwitchValueASCII(switches::kLoginProfile);
+    } else {
+      first_profile_name_ = chrome::kInitialProfile;
+    }
+#else
+    first_profile_name_ = chrome::kInitialProfile;
+#endif
+
+    FilePath first_profile =
+        user_data_directory.AppendASCII(first_profile_name_);
+    CHECK(file_util::CreateDirectory(first_profile));
+
+    FilePath stock_prefs_file;
+    PathService::Get(chrome::DIR_TEST_DATA, &stock_prefs_file);
+    stock_prefs_file = stock_prefs_file.AppendASCII("performance_monitor")
+                                       .AppendASCII("unclean_exit_prefs");
+    CHECK(file_util::PathExists(stock_prefs_file));
+
+    FilePath first_profile_prefs_file =
+        first_profile.Append(chrome::kPreferencesFilename);
+    CHECK(file_util::CopyFile(stock_prefs_file, first_profile_prefs_file));
+    CHECK(file_util::PathExists(first_profile_prefs_file));
+
+    second_profile_name_ =
+        std::string(chrome::kMultiProfileDirPrefix)
+        .append(base::IntToString(1));
+
+    FilePath second_profile =
+        user_data_directory.AppendASCII(second_profile_name_);
+    CHECK(file_util::CreateDirectory(second_profile));
+
+    FilePath second_profile_prefs_file =
+        second_profile.Append(chrome::kPreferencesFilename);
+    CHECK(file_util::CopyFile(stock_prefs_file, second_profile_prefs_file));
+    CHECK(file_util::PathExists(second_profile_prefs_file));
+
+    return true;
+  }
+
+  std::string first_profile_name() { return first_profile_name_; }

[Yoyo Zhou, 2012/07/30 09:01:02]

nit: it doesn't look like these accessors are necessary.

[Devlin, 2012/07/30 17:22:25]

Whoops, sorry - habit to have accessors instead of use members directly in
tests. Done.

+  std::string second_profile_name() { return second_profile_name_; }
+
+ protected:
+  std::string first_profile_name_;
+  std::string second_profile_name_;
+};
+
 // Test that PerformanceMonitor will correctly record an extension installation
 // event.
 IN_PROC_BROWSER_TEST_F(PerformanceMonitorBrowserTest, InstallExtensionEvent) {
@@ -410,14 +477,77 @@ IN_PROC_BROWSER_TEST_F(PerformanceMonitorBrowserTest, KilledByOSEvent) {
 }
 #endif  // !defined(OS_WIN)
 
-IN_PROC_BROWSER_TEST_F(PerformanceMonitorBrowserTest,
-                       DISABLED_RendererCrashEvent) {
+IN_PROC_BROWSER_TEST_F(PerformanceMonitorBrowserTest, RendererCrashEvent) {
+  content::WindowedNotificationObserver windowed_observer(
+      content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+      content::NotificationService::AllSources());
+
   ui_test_utils::NavigateToURL(browser(), GURL(chrome::kChromeUICrashURL));
 
+  windowed_observer.Wait();
+
   std::vector<linked_ptr<Event> > events = GetEvents();
   ASSERT_EQ(1u, events.size());
 
   CheckEventType(EVENT_RENDERER_CRASH, events[0]);
 }
 
+IN_PROC_BROWSER_TEST_F(PerformanceMonitorUncleanExitBrowserTest,
+                       OneProfileUncleanExit) {
+  const std::string time = "12985807272597591";
+  const size_t kNumEvents = 1;

[Yoyo Zhou, 2012/07/30 09:01:02]

Define this closer to where it's used.

[Devlin, 2012/07/30 17:22:25]

Done.

+
+  // Initialize the database value (if there's no value in the database, it
+  // can't determine the last active time of the profile, and doesn't insert
+  // the event).
+  AddStateValue(kStateProfilePrefix + first_profile_name(), time);
+
+  performance_monitor()->CheckForUncleanExits();
+
+  ui_test_utils::RunAllPendingInMessageLoop();
+
+  std::vector<linked_ptr<Event> > events = GetEvents();
+  ASSERT_EQ(kNumEvents, events.size());
+
+  CheckEventType(EVENT_UNCLEAN_EXIT, events[0]);
+
+  std::string event_profile;
+  ASSERT_TRUE(events[0]->data()->GetString("profileName", &event_profile));
+  ASSERT_EQ(first_profile_name(), event_profile);
+}
+
+IN_PROC_BROWSER_TEST_F(PerformanceMonitorUncleanExitBrowserTest,
+                       TwoProfileUncleanExit) {
+  const std::string time1 = "12985807272597591";
+  const std::string time2 = "12985807272599918";
+  const size_t kNumEvents = 2;

[Yoyo Zhou, 2012/07/30 09:01:02]

ditto

[Devlin, 2012/07/30 17:22:25]

Done.

+
+  FilePath second_profile_path;
+  PathService::Get(chrome::DIR_USER_DATA, &second_profile_path);
+  second_profile_path = second_profile_path.AppendASCII(second_profile_name());
+
+  AddStateValue(kStateProfilePrefix + first_profile_name(), time1);
+  AddStateValue(kStateProfilePrefix + second_profile_name(), time2);
+
+  performance_monitor()->CheckForUncleanExits();
+  ui_test_utils::RunAllPendingInMessageLoop();
+
+  // Load the second profile, which has also exited uncleanly.
+  g_browser_process->profile_manager()->GetProfile(second_profile_path);
+  ui_test_utils::RunAllPendingInMessageLoop();
+
+  std::vector<linked_ptr<Event> > events = GetEvents();
+
+  ASSERT_EQ(kNumEvents, events.size());
+  CheckEventType(EVENT_UNCLEAN_EXIT, events[0]);
+  CheckEventType(EVENT_UNCLEAN_EXIT, events[1]);
+
+  std::string event_profile;
+  ASSERT_TRUE(events[0]->data()->GetString("profileName", &event_profile));
+  ASSERT_EQ(first_profile_name(), event_profile);
+
+  ASSERT_TRUE(events[1]->data()->GetString("profileName", &event_profile));
+  ASSERT_EQ(second_profile_name(), event_profile);
+}
+
 }  // namespace performance_monitor