[Findit] Fix a bunch of bugs.

1. Add retry to http_client.
2. Make Findit more robust for unrecognized chromium revision.
3. Workaround a harmless bug in python 2.7
4. Give a better error message for check failure.

NOTRY=true

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

Cr-Commit-Position: refs/heads/master@{#293528}

6 files changed, 114 insertions, 32 deletions

diff --git a/tools/findit/OWNERS b/tools/findit/OWNERS
index 785b310..947f12e 100644
--- a/tools/findit/OWNERS
+++ b/tools/findit/OWNERS
@@ -1,2 +1 @@
 stgao@chromium.org
-jeun@chromium.org
\ No newline at end of file
diff --git a/tools/findit/chromium_deps.py b/tools/findit/chromium_deps.py
index e512688..cf8aa26 100644
--- a/tools/findit/chromium_deps.py
+++ b/tools/findit/chromium_deps.py
@@ -68,31 +68,25 @@ def _GetComponentName(path, host_dirs):
   return '_'.join(path.split('/'))
 
 
-def _GetContentOfDEPS(revision, retries=5, sleep_time=0.1):
+def _GetContentOfDEPS(revision):
   chromium_git_file_url_template = CONFIG['chromium_git_file_url']
 
-  deps_file_name = '.DEPS.git'
-  count = 0
-  while True:
-    count += 1
+  # Try .DEPS.git first, because before migration from SVN to GIT, the .DEPS.git
+  # has the dependency in GIT repo while DEPS has dependency in SVN repo.
+  url = chromium_git_file_url_template % (revision, '.DEPS.git')
+  http_status_code, content = utils.GetHttpClient().Get(
+      url, retries=5, retry_if_not=404)
 
-    url = chromium_git_file_url_template % (revision, deps_file_name)
-    http_status_code, content = utils.GetHttpClient().Get(url, timeout=60)
+  # If .DEPS.git is not found, use DEPS, assuming it is a commit after migration
+  # from SVN to GIT.
+  if http_status_code == 404:
+    url = chromium_git_file_url_template % (revision, 'DEPS')
+    http_status_code, content = utils.GetHttpClient().Get(url, retries=5)
 
-    if http_status_code == 404 and deps_file_name != 'DEPS':
-      deps_file_name = 'DEPS'
-      count = 0
-      continue
-    elif http_status_code == 200:
-      # Googlesource git returns text file encoded in base64, so decode it.
-      return base64.b64decode(content)
-
-    if count < retries:
-      time.sleep(sleep_time)
-    else:
-      break
-
-  return ''
+  if http_status_code == 200:
+    return base64.b64decode(content)
+  else:
+    return ''
 
 
 def GetChromiumComponents(chromium_revision,
@@ -111,6 +105,7 @@ def GetChromiumComponents(chromium_revision,
   Returns:
     A map from component path to parsed component name, repository URL,
     repository type and revision.
+    Return None if an error occurs.
   """
   if os_platform.lower() == 'linux':
     os_platform = 'unix'
@@ -121,15 +116,30 @@ def GetChromiumComponents(chromium_revision,
     # Convert svn revision or commit position to Git hash.
     cr_rev_url_template = CONFIG['cr_rev_url']
     url = cr_rev_url_template % chromium_revision
-    # TODO(stgao): Add retry in HttpClient.
-    _, content = utils.GetHttpClient().Get(url, timeout=60)
+    status_code, content = utils.GetHttpClient().Get(
+        url, timeout=60, retries=5, retry_if_not=404)
+    if status_code != 200 or not content:
+      if status_code == 404:
+        print 'Chromium commit position %s is not found.' % chromium_revision
+      return None
+
     cr_rev_data = json.loads(content)
     if 'git_sha' not in cr_rev_data:
-      raise Exception('Failed to convert svn revision to git hash')
-    chromium_revision = cr_rev_data['git_sha']
+      return None
+
+    if 'repo' not in cr_rev_data or cr_rev_data['repo'] != 'chromium/src':
+      print ('%s seems like a commit position of "%s", but not "chromium/src".'
+             % (chromium_revision, cr_rev_data['repo']))
+      return None
+
+    chromium_revision = cr_rev_data.get('git_sha')
+    if not chromium_revision:
+      return None
 
   # Download the content of DEPS file in chromium.
   deps_content = deps_file_downloader(chromium_revision)
+  if not deps_content:
+    return None
 
   all_deps = {}
 
@@ -196,12 +206,17 @@ def GetChromiumComponentRange(old_revision,
 
   Returns:
     A map from component path to its parsed regression and other information.
+    Return None if an error occurs.
   """
-  # Assume first revision is the old revision.
   old_components = GetChromiumComponents(old_revision, os_platform,
                                          deps_file_downloader)
+  if not old_components:
+    return None
+
   new_components = GetChromiumComponents(new_revision, os_platform,
                                          deps_file_downloader)
+  if not new_components:
+    return None
 
   components = {}
   for path in new_components:
diff --git a/tools/findit/common/http_client.py b/tools/findit/common/http_client.py
index 82c358de..75dd410 100644
--- a/tools/findit/common/http_client.py
+++ b/tools/findit/common/http_client.py
@@ -10,9 +10,19 @@ class HttpClient(object):
   """
 
   @staticmethod
-  def Get(url, params={}, timeout=None):
+  def Get(url, params={}, timeout=60, retries=5, retry_interval=0.5,
+          retry_if_not=None):
     """Send a GET request to the given url with the given parameters.
 
+    Args:
+      url: the url to send request to.
+      params: parameters to send as part of the http request.
+      timeout: timeout for the http request, default is 60 seconds.
+      retries: indicate how many retries before failing, default is 5.
+      retry_interval: interval in second to wait before retry, default is 0.5.
+      retry_if_not: a http status code. If set, retry only when the failed http
+                    status code is a different value.
+
     Returns:
       (status_code, data)
       state_code: the http status code in the response.
diff --git a/tools/findit/common/http_client_local.py b/tools/findit/common/http_client_local.py
index fccb0b6..8907bc0 100644
--- a/tools/findit/common/http_client_local.py
+++ b/tools/findit/common/http_client_local.py
@@ -21,6 +21,7 @@ import os
 import re
 import socket
 import ssl
+import time
 import urllib
 import urllib2
 
@@ -225,7 +226,25 @@ class HttpClientLocal(http_client.HttpClient):
   """This http client is used locally in a workstation, GCE VMs, etc."""
 
   @staticmethod
-  def Get(url, params={}, timeout=None):
+  def Get(url, params={}, timeout=60, retries=5, retry_interval=0.5,
+          retry_if_not=None):
     if params:
       url = '%s?%s' % (url, urllib.urlencode(params))
-    return _SendRequest(url, timeout=timeout)
+
+    count = 0
+    while True:
+      count += 1
+
+      status_code, content = _SendRequest(url, timeout=timeout)
+      if status_code == 200:
+        return status_code, content
+      if retry_if_not and status_code == retry_if_not:
+        return status_code, content
+
+      if count < retries:
+        time.sleep(retry_interval)
+      else:
+        return status_code, content
+
+    # Should never be reached.
+    return status_code, content
diff --git a/tools/findit/crash_utils.py b/tools/findit/crash_utils.py
index 6be058b..d4baee5 100644
--- a/tools/findit/crash_utils.py
+++ b/tools/findit/crash_utils.py
@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import atexit
 import cgi
 import ConfigParser
 import json
@@ -20,10 +21,37 @@ MAX_THREAD_NUMBER = 10
 TASK_QUEUE = None
 
 
+def SignalWorkerThreads():
+  global TASK_QUEUE
+  if not TASK_QUEUE:
+    return
+
+  for i in range(MAX_THREAD_NUMBER):
+    TASK_QUEUE.put(None)
+
+  # Give worker threads a chance to exit.
+  # Workaround the harmless bug in python 2.7 below.
+  time.sleep(1)
+
+
+atexit.register(SignalWorkerThreads)
+
+
 def Worker():
   global TASK_QUEUE
   while True:
-    function, args, kwargs, result_semaphore = TASK_QUEUE.get()
+    try:
+      task = TASK_QUEUE.get()
+      if not task:
+        return
+    except TypeError:
+      # According to http://bugs.python.org/issue14623, this is a harmless bug
+      # in python 2.7 which won't be fixed.
+      # The exception is raised on daemon threads when python interpreter is
+      # shutting down.
+      return
+
+    function, args, kwargs, result_semaphore = task
     try:
       function(*args, **kwargs)
     except:
diff --git a/tools/findit/findit_for_clusterfuzz.py b/tools/findit/findit_for_clusterfuzz.py
index 219947f..c109456 100644
--- a/tools/findit/findit_for_clusterfuzz.py
+++ b/tools/findit/findit_for_clusterfuzz.py
@@ -2,6 +2,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import sys
+
 import chromium_deps
 from common import utils
 import crash_utils
@@ -108,11 +110,20 @@ def FindCulpritCLs(stacktrace_string,
     if chrome_regression_start != '0':
       component_to_regression_dict = chromium_deps.GetChromiumComponentRange(
           chrome_regression_start, chrome_regression_end)
+      if not component_to_regression_dict:
+        print ('Failed to get component regression ranges for chromium '
+               'regression range %s:%s'
+               % (chrome_regression_start, chrome_regression_end))
+        sys.exit(1)
 
   # Parse crash revision.
   if chrome_crash_revision:
     component_to_crash_revision_dict = chromium_deps.GetChromiumComponents(
         chrome_crash_revision)
+    if not component_to_crash_revision_dict:
+      print ('Failed to get component dependencies for chromium revision "%s".'
+             % chrome_crash_revision)
+      sys.exit(1)
 
   # Check if component regression information is available.
   component_regression = crash_utils.SplitRange(component_regression)
@@ -207,7 +218,7 @@ def FindCulpritCLs(stacktrace_string,
     if 'mac_' in build_type:
       return ('No line information available in stacktrace.', [])
 
-    return ('Stacktrace is malformed.', [])
+    return ('Findit failed to find any stack trace. Is it in a new format?', [])
 
   # Run the algorithm on the parsed stacktrace, and return the result.
   stacktrace_list = [parsed_release_build_stacktrace,