Add retry logic for FindThreadIDWithSyscall()

components/crash/content/browser/crash_handler_host_linux.cc

 // Copyright 2013 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 "components/crash/content/browser/crash_handler_host_linux.h"
 
 #include <errno.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <sys/socket.h>
 #include <sys/syscall.h>
 #include <unistd.h>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
 #include "base/format_macros.h"
 #include "base/linux_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/rand_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/threading/platform_thread.h"
 #include "base/threading/thread.h"
 #include "breakpad/src/client/linux/handler/exception_handler.h"
 #include "breakpad/src/client/linux/minidump_writer/linux_dumper.h"
 #include "breakpad/src/client/linux/minidump_writer/minidump_writer.h"
 #include "build/build_config.h"
 #include "components/crash/content/app/breakpad_linux_impl.h"
 #include "content/public/browser/browser_thread.h"
 
 #if defined(OS_ANDROID) && !defined(__LP64__)
 #include <sys/linux-syscalls.h>
 
 #define SYS_read __NR_read
 #endif
 
 using content::BrowserThread;
 using google_breakpad::ExceptionHandler;
 
 namespace breakpad {
 
 namespace {
 
 const size_t kNumFDs = 1;
 // The length of the control message:
 const size_t kControlMsgSize =
     CMSG_SPACE(kNumFDs * sizeof(int)) + CMSG_SPACE(sizeof(struct ucred));
 // The length of the regular payload:
 const size_t kCrashContextSize = sizeof(ExceptionHandler::CrashContext);
 
+// Crashing thread might be in "running" state, i.e. after sys_sendmsg() and
+// before sys_read(). Retry 3 times with interval of 100 ms when translating
+// TID.
+const int kNumRetriesTranslatingTid = 3;
+const int kRetryIntervalTranslatingTidInMs = 100;
+
 // Handles the crash dump and frees the allocated BreakpadInfo struct.
 void CrashDumpTask(CrashHandlerHostLinux* handler,
                    std::unique_ptr<BreakpadInfo> info) {
   if (handler->IsShuttingDown() && info->upload) {
     base::DeleteFile(base::FilePath(info->filename), false);
 #if defined(ADDRESS_SANITIZER)
     base::DeleteFile(base::FilePath(info->log_filename), false);
 #endif
     return;
   }
@@ skipping 206 matching lines @@
   // |actual_crashing_pid|'s thread group and find the thread that's in the
   // read syscall with the right arguments.
 
   std::string expected_syscall_data;
   // /proc/[pid]/syscall is formatted as follows:
   // syscall_number arg1 ... arg6 sp pc
   // but we just check syscall_number through arg3.
   base::StringAppendF(&expected_syscall_data, "%d 0x%x %p 0x1 ",
                       SYS_read, tid_fd, tid_buf_addr);
   bool syscall_supported = false;
-  pid_t crashing_tid =
-      base::FindThreadIDWithSyscall(crashing_pid,
-                                    expected_syscall_data,
-                                    &syscall_supported);
+  pid_t crashing_tid = -1;
+  for (int i = 0; i < kNumRetriesTranslatingTid; ++i) {
+    crashing_tid = base::FindThreadIDWithSyscall(crashing_pid,
+                                                 expected_syscall_data,
+                                                 &syscall_supported);

[Lei Zhang, 2016/07/22 23:37:36]

If |syscall_supported| comes back false, this should not continue looping,
because it will continue to fail. Though I imagine it should return true on most
modern Linux machines.

[wzhong, 2016/07/23 00:25:11]

Acknowledged.

[wzhong, 2016/07/25 15:12:43]

Done.

+    if (crashing_tid != -1)
+      break;
+
+    base::PlatformThread::Sleep(

[Lei Zhang, 2016/07/22 23:37:36]

It's not good to sleep on the IO (AKA IPC) thread because the browser can't
process any other IPCs while waiting here. Is it possible to refactor the code
to capture the current function state and retry via PostDelayedTask() to the
current thread?

[wzhong, 2016/07/23 00:25:11]

Good point. It should be doable although context needs to be passed around.

How about PlatformThread::YieldCurrentThread()? Seems a little bit of yield or
forced scheduling already allows the crashing thread to catch up.

[wzhong, 2016/07/23 00:30:11]

Actually I'd still prefer PostDelayedTask().

[wzhong, 2016/07/25 15:12:43]

Done.

+        base::TimeDelta::FromMilliseconds(kRetryIntervalTranslatingTidInMs));
+  }
   if (crashing_tid == -1) {
     // We didn't find the thread we want. Maybe it didn't reach
     // sys_read() yet or the thread went away.  We'll just take a
     // guess here and assume the crashing thread is the thread group
     // leader.  If procfs syscall is not supported by the kernel, then
     // we assume the kernel also does not support TID namespacing and
     // trust the TID passed by the crashing process.
     LOG(WARNING) << "Could not translate tid - assuming crashing thread is "
         "thread group leader; syscall_supported=" << syscall_supported;
     crashing_tid = crashing_pid;
@@ skipping 137 matching lines @@
   // no-ops.
   shutting_down_ = true;
   uploader_thread_->Stop();
 }
 
 bool CrashHandlerHostLinux::IsShuttingDown() const {
   return shutting_down_;
 }
 
 }  // namespace breakpad