[Android] Fix a race condition in run_command.py.

This patch makes limited changes to minimize the likelyhood of breaking
dependent scripts.

NOTRY=True
TBR=brettw@chromium.org
BUG=180587
TEST=ran run_command.RunOnce with a timeout of 0.

Review URL: https://codereview.chromium.org/13047009

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

3 files changed, 114 insertions, 36 deletions

diff --git a/third_party/android_testrunner/README.chromium b/third_party/android_testrunner/README.chromium
index a870d7e..6a11861 100644
--- a/third_party/android_testrunner/README.chromium
+++ b/third_party/android_testrunner/README.chromium
@@ -14,6 +14,8 @@ Local Modifications:
   suppressed.
 2. Changed error message handling in |StartInstrumentation| from |shortMsg| to
   |longMsg| to provide more information when debugging.
+3. Applied the patch file patch.diff to fix a race condition in
+  run_command.RunOnce.
 
 Here is the detail steps
 1. Checkout Android source code
diff --git a/third_party/android_testrunner/patch.diff b/third_party/android_testrunner/patch.diff
new file mode 100644
index 0000000..2205a1c
--- /dev/null
+++ b/third_party/android_testrunner/patch.diff
@@ -0,0 +1,87 @@
+diff --git a/third_party/android_testrunner/run_command.py b/third_party/android_testrunner/run_command.py
+index d398daa..812037d 100644
+--- a/third_party/android_testrunner/run_command.py
++++ b/third_party/android_testrunner/run_command.py
+@@ -80,29 +80,28 @@ def RunOnce(cmd, timeout_time=None, return_output=True, stdin_input=None):
+   """
+   start_time = time.time()
+   so = []
+-  pid = []
+   global _abort_on_error, error_occurred
+   error_occurred = False
+ 
++  if return_output:
++    output_dest = subprocess.PIPE
++  else:
++    # None means direct to stdout
++    output_dest = None
++  if stdin_input:
++    stdin_dest = subprocess.PIPE
++  else:
++    stdin_dest = None
++  pipe = subprocess.Popen(
++      cmd,
++      executable='/bin/bash',
++      stdin=stdin_dest,
++      stdout=output_dest,
++      stderr=subprocess.STDOUT,
++      shell=True)
++
+   def Run():
+     global error_occurred
+-    if return_output:
+-      output_dest = subprocess.PIPE
+-    else:
+-      # None means direct to stdout
+-      output_dest = None
+-    if stdin_input:
+-      stdin_dest = subprocess.PIPE
+-    else:
+-      stdin_dest = None
+-    pipe = subprocess.Popen(
+-        cmd,
+-        executable='/bin/bash',
+-        stdin=stdin_dest,
+-        stdout=output_dest,
+-        stderr=subprocess.STDOUT,
+-        shell=True)
+-    pid.append(pipe.pid)
+     try:
+       output = pipe.communicate(input=stdin_input)[0]
+       if output is not None and len(output) > 0:
+@@ -119,27 +118,17 @@ def RunOnce(cmd, timeout_time=None, return_output=True, stdin_input=None):
+ 
+   t = threading.Thread(target=Run)
+   t.start()
+-
+-  break_loop = False
+-  while not break_loop:
+-    if not t.isAlive():
+-      break_loop = True
+-
+-    # Check the timeout
+-    if (not break_loop and timeout_time is not None
+-        and time.time() > start_time + timeout_time):
+-      try:
+-        os.kill(pid[0], signal.SIGKILL)
+-      except OSError:
+-        # process already dead. No action required.
+-        pass
+-
++  t.join(timeout_time)
++  if t.isAlive():
++    try:
++      pipe.kill()
++    except OSError:
++      # Can't kill a dead process.
++      pass
++    finally:
+       logger.SilentLog("about to raise a timeout for: %s" % cmd)
+       raise errors.WaitForResponseTimedOutError
+-    if not break_loop:
+-      time.sleep(0.1)
+ 
+-  t.join()
+   output = "".join(so)
+   if _abort_on_error and error_occurred:
+     raise errors.AbortError(msg=output)
diff --git a/third_party/android_testrunner/run_command.py b/third_party/android_testrunner/run_command.py
index d398daa..812037d 100644
--- a/third_party/android_testrunner/run_command.py
+++ b/third_party/android_testrunner/run_command.py
@@ -80,29 +80,28 @@ def RunOnce(cmd, timeout_time=None, return_output=True, stdin_input=None):
   """
   start_time = time.time()
   so = []
-  pid = []
   global _abort_on_error, error_occurred
   error_occurred = False
 
+  if return_output:
+    output_dest = subprocess.PIPE
+  else:
+    # None means direct to stdout
+    output_dest = None
+  if stdin_input:
+    stdin_dest = subprocess.PIPE
+  else:
+    stdin_dest = None
+  pipe = subprocess.Popen(
+      cmd,
+      executable='/bin/bash',
+      stdin=stdin_dest,
+      stdout=output_dest,
+      stderr=subprocess.STDOUT,
+      shell=True)
+
   def Run():
     global error_occurred
-    if return_output:
-      output_dest = subprocess.PIPE
-    else:
-      # None means direct to stdout
-      output_dest = None
-    if stdin_input:
-      stdin_dest = subprocess.PIPE
-    else:
-      stdin_dest = None
-    pipe = subprocess.Popen(
-        cmd,
-        executable='/bin/bash',
-        stdin=stdin_dest,
-        stdout=output_dest,
-        stderr=subprocess.STDOUT,
-        shell=True)
-    pid.append(pipe.pid)
     try:
       output = pipe.communicate(input=stdin_input)[0]
       if output is not None and len(output) > 0:
@@ -119,27 +118,17 @@ def RunOnce(cmd, timeout_time=None, return_output=True, stdin_input=None):
 
   t = threading.Thread(target=Run)
   t.start()
-
-  break_loop = False
-  while not break_loop:
-    if not t.isAlive():
-      break_loop = True
-
-    # Check the timeout
-    if (not break_loop and timeout_time is not None
-        and time.time() > start_time + timeout_time):
-      try:
-        os.kill(pid[0], signal.SIGKILL)
-      except OSError:
-        # process already dead. No action required.
-        pass
-
+  t.join(timeout_time)
+  if t.isAlive():
+    try:
+      pipe.kill()
+    except OSError:
+      # Can't kill a dead process.
+      pass
+    finally:
       logger.SilentLog("about to raise a timeout for: %s" % cmd)
       raise errors.WaitForResponseTimedOutError
-    if not break_loop:
-      time.sleep(0.1)
 
-  t.join()
   output = "".join(so)
   if _abort_on_error and error_occurred:
     raise errors.AbortError(msg=output)