944553 - Automatically detect missing ProGuard-related annotations at compile time

Status: NEW

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Richard Newman [:rnewman]]

Forking Part 6 of Bug 709230.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Chris, I updated the commit message for this. Note that it doesn't apply cleanly on fx-team yet.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Detect missing Proguard annotations at compile-time.

This one applies cleanly to fx-team and appears to work. Seems like review time.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8341360 [details] [diff] [review]
Detect missing Proguard annotations at compile-time.

Review of attachment 8341360 [details] [diff] [review]:
-----------------------------------------------------------------

Most of this looks fine.

I have misgivings about invoking shell commands from Java. It seems like an altogether smarter way to do this is to move that logic into a standalone shell script (meh, and you have to solve all of the portability problems) or Python script (good), that can follow build conventions (e.g., use of environment variables as necessary).

Use Preprocessor.py as a template for this -- it is a text-processing Python script that's invoked on a set of input files from Make.

Invoke that script in a build phase, and simply read the output from Java. If you invoke this correctly with Make, you can also avoid doing all that file processing if the C++ code hasn't changed.

::: build/annotationProcessors/AnnotationProcessor.java
@@ +23,5 @@
>      public static final String HEADERFILE = "GeneratedJNIWrappers.h";
>  
> +    // Used to track elements which have yet to be encountered by both the entry point scanner and
> +    // the annotation processor.
> +    private static final ElementEliminatingPairedSet<MemberReference> mPairingSet = new ElementEliminatingPairedSet<MemberReference>();

sPairingSet, surely?

@@ +150,5 @@
> +        if (mPairingSet.isEmpty()) {
> +            System.exit(0);
> +        }
> +
> +        System.out.println("ERROR: JNI Entry points unprotected from Proguard detected!");

Does this successfully cause the build to fail?

@@ +206,5 @@
> +     *
> +     * @param m Standardised reference to the entry point.
> +     */
> +    public static void hitEntryPoint(MemberReference m) {
> +        //System.out.println("Hitting entry point: "+m);

Kill this.

::: build/annotationProcessors/classloader/MemberReference.java
@@ +22,5 @@
> +    public MemberReference(ENTITY_TYPE entityType, String name, String type, String className) {
> +        if (entityType == ENTITY_TYPE.METHOD && name.equals("<init>")) {
> +            entityType = ENTITY_TYPE.CONSTRUCTOR;
> +        }
> +        name = name.replace('$', '.');

mName = ...

@@ +23,5 @@
> +        if (entityType == ENTITY_TYPE.METHOD && name.equals("<init>")) {
> +            entityType = ENTITY_TYPE.CONSTRUCTOR;
> +        }
> +        name = name.replace('$', '.');
> +        className = className.replace('$', '.');

mClassName = ...

@@ +31,5 @@
> +        mClassName = className;
> +    }
> +
> +    public MemberReference(String name) {
> +        name = name.replace('$', '.');

mName =

@@ +42,5 @@
> +        mName = member.getName().replace('$', '.');
> +
> +        mClassName = member.getDeclaringClass().getCanonicalName().replace('$', '.');
> +
> +        if (member instanceof Field) {

public MemberReference(ENTITY_TYPE entityType, Member member, String name) {
    this(entityType,
         name,
         Utils.getTypeSignatureStringForMember(member),
         member.getDeclaringClass().getCanonicalName().replace('$', '.'));
}

public MemberReference(ENTITY_TYPE entityType, Member member) {
    this(entityType, member, member.getName());
}

public MemberReference(Field member) {
    this(ENTITY_TYPE.FIELD, member);
}
public MemberReference(Method member) {
    this(ENTITY_TYPE.METHOD, member);
}
public MemberReference(Constructor member) {
    this(ENTITY_TYPE.CONSTRUCTOR, member, "<init>");
}

@@ +91,5 @@
> +            return false;
> +        }
> +        if (cast.mType != null && mType == null) {
> +            return false;
> +        }

The last three clauses can be simplified to

if (cast.mType == null) {
    return mType == null;
}

return cast.mType.equals(mType);

@@ +101,5 @@
> +        return true;
> +    }
> +
> +    public String toString() {
> +        return mClassName+":"+mEntityType+":"+mName+":"+mType;

Spaces.

::: build/annotationProcessors/entrypointscanner/ElementEliminatingPairedSet.java
@@ +37,5 @@
> +        }
> +    }
> +
> +    private String[] getStringSet(HashSet<T> set) {
> +

static, and lose the newline.

::: build/annotationProcessors/entrypointscanner/EntryPointScanner.java
@@ +60,5 @@
> +     * and we can abort the compile process with an error.
> +     *
> +     * An explanation of the bash/sed script follows.
> +     *
> +     * The input to the pipe is a find command yeilding the filenames of all cpp files found in the

yielding.

@@ +75,5 @@
> +     * https://pastebin.mozilla.org/3224962
> +     */
> +    private static final String COMMAND =
> +        "find . -type f -name \\*.cpp | " +
> +        "xargs sed -rn 's/.*(getClassGlobalRef|FindClass)\\(\\w*\"([^\"]*)\"\\w*\\).*/\\2/;ta;\n" +

This is a non-portable sed invocation.

There's no -r on Mac OS.

@@ +99,5 @@
> +        long s = System.currentTimeMillis();
> +
> +        try {
> +            final Runtime rt = Runtime.getRuntime();
> +            final String[] cmd = new String[]{"/bin/bash", "-c", COMMAND};

That seems non-portable.

@@ +102,5 @@
> +            final Runtime rt = Runtime.getRuntime();
> +            final String[] cmd = new String[]{"/bin/bash", "-c", COMMAND};
> +
> +            // Execute the sed script at the root of the objdir.
> +            Process proc = rt.exec(cmd, null, new File("../../../"));

I would rather you look at the environment to find this path.

::: build/annotationProcessors/utils/GeneratableElementIterator.java
@@ +143,5 @@
> +            // this fact.
> +            if (isProtected) {
> +                if (!mClassIsProtected) {
> +                    AnnotationProcessor.hitEntryPoint(new MemberReference(mClassName));
> +                    mClassIsProtected = true;

That's a confusing name.

@@ +165,5 @@
>      }
>  
> +    private boolean isProguardProtectedAnnotation(String annotationTypeName) {
> +        if (annotationTypeName.equals("org.mozilla.gecko.mozglue.JNITarget")) {
> +            return true;

return mProtectedAnnotations.contains(annotationTypeName);

would be way more pleasant.

Comment 4

[Chris Kitching [:ckitching]]

Should probably provide a status update on this one:

I'm not going to be touching this until mid-late June at the earliest. Theft encouraged.