Wait properly for renderer crashes

This replaces a Sleep in automation with a wait for renderer crash. It turns out that our IPC on POSIX had one loophole that caused it not to notice very early crashes, so I also fixed that.

The problem was that when the child process died before connecting to the parent's IPC channel, the parent wouldn't notice the crash because the child end of the IPC pipe was kept open for too long. This change makes the code close the child end of the pipe right after forking the child.

This might also help with automation not noticing the browser crash during initial launch, or at least should be a good step toward fixing that problem.

BUG=38497,90489

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

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

8 files changed, 77 insertions, 50 deletions

diff --git a/chrome/browser/automation/automation_provider_observers.cc b/chrome/browser/automation/automation_provider_observers.cc
index b5b058a..8377cc8 100644
--- a/chrome/browser/automation/automation_provider_observers.cc
+++ b/chrome/browser/automation/automation_provider_observers.cc
@@ -100,6 +100,7 @@ class InitialLoadObserver::TabTime {
 InitialLoadObserver::InitialLoadObserver(size_t tab_count,
                                          AutomationProvider* automation)
     : automation_(automation->AsWeakPtr()),
+      crashed_tab_count_(0),
       outstanding_tab_count_(tab_count),
       init_time_(base::TimeTicks::Now()) {
   if (outstanding_tab_count_ > 0) {
@@ -107,6 +108,8 @@ InitialLoadObserver::InitialLoadObserver(size_t tab_count,
                    NotificationService::AllSources());
     registrar_.Add(this, content::NOTIFICATION_LOAD_STOP,
                    NotificationService::AllSources());
+    registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                   NotificationService::AllSources());
   }
 }
 
@@ -128,12 +131,37 @@ void InitialLoadObserver::Observe(int type,
         finished_tabs_.insert(source.map_key());
         iter->second.set_stop_time(base::TimeTicks::Now());
       }
-      if (outstanding_tab_count_ == finished_tabs_.size())
-        ConditionMet();
+    }
+  } else if (type == content::NOTIFICATION_RENDERER_PROCESS_CLOSED) {
+    base::TerminationStatus status =
+        Details<RenderProcessHost::RendererClosedDetails>(details)->status;
+    switch (status) {
+      case base::TERMINATION_STATUS_NORMAL_TERMINATION:
+        break;
+
+      case base::TERMINATION_STATUS_ABNORMAL_TERMINATION:
+      case base::TERMINATION_STATUS_PROCESS_WAS_KILLED:
+      case base::TERMINATION_STATUS_PROCESS_CRASHED:
+        crashed_tab_count_++;
+        break;
+
+      case base::TERMINATION_STATUS_STILL_RUNNING:
+        LOG(ERROR) << "Got RENDERER_PROCESS_CLOSED notification, "
+                   << "but the process is still running. We may miss further "
+                   << "crash notification, resulting in hangs.";
+        break;
+
+      default:
+        LOG(ERROR) << "Unhandled termination status " << status;
+        NOTREACHED();
+        break;
     }
   } else {
     NOTREACHED();
   }
+
+  if (finished_tabs_.size() + crashed_tab_count_ >= outstanding_tab_count_)
+    ConditionMet();
 }
 
 DictionaryValue* InitialLoadObserver::GetTimingInformation() const {
diff --git a/chrome/browser/automation/automation_provider_observers.h b/chrome/browser/automation/automation_provider_observers.h
index 4aa04b0..c65d208 100644
--- a/chrome/browser/automation/automation_provider_observers.h
+++ b/chrome/browser/automation/automation_provider_observers.h
@@ -109,6 +109,7 @@ class InitialLoadObserver : public NotificationObserver {
   NotificationRegistrar registrar_;
 
   base::WeakPtr<AutomationProvider> automation_;
+  size_t crashed_tab_count_;
   size_t outstanding_tab_count_;
   base::TimeTicks init_time_;
   TabTimeMap loading_tabs_;
diff --git a/chrome/browser/importer/firefox_importer_unittest_utils_mac.cc b/chrome/browser/importer/firefox_importer_unittest_utils_mac.cc
index 09a29a4..83d5283 100644
--- a/chrome/browser/importer/firefox_importer_unittest_utils_mac.cc
+++ b/chrome/browser/importer/firefox_importer_unittest_utils_mac.cc
@@ -7,6 +7,7 @@
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
+#include "base/file_util.h"
 #include "base/message_loop.h"
 #include "base/test/test_timeouts.h"
 #include "chrome/browser/importer/firefox_importer_utils.h"
@@ -30,7 +31,7 @@ const char kTestChannelID[] = "T1";
 // |handle| - On return, the process handle to use to communicate with the
 // child.
 bool LaunchNSSDecrypterChildProcess(const FilePath& nss_path,
-    const IPC::Channel& channel, base::ProcessHandle* handle) {
+    IPC::Channel* channel, base::ProcessHandle* handle) {
   CommandLine cl(*CommandLine::ForCurrentProcess());
   cl.AppendSwitchASCII(switches::kTestChildProcess, "NSSDecrypterChildProcess");
 
@@ -43,13 +44,13 @@ bool LaunchNSSDecrypterChildProcess(const FilePath& nss_path,
   dyld_override.second = nss_path.value();
   env.push_back(dyld_override);
 
-  base::file_handle_mapping_vector fds_to_map;
-  const int ipcfd = channel.GetClientFileDescriptor();
-  if (ipcfd > -1) {
-    fds_to_map.push_back(std::pair<int,int>(ipcfd, kPrimaryIPCChannel + 3));
-  } else {
+  int ipcfd = channel->TakeClientFileDescriptor();
+  if (ipcfd == -1)
     return false;
-  }
+
+  file_util::ScopedFD client_file_descriptor_closer(&ipcfd);
+  base::file_handle_mapping_vector fds_to_map;
+  fds_to_map.push_back(std::pair<int,int>(ipcfd, kPrimaryIPCChannel + 3));
 
   bool debug_on_start = CommandLine::ForCurrentProcess()->HasSwitch(
                             switches::kDebugChildren);
@@ -139,7 +140,7 @@ bool FFUnitTestDecryptorProxy::Setup(const FilePath& nss_path) {
 
   // Spawn child and set up sync IPC connection.
   bool ret = LaunchNSSDecrypterChildProcess(nss_path,
-                                            *(channel_.get()),
+                                            channel_.get(),
                                             &child_process_);
   return ret && (child_process_ != 0);
 }
diff --git a/chrome/common/logging_chrome_uitest.cc b/chrome/common/logging_chrome_uitest.cc
index b72661a..d4bbfa1 100644
--- a/chrome/common/logging_chrome_uitest.cc
+++ b/chrome/common/logging_chrome_uitest.cc
@@ -84,16 +84,14 @@ TEST_F(ChromeLoggingTest, EnvironmentLogFileName) {
 #endif
 
 #if !defined(NDEBUG)  // We don't have assertions in release builds.
-// Tests whether we correctly fail on browser assertions during tests.
+// Tests whether we correctly fail on renderer assertions during tests.
 class AssertionTest : public UITest {
  protected:
-  AssertionTest() : UITest() {
-    // Initial loads will never complete due to assertion.
+  AssertionTest() {
+#if defined(OS_WIN)
+    // TODO(phajdan.jr): Make crash notifications on launch work on Win.
     wait_for_initial_loads_ = false;
-
-    // We're testing the renderer rather than the browser assertion here,
-    // because the browser assertion would flunk the test during SetUp()
-    // (since TAU wouldn't be able to find the browser window).
+#endif
     launch_arguments_.AppendSwitch(switches::kRendererAssertTest);
   }
 };
@@ -116,13 +114,11 @@ TEST_F(AssertionTest, Assertion) {
 // Only works on Linux in Release mode with CHROME_HEADLESS=1
 class CheckFalseTest : public UITest {
  protected:
-  CheckFalseTest() : UITest() {
-    // Initial loads will never complete due to assertion.
+  CheckFalseTest() {
+#if defined(OS_WIN)
+    // TODO(phajdan.jr): Make crash notifications on launch work on Win.
     wait_for_initial_loads_ = false;
-
-    // We're testing the renderer rather than the browser assertion here,
-    // because the browser assertion would flunk the test during SetUp()
-    // (since TAU wouldn't be able to find the browser window).
+#endif
     launch_arguments_.AppendSwitch(switches::kRendererCheckFalseTest);
   }
 };
@@ -144,10 +140,11 @@ TEST_F(CheckFalseTest, CheckFails) {
 // Tests whether we correctly fail on browser crashes during UI Tests.
 class RendererCrashTest : public UITest {
  protected:
-  RendererCrashTest() : UITest() {
-    // Initial loads will never complete due to crash.
+  RendererCrashTest() {
+#if defined(OS_WIN)
+    // TODO(phajdan.jr): Make crash notifications on launch work on Win.
     wait_for_initial_loads_ = false;
-
+#endif
     launch_arguments_.AppendSwitch(switches::kRendererCrashTest);
   }
 };
diff --git a/chrome/test/automation/automation_proxy.cc b/chrome/test/automation/automation_proxy.cc
index 4704deb..10bcf6d 100644
--- a/chrome/test/automation/automation_proxy.cc
+++ b/chrome/test/automation/automation_proxy.cc
@@ -432,15 +432,9 @@ scoped_refptr<BrowserProxy> AutomationProxy::GetLastActiveBrowserWindow() {
   return ProxyObjectFromHandle<BrowserProxy>(handle);
 }
 
-#if defined(OS_POSIX)
-base::file_handle_mapping_vector AutomationProxy::fds_to_map() const {
-  base::file_handle_mapping_vector map;
-  const int ipcfd = channel_->GetClientFileDescriptor();
-  if (ipcfd > -1)
-    map.push_back(std::make_pair(ipcfd, kPrimaryIPCChannel + 3));
-  return map;
-}
-#endif  // defined(OS_POSIX)
+IPC::SyncChannel* AutomationProxy::channel() {
+  return channel_.get();
+}
 
 bool AutomationProxy::Send(IPC::Message* message) {
   return Send(message,
diff --git a/chrome/test/automation/automation_proxy.h b/chrome/test/automation/automation_proxy.h
index a41689d..0fccd90 100644
--- a/chrome/test/automation/automation_proxy.h
+++ b/chrome/test/automation/automation_proxy.h
@@ -222,9 +222,7 @@ class AutomationProxy : public IPC::Channel::Listener,
                             const std::string& password) WARN_UNUSED_RESULT;
 #endif
 
-#if defined(OS_POSIX)
-  base::file_handle_mapping_vector fds_to_map() const;
-#endif
+  IPC::SyncChannel* channel();
 
   // AutomationMessageSender implementation.
   virtual bool Send(IPC::Message* message) WARN_UNUSED_RESULT;
diff --git a/chrome/test/automation/proxy_launcher.cc b/chrome/test/automation/proxy_launcher.cc
index 34c4060..469a0f2 100644
--- a/chrome/test/automation/proxy_launcher.cc
+++ b/chrome/test/automation/proxy_launcher.cc
@@ -26,6 +26,7 @@
 #include "content/common/child_process_info.h"
 #include "content/common/debug_flags.h"
 #include "ipc/ipc_channel.h"
+#include "ipc/ipc_descriptors.h"
 #include "sql/connection.h"
 
 namespace {
@@ -109,11 +110,11 @@ bool ProxyLauncher::WaitForBrowserLaunch(bool wait_for_initial_loads) {
       return false;
     }
   } else {
-    // TODO(phajdan.jr): We should get rid of this sleep, but some tests
-    // "rely" on it, e.g. AssertionTest.Assertion and CheckFalseTest.CheckFails.
-    // Those tests do not wait in any way until the crash gets noticed,
-    // so it's possible for the browser to exit before the tested crash happens.
+#if defined(OS_WIN)
+    // TODO(phajdan.jr): Get rid of this Sleep when logging_chrome_uitest
+    // stops "relying" on it.
     base::PlatformThread::Sleep(TestTimeouts::action_timeout_ms());
+#endif
   }
 
   if (!automation()->SetFilteredInet(ShouldFilterInet())) {
@@ -198,7 +199,7 @@ bool ProxyLauncher::LaunchBrowser(const LaunchState& state) {
   if (!state.setup_profile_callback.is_null())
     state.setup_profile_callback.Run();
 
-  if (!LaunchBrowserHelper(state, false, &process_)) {
+  if (!LaunchBrowserHelper(state, true, false, &process_)) {
     LOG(ERROR) << "LaunchBrowserHelper failed.";
     return false;
   }
@@ -210,7 +211,7 @@ bool ProxyLauncher::LaunchBrowser(const LaunchState& state) {
 #if !defined(OS_MACOSX)
 bool ProxyLauncher::LaunchAnotherBrowserBlockUntilClosed(
     const LaunchState& state) {
-  return LaunchBrowserHelper(state, true, NULL);
+  return LaunchBrowserHelper(state, false, true, NULL);
 }
 #endif
 
@@ -435,7 +436,9 @@ void ProxyLauncher::PrepareTestCommandline(CommandLine* command_line,
   command_line->AppendSwitch(switches::kUnlimitedQuotaForFiles);
 }
 
-bool ProxyLauncher::LaunchBrowserHelper(const LaunchState& state, bool wait,
+bool ProxyLauncher::LaunchBrowserHelper(const LaunchState& state,
+                                        bool main_launch,
+                                        bool wait,
                                         base::ProcessHandle* process) {
   CommandLine command_line(state.command);
 
@@ -459,8 +462,8 @@ bool ProxyLauncher::LaunchBrowserHelper(const LaunchState& state, bool wait,
             << browser_wrapper;
   }
 
-  // TODO(phajdan.jr): Only run it for "main" browser launch.
-  browser_launch_time_ = base::TimeTicks::Now();
+  if (main_launch)
+    browser_launch_time_ = base::TimeTicks::Now();
 
   base::LaunchOptions options;
   options.wait = wait;
@@ -468,10 +471,14 @@ bool ProxyLauncher::LaunchBrowserHelper(const LaunchState& state, bool wait,
 #if defined(OS_WIN)
   options.start_hidden = !state.show_window;
 #elif defined(OS_POSIX)
+  int ipcfd = -1;
+  file_util::ScopedFD ipcfd_closer(&ipcfd);
   base::file_handle_mapping_vector fds;
-  if (automation_proxy_.get())
-    fds = automation_proxy_->fds_to_map();
-  options.fds_to_remap = &fds;
+  if (main_launch && automation_proxy_.get()) {
+    ipcfd = automation_proxy_->channel()->TakeClientFileDescriptor();
+    fds.push_back(std::make_pair(ipcfd, kPrimaryIPCChannel + 3));
+    options.fds_to_remap = &fds;
+  }
 #endif
 
   return base::LaunchProcess(command_line, options, process);
diff --git a/chrome/test/automation/proxy_launcher.h b/chrome/test/automation/proxy_launcher.h
index 0e497b7..64f5b2b 100644
--- a/chrome/test/automation/proxy_launcher.h
+++ b/chrome/test/automation/proxy_launcher.h
@@ -166,6 +166,7 @@ class ProxyLauncher {
                               bool include_testing_id);
 
   bool LaunchBrowserHelper(const LaunchState& state,
+                           bool main_launch,
                            bool wait,
                            base::ProcessHandle* process) WARN_UNUSED_RESULT;