[Cronet] Enforce implementation does not call through API classes

components/cronet/tools/api_static_checks.py

+#!/usr/bin/python
+# Copyright 2016 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.
+
+"""api_static_checks.py - Check Cronet implementation does not call through
+API classes.
+"""
+
+import argparse
+import os
+import re
+import shutil
+import sys
+import tempfile
+
+REPOSITORY_ROOT = os.path.abspath(os.path.join(
+    os.path.dirname(__file__), '..', '..', '..'))
+
+sys.path.append(os.path.join(REPOSITORY_ROOT, 'build/android/gyp/util'))
+import build_utils
+
+# These regular expressions catch the beginning of lines that declar classes and

[kapishnikov, 2016/12/01 20:34:17]

declar -> declare

[pauljensen, 2016/12/02 18:24:14]

Done.

+# methods.  The first group returned by a match is the class or method name.
+CLASS_RE = re.compile(r'.*class ([^ ]*) .*{')

[kapishnikov, 2016/12/01 20:34:16]

I think '\' is missing before '{' since we want to literally match one '{'
character. Otherwise the regular expression is not completely valid.

[pauljensen, 2016/12/02 18:24:14]

Done.

+METHOD_RE = re.compile(r'.* ([^ ]*)\(.*\);')
+
+# Allowed exceptions.  Adding anything to this list is dangerous and should be
+# avoided if possible.  For now these exceptions are for APIs that existed in
+# the first version of Cronet and will be supported forever.
+# TODO(pauljensen): Remove these.
+ALLOWED_EXCEPTIONS = [
+'org.chromium.net.impl.CronetEngineBuilderImpl/build ->'
+    ' org/chromium/net/ExperimentalCronetEngine/getVersionString',
+'org.chromium.net.urlconnection.CronetFixedModeOutputStream$UploadDataProviderI'
+    'mpl/read -> org/chromium/net/UploadDataSink/onReadSucceeded',
+'org.chromium.net.urlconnection.CronetFixedModeOutputStream$UploadDataProviderI'
+    'mpl/rewind -> org/chromium/net/UploadDataSink/onRewindError',
+'org.chromium.net.urlconnection.CronetHttpURLConnection/disconnect ->'
+    ' org/chromium/net/UrlRequest/cancel',
+'org.chromium.net.urlconnection.CronetHttpURLConnection/disconnect ->'
+    ' org/chromium/net/UrlResponseInfo/getHttpStatusText',
+'org.chromium.net.urlconnection.CronetHttpURLConnection/disconnect ->'
+    ' org/chromium/net/UrlResponseInfo/getHttpStatusCode',
+'org.chromium.net.urlconnection.CronetHttpURLConnection/getHeaderField ->'
+    ' org/chromium/net/UrlResponseInfo/getHttpStatusCode',
+'org.chromium.net.urlconnection.CronetHttpURLConnection/getErrorStream ->'
+    ' org/chromium/net/UrlResponseInfo/getHttpStatusCode',
+'org.chromium.net.urlconnection.CronetHttpURLConnection/setConnectTimeout ->'
+    ' org/chromium/net/UrlRequest/read',
+'org.chromium.net.urlconnection.CronetHttpURLConnection$CronetUrlRequestCallbac'
+    'k/onRedirectReceived -> org/chromium/net/UrlRequest/followRedirect',
+'org.chromium.net.urlconnection.CronetHttpURLConnection$CronetUrlRequestCallbac'
+    'k/onRedirectReceived -> org/chromium/net/UrlRequest/cancel',
+'org.chromium.net.urlconnection.CronetChunkedOutputStream$UploadDataProviderImp'
+    'l/read -> org/chromium/net/UploadDataSink/onReadSucceeded',
+'org.chromium.net.urlconnection.CronetChunkedOutputStream$UploadDataProviderImp'
+    'l/rewind -> org/chromium/net/UploadDataSink/onRewindError',
+'org.chromium.net.urlconnection.CronetBufferedOutputStream$UploadDataProviderIm'
+    'pl/read -> org/chromium/net/UploadDataSink/onReadSucceeded',
+'org.chromium.net.urlconnection.CronetBufferedOutputStream$UploadDataProviderIm'
+    'pl/rewind -> org/chromium/net/UploadDataSink/onRewindSucceeded',
+]
+
+
+def find_api_calls(dump, api_classes, bad_calls):
+  # Given a dump of an implementation class, find calls through API classes.
+  # |dump| is the output of "javap -c" on the implementation class files.
+  # |api_classes| is the list of classes comprising the API.
+  # |bad_calls| is the list of calls through API classes.  This list is built up
+  #             by this function.
+
+  for line in dump:
+    if CLASS_RE.match(line):
+      caller_class = CLASS_RE.match(line).group(1)
+    if METHOD_RE.match(line):
+      caller_method = METHOD_RE.match(line).group(1)
+    if line[8:16] == ': invoke':

[kapishnikov, 2016/12/01 20:34:17]

This looks a little fragile. Can the output format change in the future? Should
we have a test to detect such change?

[pauljensen, 2016/12/02 18:24:14]

I doubt the format will ever change; for example "man javap" shows this output
format.  I've added some tests that it keeps working.

[kapishnikov, 2016/12/28 21:54:41]

Acknowledged.

+      callee = line[54:].split(':')[0]

[kapishnikov, 2016/12/01 20:34:16]

1. Can we add an assertion that |caller_class| and |caller_method| are not empty
at this point?

2. Would it be more safe to get substring between "// Method " and ":"? '54'
looks a little fragile.

[pauljensen, 2016/12/02 18:24:14]

Done.

+      callee_class = callee.split('.')[0]
+      if callee_class in api_classes:
+        callee_method = callee.split('.')[1]
+        # Ignore default constructor calls
+        if callee_method == '"<init>"':

[kapishnikov, 2016/12/01 20:34:17]

Should we also track the usage of constructors, i.e. if we construct an API
class from impl, should we do it through the wrappers? E.g., what will happen if
we add a non-default constructor to an API class?

Btw, are we constructing any API classes from the impl?

[pauljensen, 2016/12/02 18:24:14]

We construct plenty of API classes from the impl.  We "extend" many API classes,
and those constructors call the "super" constructors.
We might want to track the constructor usage, but that is more complicated. 
I've added a TODO.

[kapishnikov, 2016/12/28 21:54:41]

Acknowledged.

+          continue
+        # Ignore VersionSafe calls
+        if 'VersionSafeCallbacks' in caller_class:
+          continue
+        bad_call = '%s/%s -> %s/%s' % (caller_class, caller_method,
+                                       callee_class, callee_method)
+        if bad_call in ALLOWED_EXCEPTIONS:
+          continue
+        bad_calls += [bad_call]
+
+
+def main(args):
+  # Returns True if no calls through API classes in implementation.
+
+  parser = argparse.ArgumentParser(
+      description='Check modules do not contain ARM Neon instructions.')
+  parser.add_argument('--api_jar',
+                      help='Path to API jar (i.e. cronet_api.jar)',
+                      required=True,
+                      metavar='path/to/cronet_api.jar')
+  parser.add_argument('--impl_jar',
+                      help='Path to implementation jar '
+                          '(i.e. cronet_impl_native_java.jar)',
+                      required=True,
+                      metavar='path/to/cronet_impl_native_java.jar',
+                      action='append')
+  parser.add_argument('--stamp', help='Path to touch on success.')
+  opts = parser.parse_args(args)
+
+  temp_dir = tempfile.mkdtemp()
+
+  # Extract API class files from jar
+  jar_cmd = ['jar', 'xf', os.path.abspath(opts.api_jar)]
+  build_utils.CheckOutput(jar_cmd, cwd=temp_dir)
+  shutil.rmtree(os.path.join(temp_dir, 'META-INF'))
+
+  # Collect names of API classes
+  api_classes = []
+  for root, _, filenames in os.walk(temp_dir):

[kapishnikov, 2016/12/01 20:34:17]

Should |root| be renamed to |dirpath|? I guess |temp_dir| is the root.

[pauljensen, 2016/12/02 18:24:14]

Done.

+    if not filenames:
+      continue
+    package = root[len(temp_dir + '/'):]
+    for filename in filenames:
+      if filename.endswith('.class'):
+        classname = filename[:-len('.class')]
+        api_classes += [package + '/' + classname]
+
+  shutil.rmtree(temp_dir)
+  temp_dir = tempfile.mkdtemp()
+
+  # Extract impl class files from jars
+  for impl_jar in opts.impl_jar:
+    jar_cmd = ['jar', 'xf', os.path.abspath(impl_jar)]
+    build_utils.CheckOutput(jar_cmd, cwd=temp_dir)
+  shutil.rmtree(os.path.join(temp_dir, 'META-INF'))
+
+  # Process classes
+  api_calls = []

[kapishnikov, 2016/12/01 20:34:16]

Should we rename it to |bad_api_calls|?

[pauljensen, 2016/12/02 18:24:14]

Done.

+  for root, _, filenames in os.walk(temp_dir):
+    if not filenames:
+      continue
+    # Dump classes
+    dump_file = os.path.join(temp_dir, 'dump.txt')
+    if os.system('javap -c %s > %s' % (
+        ' '.join(os.path.join(root, f) for f in filenames).replace('$', '\\$'),
+        dump_file)):
+      print 'ERROR: javap failed on ' + filenames

[kapishnikov, 2016/12/01 20:34:16]

I think it is not allowed to concatenate string with a list. Suggestion:
print 'ERROR: javap failed on ' + ' '.join(filenames)

[pauljensen, 2016/12/02 18:24:14]

Done.

+      return False
+    # Process class dump
+    with open(dump_file, 'r') as dump:
+      find_api_calls(dump, api_classes, api_calls)
+
+  shutil.rmtree(temp_dir)
+
+  if api_calls:
+    print 'ERROR: Found the following calls from implementation classes through'
+    print '       API classes.  These could fail if older API is used that'
+    print '       does not contain newer methods.  Please call through a'
+    print '       wrapper class from VersionSafeCallbacks.'
+    print '\n'.join(api_calls)
+
+  if not api_calls and opts.stamp:
+    build_utils.Touch(opts.stamp)
+  return not api_calls
+
+
+if __name__ == '__main__':
+  sys.exit(0 if main(sys.argv[1:]) != 0 else -1)
+