[Cronet] Enforce implementation does not call through API classes

components/cronet/tools/api_static_checks.py

Index: components/cronet/tools/api_static_checks.py
diff --git a/components/cronet/tools/api_static_checks.py b/components/cronet/tools/api_static_checks.py
new file mode 100755
index 0000000000000000000000000000000000000000..6880b0d968e2892f420f94def6cc67deac4d87f2
--- /dev/null
+++ b/components/cronet/tools/api_static_checks.py
@@ -0,0 +1,179 @@
+#!/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 declare classes
+# and methods.  The first group returned by a match is the class or method name.
+CLASS_RE = re.compile(r'.*class ([^ ]*) .*\{')
+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 = [

[kapishnikov, 2016/12/02 20:05:03]

I think we should add the callee method signatures to the exception list in
order to detect the case when an overloaded method is added to the API.

[pauljensen, 2016/12/16 18:45:57]

Done.

+'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':
+      callee = line.split(' // ')[1].split('Method ')[1].split(':')[0]

[kapishnikov, 2016/12/02 20:05:03]

Can we merge two '//' & 'Method' splits into one split?

[pauljensen, 2016/12/16 18:45:57]

No, when calling through an interface it's "InterfaceMethod", otherwise it's
just "Method", so the text between "//" and "Method" isn't constant.  Having the
two splits()s better enforces that the output format isn't changing, i.e. it
will fail if either is removed.

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

Acknowledged.

+      callee_class = callee.split('.')[0]
+      assert callee_class
+      if callee_class in api_classes:
+        callee_method = callee.split('.')[1]
+        assert callee_method
+        # Ignore constructor calls for now as every implementation class
+        # that extends an API class will call them.
+        # TODO(pauljensen): Look into enforcing restricting constructor calls.

[kapishnikov, 2016/12/02 20:05:03]

Let's file a bug for that. I think it is important that we fix it before the
integration.

[pauljensen, 2016/12/16 18:45:57]

Done, and updated comment.

+        if callee_method == '"<init>"':
+          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 dirpath, _, filenames in os.walk(temp_dir):
+    if not filenames:
+      continue
+    package = dirpath[len(temp_dir + '/'):]
+    if package:
+      package += '/'
+    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
+  bad_api_calls = []
+  for dirpath, _, 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(dirpath, f) for f in filenames).replace(
+            '$', '\\$'),
+        dump_file)):
+      print 'ERROR: javap failed on ' + ' '.join(filenames)
+      return False
+    # Process class dump
+    with open(dump_file, 'r') as dump:
+      find_api_calls(dump, api_classes, bad_api_calls)
+
+  shutil.rmtree(temp_dir)
+
+  if bad_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(bad_api_calls)
+
+  if not bad_api_calls and opts.stamp:
+    build_utils.Touch(opts.stamp)
+  return not bad_api_calls
+
+
+if __name__ == '__main__':
+  sys.exit(0 if main(sys.argv[1:]) else -1)