Changeset 178116 in webkit

Timestamp:

Jan 8, 2015 9:10:01 AM (9 years ago)

Author:

ap@apple.com

Message:

When WebProcess is slow to respond to IPC, that's mistakenly reported as crash
​https://bugs.webkit.org/show_bug.cgi?id=140218

Reviewed by Darin Adler.

• Scripts/webkitpy/port/driver.py:

(Driver.init): Removed _subprocess_was_unresponsive that was a modifier for
"process crashed" condition. These don't make sense together - it's either a crash
or a timeout, and we should detect those properly at a much lower level.
(Driver.run_test): Ditto.
(Driver._check_for_driver_crash_or_unresponsiveness): Split crash and unresponsiveness
cases.

• WebKitTestRunner/TestController.h:
• WebKitTestRunner/TestController.cpp:

(WTR::TestController::webProcessName):
(WTR::TestController::processDidCrash):
Factored out hardcoded child process names, as we now use these in two places.

• WebKitTestRunner/TestInvocation.cpp:

(WTR::TestInvocation::dumpWebProcessUnresponsiveness): Dump an accurate child
process name, so that it can be sampled (fixes sampling on Mavericks and above).

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/port/driver.py (modified) (4 diffs)
• WebKitTestRunner/TestController.cpp (modified) (2 diffs)
• WebKitTestRunner/TestController.h (modified) (1 diff)
• WebKitTestRunner/TestInvocation.cpp (modified) (1 diff)

trunk/Tools/ChangeLog

@@ +1 @@
+2015-01-08  Alexey Proskuryakov  <ap@apple.com>
+
+        When WebProcess is slow to respond to IPC, that's mistakenly reported as crash
+        https://bugs.webkit.org/show_bug.cgi?id=140218
+
+        Reviewed by Darin Adler.
+
+        * Scripts/webkitpy/port/driver.py:
+        (Driver.__init__): Removed _subprocess_was_unresponsive that was a modifier for
+        "process crashed" condition. These don't make sense together - it's either a crash
+        or a timeout, and we should detect those properly at a much lower level.
+        (Driver.run_test): Ditto.
+        (Driver._check_for_driver_crash_or_unresponsiveness): Split crash and unresponsiveness
+        cases.
+
+        * WebKitTestRunner/TestController.h:
+        * WebKitTestRunner/TestController.cpp:
+        (WTR::TestController::webProcessName): 
+        (WTR::TestController::processDidCrash):
+        Factored out hardcoded child process names, as we now use these in two places.
+
+        * WebKitTestRunner/TestInvocation.cpp:
+        (WTR::TestInvocation::dumpWebProcessUnresponsiveness): Dump an accurate child
+        process name, so that it can be sampled (fixes sampling on Mavericks and above).
+
 2015-01-08  Zan Dobersek  <zdobersek@igalia.com>
 

trunk/Tools/Scripts/webkitpy/port/driver.py

@@ -140 +140 @@
         self._driver_timed_out = False
 
-        # WebKitTestRunner can report back subprocesses that became unresponsive
-        # This could mean they crashed.
-        self._subprocess_was_unresponsive = False
-
         # stderr reading is scoped on a per-test (not per-block) basis, so we store the accumulated
         # stderr output, as well as if we've seen #EOF on this driver instance.
@@ -214 +210 @@
                 pid_str = str(self._crashed_pid) if self._crashed_pid else "unknown pid"
                 crash_log = 'No crash log found for %s:%s.\n' % (self._crashed_process_name, pid_str)
-                # If we were unresponsive append a message informing there may not have been a crash.
-                if self._subprocess_was_unresponsive:
-                    crash_log += 'Process failed to become responsive before timing out.\n'
 
                 # Print stdout and stderr to the placeholder crash log; we want as much context as possible.
@@ -373 +366 @@
             self._driver_timed_out = True
 
-    def _check_for_driver_crash(self, error_line):
+    def _check_for_driver_crash_or_unresponsiveness(self, error_line):
         if error_line == "#CRASHED\n":
             self._crashed_process_name = self._server_process.name()
             self._crashed_pid = self._server_process.pid()
-        elif (error_line.startswith("#CRASHED - ")
-            or error_line.startswith("#PROCESS UNRESPONSIVE - ")):
-            # WebKitTestRunner/LayoutTestRelay uses this to report that the subprocess (e.g. WebProcess) crashed.
-            match = re.match('#(?:CRASHED|PROCESS UNRESPONSIVE) - (\S+)', error_line)
+            return True
+        elif error_line.startswith("#CRASHED - "):
+            match = re.match('#CRASHED - (\S+)', error_line)
             self._crashed_process_name = match.group(1) if match else 'WebProcess'
             match = re.search('pid (\d+)', error_line)
-            pid = int(match.group(1)) if match else None
-            self._crashed_pid = pid
-            # FIXME: delete this after we're sure this code is working :)
-            _log.debug('%s crash, pid = %s, error_line = %s' % (self._crashed_process_name, str(pid), error_line))
-            if error_line.startswith("#PROCESS UNRESPONSIVE - "):
-                self._subprocess_was_unresponsive = True
-                self._port.sample_process(self._crashed_process_name, self._crashed_pid)
-                # We want to show this since it's not a regular crash and probably we don't have a crash log.
-                self.error_from_test += error_line
+            self._crashed_pid = int(match.group(1)) if match else None
+            _log.debug('%s crash, pid = %s' % (self._crashed_process_name, str(self._crashed_pid)))
             return True
-        return self.has_crashed()
+        elif error_line.startswith("#PROCESS UNRESPONSIVE - "):
+            match = re.match('#PROCESS UNRESPONSIVE - (\S+)', error_line)
+            child_process_name = match.group(1) if match else 'WebProcess'
+            match = re.search('pid (\d+)', error_line)
+            child_process_pid = int(match.group(1)) if match else None
+            _log.debug('%s is unresponsive, pid = %s' % (child_process_name, str(child_process_pid)))
+            self._driver_timed_out = True
+            if child_process_pid:
+                self._port.sample_process(child_process_name, child_process_pid)
+            self.error_from_test += error_line
+            return True
+        return False
 
     def _command_from_driver_input(self, driver_input):
@@ -500 +496 @@
 
             if err_line:
-                if self._check_for_driver_crash(err_line):
+                if self._check_for_driver_crash_or_unresponsiveness(err_line):
                     break
                 self.error_from_test += err_line

trunk/Tools/WebKitTestRunner/TestController.cpp

@@ -753 +753 @@
 }
 
+const char* TestController::webProcessName()
+{
+    // FIXME: Find a way to not hardcode the process name.
+#if PLATFORM(IOS)
+    return  "com.apple.WebKit.WebContent";
+#elif PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED > 1080
+    return "com.apple.WebKit.WebContent.Development";
+#else
+    return "WebProcess";
+#endif
+}
+
 void TestController::updateWebViewSizeForTest(const TestInvocation& test)
 {
@@ -1372 +1384 @@
 #if PLATFORM(COCOA)
         pid_t pid = WKPageGetProcessIdentifier(m_mainWebView->page());
-        // FIXME: Find a way to not hardcode the process name.
-#if PLATFORM(IOS)
-        const char* processName = "com.apple.WebKit.WebContent";
-#elif PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED > 1080
-        const char* processName = "com.apple.WebKit.WebContent.Development";
+        fprintf(stderr, "#CRASHED - %s (pid %ld)\n", webProcessName(), static_cast<long>(pid));
 #else
-        const char* processName = "WebProcess";
-#endif
-        fprintf(stderr, "#CRASHED - %s (pid %ld)\n", processName, static_cast<long>(pid));
-#else
-        fputs("#CRASHED - WebProcess\n", stderr);
+        fprintf(stderr, "#CRASHED - %s\n", webProcessName());
 #endif
         fflush(stderr);

trunk/Tools/WebKitTestRunner/TestController.h

@@ -105 +105 @@
     void reattachPageToWebProcess();
 
+    static const char* webProcessName();
+
     WorkQueueManager& workQueueManager() { return m_workQueueManager; }
 

trunk/Tools/WebKitTestRunner/TestInvocation.cpp

@@ -202 +202 @@
 void TestInvocation::dumpWebProcessUnresponsiveness(const char* errorMessage)
 {
-    const char* errorMessageToStderr = 0;
+    char errorMessageToStderr[1024];
 #if PLATFORM(COCOA)
-    char buffer[64];
     pid_t pid = WKPageGetProcessIdentifier(TestController::shared().mainWebView()->page());
-    sprintf(buffer, "#PROCESS UNRESPONSIVE - WebProcess (pid %ld)\n", static_cast<long>(pid));
-    errorMessageToStderr = buffer;
+    sprintf(errorMessageToStderr, "#PROCESS UNRESPONSIVE - %s (pid %ld)\n", TestController::webProcessName(), static_cast<long>(pid));
 #else
-    errorMessageToStderr = "#PROCESS UNRESPONSIVE - WebProcess";
+    sprintf(errorMessageToStderr, "#PROCESS UNRESPONSIVE - %s", TestController::webProcessName());
 #endif