Added histograms on process singleton create when remote process exists and we cannot notify it

chrome/browser/process_singleton_posix.cc

 // Copyright 2014 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.
 
 // On Linux, when the user tries to launch a second copy of chrome, we check
 // for a socket in the user's profile directory.  If the socket file is open we
 // send a message to the first chrome browser process with the current
 // directory and second process command line flags.  The second process then
 // exits.
 //
@@ skipping 758 matching lines @@
     std::string hostname;
     int pid;
     if (!ParseLockPath(lock_path_, &hostname, &pid)) {
       // No lockfile exists.
       return PROCESS_NONE;
     }
 
     if (hostname.empty()) {
       // Invalid lockfile.
       UnlinkPath(lock_path_);
+      UMA_HISTOGRAM_ENUMERATION(
+          "Chrome.ProcessSingleton.RemoteProcessInteractionResult",
+          INVALID_LOCK_FILE, MAX_HUNG_REMOTE_PROCESS_ACTION);

[gab, 2017/05/10 15:16:50]

Each UMA macro expands to a fair amount of code. Extract this to a helper method
that takes the enum instead of inlining the macro multiple times for the same
enum.

[Alexey Seren, 2017/05/11 13:42:05]

Acknowledged.

       return PROCESS_NONE;
     }
 
     if (hostname != net::GetHostName() && !IsChromeProcess(pid)) {
       // Locked by process on another host. If the user selected to unlock
       // the profile, try to continue; otherwise quit.
       if (DisplayProfileInUseError(lock_path_, hostname, pid)) {
         UnlinkPath(lock_path_);
+        UMA_HISTOGRAM_ENUMERATION(
+            "Chrome.ProcessSingleton.RemoteProcessInteractionResult",
+            PROFILE_UNLOCKED, MAX_HUNG_REMOTE_PROCESS_ACTION);
         return PROCESS_NONE;
       }
       return PROFILE_IN_USE;
     }
 
     if (!IsChromeProcess(pid)) {
       // Orphaned lockfile (no process with pid, or non-chrome process.)
+      UMA_HISTOGRAM_ENUMERATION(
+          "Chrome.ProcessSingleton.RemoteProcessInteractionResult",
+          ORPHANED_LOCK_FILE, MAX_HUNG_REMOTE_PROCESS_ACTION);

[gab, 2017/05/10 15:16:50]

To be consistent I'd say have the UMA logging always on the line just before
"return PROCESS_NONE;"

[Alexey Seren, 2017/05/11 13:42:05]

Acknowledged.

       UnlinkPath(lock_path_);
       return PROCESS_NONE;
     }
 
     if (IsSameChromeInstance(pid)) {
       // Orphaned lockfile (pid is part of same chrome instance we are, even
       // though we haven't tried to create a lockfile yet).
       UnlinkPath(lock_path_);
+      UMA_HISTOGRAM_ENUMERATION(
+          "Chrome.ProcessSingleton.RemoteProcessInteractionResult",
+          SAME_BROWSER_INSTANCE, MAX_HUNG_REMOTE_PROCESS_ACTION);
       return PROCESS_NONE;
     }
 
     if (retries == retry_attempts) {
+      if (kill_unresponsive) {
+        UMA_HISTOGRAM_ENUMERATION(
+            "Chrome.ProcessSingleton.RemoteHungProcessTerminateReason",
+            NOTIFY_ATTEMPTS_EXCEEDED, MAX_REMOTE_HUNG_PROCESS_TERMINATE_REASON);

[gab, 2017/05/10 15:16:50]

Same here, instead of if (kill_unresponsive) {}, move this to right before
PROCESS_NONE (below as well)

[Alexey Seren, 2017/05/11 13:42:05]

Acknowledged.

+      }
       // Retries failed.  Kill the unresponsive chrome process and continue.
       if (!kill_unresponsive || !KillProcessByLockPath())
         return PROFILE_IN_USE;
       return PROCESS_NONE;
     }
 
     base::PlatformThread::Sleep(sleep_interval);
   }
 
   timeval socket_timeout = TimeDeltaToTimeVal(timeout);
@@ skipping 15 matching lines @@
 
   const std::vector<std::string>& argv = cmd_line.argv();
   for (std::vector<std::string>::const_iterator it = argv.begin();
       it != argv.end(); ++it) {
     to_send.push_back(kTokenDelimiter);
     to_send.append(*it);
   }
 
   // Send the message
   if (!WriteToSocket(socket.fd(), to_send.data(), to_send.length())) {
+    if (kill_unresponsive) {
+      UMA_HISTOGRAM_ENUMERATION(
+          "Chrome.ProcessSingleton.RemoteHungProcessTerminateReason",
+          SOCKET_WRITE_FAILED, MAX_REMOTE_HUNG_PROCESS_TERMINATE_REASON);
+    }
     // Try to kill the other process, because it might have been dead.
     if (!kill_unresponsive || !KillProcessByLockPath())
       return PROFILE_IN_USE;
     return PROCESS_NONE;
   }
 
   if (shutdown(socket.fd(), SHUT_WR) < 0)
     PLOG(ERROR) << "shutdown() failed";
 
   // Read ACK message from the other process. It might be blocked for a certain
   // timeout, to make sure the other process has enough time to return ACK.
   char buf[kMaxACKMessageLength + 1];
   ssize_t len = ReadFromSocket(socket.fd(), buf, kMaxACKMessageLength, timeout);
 
   // Failed to read ACK, the other process might have been frozen.
   if (len <= 0) {
+    if (kill_unresponsive) {
+      UMA_HISTOGRAM_ENUMERATION(
+          "Chrome.ProcessSingleton.RemoteHungProcessTerminateReason",
+          SOCKET_READ_FAILED, MAX_REMOTE_HUNG_PROCESS_TERMINATE_REASON);
+    }
     if (!kill_unresponsive || !KillProcessByLockPath())
       return PROFILE_IN_USE;
     return PROCESS_NONE;
   }
 
   buf[len] = '\0';
   if (strncmp(buf, kShutdownToken, arraysize(kShutdownToken) - 1) == 0) {
     // The other process is shutting down, it's safe to start a new process.
+    UMA_HISTOGRAM_ENUMERATION(
+        "Chrome.ProcessSingleton.RemoteProcessInteractionResult",
+        REMOTE_PROCESS_SHUTTING_DOWN, MAX_HUNG_REMOTE_PROCESS_ACTION);
     return PROCESS_NONE;
   } else if (strncmp(buf, kACKToken, arraysize(kACKToken) - 1) == 0) {
 #if defined(TOOLKIT_VIEWS) && defined(OS_LINUX) && !defined(OS_CHROMEOS)
     // Likely NULL in unit tests.
     views::LinuxUI* linux_ui = views::LinuxUI::instance();
     if (linux_ui)
       linux_ui->NotifyWindowManagerStartupComplete();
 #endif
 
     // Assume the other process is handling the request.
@@ skipping 175 matching lines @@
   }
   return true;
 }
 
 bool ProcessSingleton::KillProcessByLockPath() {
   std::string hostname;
   int pid;
   ParseLockPath(lock_path_, &hostname, &pid);
 
   if (!hostname.empty() && hostname != net::GetHostName()) {
-    return DisplayProfileInUseError(lock_path_, hostname, pid);
+    bool res = DisplayProfileInUseError(lock_path_, hostname, pid);
+    if (res) {
+      UnlinkPath(lock_path_);

[Alexey Seren, 2017/05/10 04:27:07]

We need to unlink lock file if user selects to unlock profile.

[gab, 2017/05/10 15:16:50]

Is this a bug fix? If so, should go in a separate CL.

[Alexey Seren, 2017/05/11 13:42:05]

Yes it is a bug fix. I will move it to separate CL

+      UMA_HISTOGRAM_ENUMERATION(
+          "Chrome.ProcessSingleton.RemoteProcessInteractionResult",
+          PROFILE_UNLOCKED, MAX_HUNG_REMOTE_PROCESS_ACTION);
+    }
+    return res;
   }
   UnlinkPath(lock_path_);
 
-  if (IsSameChromeInstance(pid))
+  if (IsSameChromeInstance(pid)) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "Chrome.ProcessSingleton.RemoteProcessInteractionResult",
+        SAME_BROWSER_INSTANCE, MAX_HUNG_REMOTE_PROCESS_ACTION);
     return true;
+  }
 
   if (pid > 0) {
     kill_callback_.Run(pid);
     return true;
   }
 
+  UMA_HISTOGRAM_ENUMERATION(
+      "Chrome.ProcessSingleton.RemoteProcessInteractionResult",
+      FAILED_TO_EXTRACT_PID, MAX_HUNG_REMOTE_PROCESS_ACTION);
+
   LOG(ERROR) << "Failed to extract pid from path: " << lock_path_.value();
   return true;
 }
 
 void ProcessSingleton::KillProcess(int pid) {
   // TODO(james.su@gmail.com): Is SIGKILL ok?
   int rv = kill(static_cast<base::ProcessHandle>(pid), SIGKILL);
   // ESRCH = No Such Process (can happen if the other process is already in
   // progress of shutting down and finishes before we try to kill it).
   DCHECK(rv == 0 || errno == ESRCH) << "Error killing process: "
                                     << base::safe_strerror(errno);
+
+  int error_code = (rv == 0) ? 0 : errno;
+  UMA_HISTOGRAM_SPARSE_SLOWLY(
+      "Chrome.ProcessSingleton.TerminateProcessErrorCode.Posix", error_code);
+
+  RemoteProcessInteractionResult action = TERMINATE_SUCCEEDED;
+  if (rv != 0) {
+    switch (error_code) {
+      case ESRCH:
+        action = REMOTE_PROCESS_NOT_FOUND;
+        break;
+      case EPERM:
+        action = TERMINATE_NOT_ENOUGH_PERMISSIONS;
+        break;
+      default: { action = TERMINATE_FAILED; }

[gab, 2017/05/10 15:16:50]

use same syntax as other cases for default, i.e. 
  action = TERMINATE_FAILED;
  break;

[Alexey Seren, 2017/05/11 13:42:05]

Acknowledged.

+    }
+  }
+  UMA_HISTOGRAM_ENUMERATION(
+      "Chrome.ProcessSingleton.RemoteProcessInteractionResult", action,
+      MAX_HUNG_REMOTE_PROCESS_ACTION);
 }