Move bytecode_rewriter off compilation critical path

Changes it into an analysis step rather that runs at the same time as
errorprone.

Removes ability to output .jar files from bytecode_processor. It now
outputs a stamp file.

TBR=agrieve # Renamed build variable.

Bug: 1080670
Change-Id: If98eb4e3673ccdfa47c151370b3f98aa23488a15
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2210873
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Peter Wen <wnwen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771033}

build/android/bytecode/BUILD.gn

@@ -4,13 +4,10 @@
 
 import("//build/config/android/rules.gni")
 
-assert(current_toolchain == default_toolchain)
-
-java_binary("java_bytecode_rewriter") {
+java_binary("bytecode_processor") {
   sources = [
     "java/org/chromium/bytecode/ByteCodeProcessor.java",
     "java/org/chromium/bytecode/ClassPathValidator.java",
-    "java/org/chromium/bytecode/ThreadAssertionClassAdapter.java",
     "java/org/chromium/bytecode/TypeUtils.java",
   ]
   main_class = "org.chromium.bytecode.ByteCodeProcessor"
@@ -18,7 +15,7 @@ java_binary("java_bytecode_rewriter") {
     "//third_party/android_deps:org_ow2_asm_asm_java",
     "//third_party/android_deps:org_ow2_asm_asm_util_java",
   ]
-  wrapper_script_name = "helper/java_bytecode_rewriter"
+  wrapper_script_name = "helper/bytecode_processor"
   enable_bytecode_checks = false
   enable_desugar = false
 }

build/android/bytecode/java/org/chromium/bytecode/ByteCodeProcessor.java

@@ -5,40 +5,27 @@
 package org.chromium.bytecode;
 
 import org.objectweb.asm.ClassReader;
-import org.objectweb.asm.ClassVisitor;
-import org.objectweb.asm.ClassWriter;
 
 import java.io.BufferedInputStream;
-import java.io.BufferedOutputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.PrintStream;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLClassLoader;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.nio.file.Paths;
-import java.nio.file.StandardCopyOption;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
-import java.util.concurrent.Future;
-import java.util.zip.CRC32;
+import java.util.concurrent.TimeUnit;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipInputStream;
-import java.util.zip.ZipOutputStream;
 
 /**
  * Java application that takes in an input jar, performs a series of bytecode
@@ -46,154 +33,54 @@ import java.util.zip.ZipOutputStream;
  */
 class ByteCodeProcessor {
     private static final String CLASS_FILE_SUFFIX = ".class";
-    private static final String TEMPORARY_FILE_SUFFIX = ".temp";
     private static final int BUFFER_SIZE = 16384;
     private static boolean sVerbose;
     private static boolean sIsPrebuilt;
-    private static boolean sShouldUseThreadAnnotations;
-    private static boolean sShouldCheckClassPath;
     private static ClassLoader sDirectClassPathClassLoader;
     private static ClassLoader sFullClassPathClassLoader;
     private static Set<String> sFullClassPathJarPaths;
     private static Set<String> sMissingClassesAllowlist;
     private static ClassPathValidator sValidator;
 
-    private static class EntryDataPair {
-        private final ZipEntry mEntry;
-        private final byte[] mData;
-
-        private EntryDataPair(ZipEntry mEntry, byte[] mData) {
-            this.mEntry = mEntry;
-            this.mData = mData;
-        }
-
-        private static EntryDataPair create(String zipPath, byte[] data) {
-            ZipEntry entry = new ZipEntry(zipPath);
-            entry.setMethod(ZipEntry.STORED);
-            entry.setTime(0);
-            entry.setSize(data.length);
-            CRC32 crc = new CRC32();
-            crc.update(data);
-            entry.setCrc(crc.getValue());
-            return new EntryDataPair(entry, data);
-        }
-    }
-
-    private static EntryDataPair processEntry(ZipEntry entry, byte[] data)
-            throws ClassPathValidator.ClassNotLoadedException {
-        // Copy all non-.class files to the output jar.
-        if (entry.isDirectory() || !entry.getName().endsWith(CLASS_FILE_SUFFIX)) {
-            return new EntryDataPair(entry, data);
-        }
-
+    private static Void processEntry(ZipEntry entry, byte[] data) {
         ClassReader reader = new ClassReader(data);
-        if (sShouldCheckClassPath) {
-            sValidator.validateClassPathsAndOutput(reader, sDirectClassPathClassLoader,
-                    sFullClassPathClassLoader, sFullClassPathJarPaths, sIsPrebuilt, sVerbose,
-                    sMissingClassesAllowlist);
-        }
-
-        ClassWriter writer = new ClassWriter(reader, 0);
-        ClassVisitor chain = writer;
-        /* DEBUGGING:
-         To see objectweb.asm code that will generate bytecode for a given class:
-
-         java -cp
-         "third_party/android_deps/libs/org_ow2_asm_asm/asm-7.0.jar:third_party/android_deps/libs/org_ow2_asm_asm_util/asm-util-7.0.jar:out/Debug/lib.java/jar_containing_yourclass.jar"
-         org.objectweb.asm.util.ASMifier org.package.YourClassName
-
-         See this pdf for more details: https://asm.ow2.io/asm4-guide.pdf
-
-         To see the bytecode for a specific class, uncomment this code with your class name:
-
-        if (entry.getName().contains("YOUR_CLASS_NAME")) {
-          chain = new TraceClassVisitor(chain, new PrintWriter(System.out));
-        }
-        */
-        if (sShouldUseThreadAnnotations) {
-            chain = new ThreadAssertionClassAdapter(chain);
+        if (sIsPrebuilt) {
+            sValidator.validateFullClassPath(
+                    reader, sFullClassPathClassLoader, sMissingClassesAllowlist);
+        } else {
+            sValidator.validateDirectClassPath(reader, sDirectClassPathClassLoader,
+                    sFullClassPathClassLoader, sFullClassPathJarPaths, sMissingClassesAllowlist,
+                    sVerbose);
         }
-        reader.accept(chain, 0);
-        byte[] patchedByteCode = writer.toByteArray();
-        return EntryDataPair.create(entry.getName(), patchedByteCode);
+        return null;
     }
 
-    private static void process(String inputJarPath, String outputJarPath)
-            throws ClassPathValidator.ClassNotLoadedException, ExecutionException,
-                   InterruptedException {
-        String tempJarPath = outputJarPath + TEMPORARY_FILE_SUFFIX;
+    private static void process(String inputJarPath)
+            throws ExecutionException, InterruptedException {
         ExecutorService executorService =
                 Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors());
         try (ZipInputStream inputStream = new ZipInputStream(
-                     new BufferedInputStream(new FileInputStream(inputJarPath)));
-                ZipOutputStream tempStream = new ZipOutputStream(
-                        new BufferedOutputStream(new FileOutputStream(tempJarPath)))) {
-            List<Future<EntryDataPair>> list = new ArrayList<>();
+                     new BufferedInputStream(new FileInputStream(inputJarPath)))) {
             while (true) {
                 ZipEntry entry = inputStream.getNextEntry();
                 if (entry == null) {
                     break;
                 }
                 byte[] data = readAllBytes(inputStream);
-                list.add(executorService.submit(() -> processEntry(entry, data)));
+                executorService.submit(() -> processEntry(entry, data));
             }
             executorService.shutdown(); // This is essential in order to avoid waiting infinitely.
-            // Write the zip file entries in order to preserve determinism.
-            for (Future<EntryDataPair> futurePair : list) {
-                EntryDataPair pair = futurePair.get();
-                tempStream.putNextEntry(pair.mEntry);
-                tempStream.write(pair.mData);
-                tempStream.closeEntry();
-            }
+            executorService.awaitTermination(1, TimeUnit.HOURS);
         } catch (IOException e) {
             throw new RuntimeException(e);
         }
-        try {
-            Path src = Paths.get(tempJarPath);
-            Path dest = Paths.get(outputJarPath);
-            Files.move(src, dest, StandardCopyOption.REPLACE_EXISTING);
-        } catch (IOException ioException) {
-            throw new RuntimeException(ioException);
-        }
 
         if (sValidator.hasErrors()) {
-            System.err.println("Direct classpath is incomplete. To fix, add deps on the "
-                    + "GN target(s) that provide:");
-            for (Map.Entry<String, Map<String, Set<String>>> entry :
-                    sValidator.getErrors().entrySet()) {
-                printValidationError(System.err, entry.getKey(), entry.getValue());
-            }
+            sValidator.printAll();
             System.exit(1);
         }
     }
 
-    private static void printValidationError(
-            PrintStream out, String jarName, Map<String, Set<String>> missingClasses) {
-        out.print(" * ");
-        out.println(jarName);
-        int i = 0;
-        final int numErrorsPerJar = 2;
-        // The list of missing classes is non-exhaustive because each class that fails to validate
-        // reports only the first missing class.
-        for (Map.Entry<String, Set<String>> entry : missingClasses.entrySet()) {
-            String missingClass = entry.getKey();
-            Set<String> filesThatNeededIt = entry.getValue();
-            out.print("     * ");
-            if (i == numErrorsPerJar) {
-                out.print(String.format("And %d more...", missingClasses.size() - numErrorsPerJar));
-                break;
-            }
-            out.print(missingClass.replace('/', '.'));
-            out.print(" (needed by ");
-            out.print(filesThatNeededIt.iterator().next().replace('/', '.'));
-            if (filesThatNeededIt.size() > 1) {
-                out.print(String.format(" and %d more", filesThatNeededIt.size() - 1));
-            }
-            out.println(")");
-            i++;
-        }
-    }
-
     private static byte[] readAllBytes(InputStream inputStream) throws IOException {
         ByteArrayOutputStream buffer = new ByteArrayOutputStream();
         int numRead = 0;
@@ -236,11 +123,8 @@ class ByteCodeProcessor {
         // Invoke this script using //build/android/gyp/bytecode_processor.py
         int currIndex = 0;
         String inputJarPath = args[currIndex++];
-        String outputJarPath = args[currIndex++];
         sVerbose = args[currIndex++].equals("--verbose");
         sIsPrebuilt = args[currIndex++].equals("--is-prebuilt");
-        sShouldUseThreadAnnotations = args[currIndex++].equals("--enable-thread-annotations");
-        sShouldCheckClassPath = args[currIndex++].equals("--enable-check-class-path");
 
         sMissingClassesAllowlist = new HashSet<>();
         currIndex = parseListArgument(args, currIndex, sMissingClassesAllowlist);
@@ -267,6 +151,6 @@ class ByteCodeProcessor {
         sFullClassPathJarPaths.removeAll(directClassPathJarPaths);
 
         sValidator = new ClassPathValidator();
-        process(inputJarPath, outputJarPath);
+        process(inputJarPath);
     }
 }

build/android/bytecode/java/org/chromium/bytecode/ThreadAssertionClassAdapter.java

-// Copyright 2018 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.
-
-package org.chromium.bytecode;
-
-import static org.objectweb.asm.Opcodes.ASM7;
-import static org.objectweb.asm.Opcodes.INVOKESTATIC;
-
-import org.objectweb.asm.AnnotationVisitor;
-import org.objectweb.asm.ClassVisitor;
-import org.objectweb.asm.MethodVisitor;
-
-/**
- * A ClassVisitor which adds calls to
- * {@link org.chromium.base.ThreadUtils}'s assertOnUiThread/assertOnBackgroundThread when the
- * corresponding {@link androidx.annotation.UiThread} or
- * {@link androidx.annotation.WorkerThread} annotations are present. The function calls
- * are placed at the start of the method.
- */
-class ThreadAssertionClassAdapter extends ClassVisitor {
-    private static final String THREAD_UTILS_DESCRIPTOR = "org/chromium/base/ThreadUtils";
-    private static final String THREAD_UTILS_SIGNATURE = "()V";
-    private static final String UI_THREAD_ANNOTATION_DESCRIPTOR =
-            "Landroid/support/annotation/UiThread;";
-    private static final String WORKER_THREAD_ANNOTATION_DESCRIPTOR =
-            "Landroid/support/annotation/WorkerThread;";
-
-    ThreadAssertionClassAdapter(ClassVisitor visitor) {
-        super(ASM7, visitor);
-    }
-
-    @Override
-    public MethodVisitor visitMethod(final int access, final String name, String desc,
-            String signature, String[] exceptions) {
-        return new AddAssertMethodVisitor(
-                super.visitMethod(access, name, desc, signature, exceptions));
-    }
-
-    private static class AddAssertMethodVisitor extends MethodVisitor {
-        String mAssertMethodName = "";
-
-        AddAssertMethodVisitor(MethodVisitor mv) {
-            super(ASM7, mv);
-        }
-
-        /**
-         * Call for annotations on the method. Checks if the annotation is @UiThread
-         * or @WorkerThread, and if so will set the mAssertMethodName property to the name of the
-         * method to call in order to assert that a method is running on the intented thread.
-         *
-         * @param descriptor Annotation descriptor containing its name and package.
-         */
-        @Override
-        public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) {
-            switch (descriptor) {
-                case UI_THREAD_ANNOTATION_DESCRIPTOR:
-                    mAssertMethodName = "assertOnUiThread";
-                    break;
-                case WORKER_THREAD_ANNOTATION_DESCRIPTOR:
-                    mAssertMethodName = "assertOnBackgroundThread";
-                    break;
-                default:
-                    break;
-            }
-
-            return super.visitAnnotation(descriptor, visible);
-        }
-
-        /**
-         * Called to start visiting code. Will also insert the assertOnXThread methods at the start
-         * of the method if needed.
-         */
-        @Override
-        public void visitCode() {
-            super.visitCode();
-            if (!mAssertMethodName.equals("")) {
-                visitMethodInsn(INVOKESTATIC, THREAD_UTILS_DESCRIPTOR, mAssertMethodName,
-                        THREAD_UTILS_SIGNATURE, false);
-            }
-        }
-    }
-}
\ No newline at end of file

build/android/docs/java_toolchain.md

@@ -37,11 +37,6 @@ What are interface jars?:
   removed.
 * Dependant targets use interface `.jar` files to skip having to be rebuilt
   when only private implementation details change.
-  * To accomplish this behavior, library targets list only their
-    interface `.jar` files as outputs. Ninja's `restat=1` feature then causes
-    dependent targets to be rebuilt only when the interface `.jar` changes.
-    Final dex targets are always rebuilt because they depend on the
-    non-interface `.jar` through a `depfile`.
 
 [//third_party/ijar]: /third_party/ijar/README.chromium
 [//third_party/turbine]: /third_party/turbine/README.chromium
@@ -77,13 +72,7 @@ This step can be disabled via GN arg: `use_errorprone_java_compiler = false`
 [ErrorProne]: https://errorprone.info/
 [ep_plugins]: /tools/android/errorprone_plugin/
 
-### Step 3: Bytecode Processing
-
-* `//build/android/bytecode` runs on the compiled `.jar` in order to:
-  * Enable Java assertions (when dcheck is enabled).
-  * Assert that libraries have properly declared `deps`.
-
-### Step 4: Desugaring
+### Step 3: Desugaring
 
 This step happens only when targets have `supports_android = true`.
 
@@ -91,7 +80,7 @@ This step happens only when targets have `supports_android = true`.
   lambdas and default interface methods, into constructs that are compatible
   with Java 7.
 
-### Step 5: Filtering
+### Step 4: Filtering
 
 This step happens only when targets that have `jar_excluded_patterns` or
 `jar_included_patterns` set (e.g. all `android_` targets).
@@ -108,7 +97,7 @@ This step happens only when targets that have `jar_excluded_patterns` or
 [Android Resources]: life_of_a_resource.md
 [apphooks]: /chrome/android/java/src/org/chromium/chrome/browser/AppHooksImpl.java
 
-### Step 6: Instrumentation
+### Step 5: Instrumentation
 
 This step happens only when this GN arg is set: `use_jacoco_coverage = true`
 
@@ -116,14 +105,14 @@ This step happens only when this GN arg is set: `use_jacoco_coverage = true`
 
 [Jacoco]: https://www.eclemma.org/jacoco/
 
-### Step 7: Copy to lib.java
+### Step 6: Copy to lib.java
 
 * The `.jar` is copied into `$root_build_dir/lib.java` (under target-specific
   subdirectories) so that it will be included by bot archive steps.
   * These `.jar` files are the ones used when running `java_binary` and
     `junit_binary` targets.
 
-### Step 8: Per-Library Dexing
+### Step 7: Per-Library Dexing
 
 This step happens only when targets have `supports_android = true`.
 
@@ -139,7 +128,7 @@ This step happens only when targets have `supports_android = true`.
 [d8]: https://developer.android.com/studio/command-line/d8
 [incremental install]: /build/android/incremental_install/README.md
 
-### Step 9: Apk / Bundle Module Compile
+### Step 8: Apk / Bundle Module Compile
 
 * Each `android_apk` and `android_bundle_module` template has a nested
   `java_library` target. The nested library includes final copies of files
@@ -150,7 +139,7 @@ This step happens only when targets have `supports_android = true`.
 
 [JNI glue]: /base/android/jni_generator/README.md
 
-### Step 10: Final Dexing
+### Step 9: Final Dexing
 
 This step is skipped when building using [Incremental Install].
 
@@ -166,7 +155,7 @@ When `is_java_debug = false`:
 [Incremental Install]: /build/android/incremental_install/README.md
 [R8]: https://r8.googlesource.com/r8
 
-### Step 11: Bundle Module Dex Splitting
+### Step 10: Bundle Module Dex Splitting
 
 This step happens only when `is_java_debug = false`.
 
@@ -266,8 +255,7 @@ We use several tools for static analysis.
 
 [lint_plugins]: http://tools.android.com/tips/lint-custom-rules
 
-### [Bytecode Rewriter](/build/android/bytecode/)
-* Runs as part of normal compilation.
+### [Bytecode Processor](/build/android/bytecode/)
 * Performs a single check:
   * That target `deps` are not missing any entries.
   * In other words: Enforces that targets do not rely on indirect dependencies

build/android/gyp/bytecode_processor.py

@@ -3,7 +3,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-"""Wraps bin/helper/java_bytecode_rewriter and expands @FileArgs."""
+"""Wraps bin/helper/bytecode_processor and expands @FileArgs."""
 
 import argparse
 import os
@@ -24,38 +24,37 @@ def main(argv):
   parser.add_argument('--script', required=True,
                       help='Path to the java binary wrapper script.')
   parser.add_argument('--input-jar', required=True)
-  parser.add_argument('--output-jar', required=True)
-  parser.add_argument('--direct-classpath-jars', required=True)
+  parser.add_argument('--direct-classpath-jars')
   parser.add_argument('--sdk-classpath-jars')
-  parser.add_argument('--extra-classpath-jars', dest='extra_jars',
-                      action='append', default=[],
-                      help='Extra inputs, passed last to the binary script.')
+  parser.add_argument('--full-classpath-jars', action='append')
+  parser.add_argument('--stamp')
   parser.add_argument('-v', '--verbose', action='store_true')
   parser.add_argument('--missing-classes-allowlist')
   _AddSwitch(parser, '--is-prebuilt')
-  _AddSwitch(parser, '--enable-thread-annotations')
-  _AddSwitch(parser, '--enable-check-class-path')
   args = parser.parse_args(argv)
 
-  sdk_jars = build_utils.ParseGnList(args.sdk_classpath_jars)
-  direct_jars = build_utils.ParseGnList(args.direct_classpath_jars)
-  extra_classpath_jars = build_utils.ParseGnList(args.extra_jars)
+  args.sdk_classpath_jars = build_utils.ParseGnList(args.sdk_classpath_jars)
+  args.direct_classpath_jars = build_utils.ParseGnList(
+      args.direct_classpath_jars)
+  args.full_classpath_jars = build_utils.ParseGnList(args.full_classpath_jars)
   args.missing_classes_allowlist = build_utils.ParseGnList(
       args.missing_classes_allowlist)
 
-  if args.verbose:
-    verbose = '--verbose'
-  else:
-    verbose = '--not-verbose'
-
-  cmd = ([
-      args.script, args.input_jar, args.output_jar, verbose, args.is_prebuilt,
-      args.enable_thread_annotations, args.enable_check_class_path
-  ] + [str(len(args.missing_classes_allowlist))] +
-         args.missing_classes_allowlist + [str(len(sdk_jars))] + sdk_jars +
-         [str(len(direct_jars))] + direct_jars + extra_classpath_jars)
+  verbose = '--verbose' if args.verbose else '--not-verbose'
+
+  cmd = [args.script, args.input_jar, verbose, args.is_prebuilt]
+  cmd += [str(len(args.missing_classes_allowlist))]
+  cmd += args.missing_classes_allowlist
+  cmd += [str(len(args.sdk_classpath_jars))]
+  cmd += args.sdk_classpath_jars
+  cmd += [str(len(args.direct_classpath_jars))]
+  cmd += args.direct_classpath_jars
+  cmd += args.full_classpath_jars
   subprocess.check_call(cmd)
 
+  if args.stamp:
+    build_utils.Touch(args.stamp)
+
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv))

build/config/android/rules.gni

@@ -4290,11 +4290,10 @@ if (enable_java_templates) {
 
     _java_library_vars = [
       "enable_bytecode_checks",
-      "enable_bytecode_rewriter",
-      "missing_classes_allowlist",
       "enable_jetify",
       "jar_excluded_patterns",
       "jar_included_patterns",
+      "missing_classes_allowlist",
       "requires_android",
       "testonly",
     ]

components/cronet/android/BUILD.gn

@@ -731,7 +731,7 @@ android_java_prebuilt("repackage_native_java") {
 
   # Since only the unprocessed jar is used, no need to complete the bytecode
   # processing steps.
-  enable_bytecode_rewriter = false
+  enable_bytecode_checks = false
   deps = [ ":repackage_native_impl" ]
 }
 

tools/android/errorprone_plugin/BUILD.gn

@@ -19,7 +19,7 @@ java_library("errorprone_plugin_java") {
 
   # Necessary to avoid dependency cycle
   enable_errorprone = false
-  enable_bytecode_rewriter = false
+  enable_bytecode_checks = false
 
   # So that we don't need to inject jacoco runtime into the compiler's classpath.
   jacoco_never_instrument = true