794981 - Automagically produce AndroidBridge code that wraps java functions at build time

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

Comment 2

[Chris Kitching [:ckitching]]

Stealing this - needed as part of the make-my-proguard-patch-not-such work for Bug 709230.

I currently have a device that can generate the appropriate C code from Java annotations, but the Java code is compiled after the C code in our build process. As a result, the C code is generated after the C code is compiled, so it is too late to be used. Since the WIP patch above seemed to be following this same approach of using Java annotations to generate the C++ code in AndroidBridge.cpp, I was wondering if you'd thought about this problem and had an idea on how to deal with it.
Perhaps we have some "If the code we've generated has changed then recompile AndroidBridge.cpp" logic? Rejig the makefile so AndroidBridge.cpp is compiled after the Java? (We seem to do something of this flavour with the generation of jni-stubs.inc)

It's also the case that some of the functions in AndroidBridge.cpp have had some additional logic put into them, so my solution currently does not generate the entire file, merely the large fraction of it that consists of entirely mundane wrapper classes. I intend to investigate refactoring this extra logic out into the callers of such functions in due course (So we can generate all the boring JNI boilerplate and untangle the actually interesting logic from it).
Or should I just leave those alone and stick to generating the uninteresting functions only?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Rejigging the makefile to build things in a different order is nontrivial. I would probably go with the same approach that I used for the jni-stubs generation. That is, check-in a version of the .inc generated file and include that into AndroidBridge.cpp. Regenerate that file whenever we do the java-side build, and compare it to the existing .inc file; if it has changed then spit out an error and abort the build. You can see some backstory at bug 794982, comments 2 to 7.

I would like to see all of the JNI wrappers autogenerated, and then, if needed, have additional hand-written wrappers around the JNI ones to do the extra logic needed. Or move that logic to the callers, like you suggested. But in general yes, I would like to generate all the JNI wrappers and not just the uninteresting ones.

Comment 4

[Chris Kitching [:ckitching]]

Attachment: Part 1: Annotate Java methods that are to have JNI wrappers generated.

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Part 2: Don't pass GeckoAppShell class to Init as a parameter - find it in the usual way.

Eliminating the special handling of the GeckoAppShellClass during the startup period. This change is functionally equivalent to the old way, but allows the generator to be more perfectly general.

Comment 6

[Chris Kitching [:ckitching]]

Attachment: Part 3: Make use of the existing macros to simplify JNI code in all applicable places, and relocate them.

There are some rather handy #defines for simplifying AndroidBridge startup code that were being used over in AndroidJNIWrappers.cpp. I've pinched them and applied them to the startup code in AndroidBridge, as well as using them in every applicable place in both locations.
Simpler code is better! (At least, most of the time..)

Comment 7

[Chris Kitching [:ckitching]]

Attachment: Part 4 - Consistently pass nsAString into the JNI methods.

It appears we have a bunch of our own string types. Zillions of them, in fact. Since there is no way for the annotation processor to inspect a Java signature and determine which of these is desired as the input type to the generated C++ wrapper function, one type had to be selected for this purpose.
The alternative is specifying the string type in the Java annotations, which seemed ugly to me. It all boils down to making these conversions eventually anyway - here I'm just doing it at the call sites.

This subsequence of four introductory refactoring patches have green'd the try:
https://tbpl.mozilla.org/?tree=Try&rev=5c4b6adad1b1

Comment 8

[Chris Kitching [:ckitching]]

Attachment: Part 5.1: Add the generated code files to version control.

As per the previous suggestions - adding the generated code to version control.
Not an interesting patch.

It seemed sensible to seperate this into its own patch for the sake of reviewer sanity.

Comment 9

[Chris Kitching [:ckitching]]

Attachment: Part 5.2: Refactoring existing code to accomodate included code.

Semi-desperately factoring out a noninsane subset of the subsequent patch to attempt to reduce its size.

Some final tweaks and including the generated code.

Comment 10

[Chris Kitching [:ckitching]]

Attachment: Part 5.3: Refactor AndroidBridge to make use of generated code.

The gargantuan patch making the content of patch 5.1 actually be used. Deletes all the redundant methods from AndroidBridge.
In the case of nontrivial methods, this patch is responsible for refactoring them such that the part talking to Java is representable as a trivial, generatable function.

Not quite every plausibly-generatable method is generated yet, for reasons to be explained in due course.

Comment 11

[Chris Kitching [:ckitching]]

Attachment: Part 6: Add an annotation processor to the build process to generate the contents of part 5.1

The generator itself. A Java program run during the build process which uses Reflection to inspect the Java classes and generate appropriate JNI method stubs for annotated methods.
Ostensibly my Javadoc comments should make the code fairly self-explanatory.

It adds around 700ms to the build time. It behaves as per the suggestion of Comment 3 and aborts the build if a change is detected. (You then get the fun task of moving two files and building all over again.)

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 794538 [details] [diff] [review]
Part 6: Add an annotation processor to the build process to generate the contents of part 5.1

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

::: mobile/android/base/Makefile.in
@@ +1414,5 @@
>  	$(INSTALL) package-name.txt $(FINAL_TARGET)
>  	@(diff jni-stubs.inc $(topsrcdir)/mozglue/android/jni-stubs.inc >/dev/null) || \
>  	 (echo "*** Error: The jni-stubs have changed. Copy $(CURDIR)/jni-stubs.inc to $(topsrcdir)/mozglue/android" && exit 1)
> +	@(diff GeneratedJNIWrappers.cpp $(topsrcdir)/widget/android/GeneratedJNIWrappers.cpp >/dev/null) || \
> +	 (echo "*** Error: GeneratedJNIWrappers.cpp has changed. Run cp $(CURDIR)/GeneratedJNIWrappers.* $(topsrcdir)/widget/android and repeat the build." && exit 1)

This is a build system: you should be able to do this automatically. In fact, why should we even check in a pre-generated file if we can regenerate it at build time?

Comment 13

[Chris Kitching [:ckitching]]

(In reply to Gregory Szorc [:gps] from comment #12)
> Comment on attachment 794538 [details] [diff] [review]
> Part 6: Add an annotation processor to the build process to generate the
> contents of part 5.1
> 
> Review of attachment 794538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Makefile.in
> @@ +1414,5 @@
> >  	$(INSTALL) package-name.txt $(FINAL_TARGET)
> >  	@(diff jni-stubs.inc $(topsrcdir)/mozglue/android/jni-stubs.inc >/dev/null) || \
> >  	 (echo "*** Error: The jni-stubs have changed. Copy $(CURDIR)/jni-stubs.inc to $(topsrcdir)/mozglue/android" && exit 1)
> > +	@(diff GeneratedJNIWrappers.cpp $(topsrcdir)/widget/android/GeneratedJNIWrappers.cpp >/dev/null) || \
> > +	 (echo "*** Error: GeneratedJNIWrappers.cpp has changed. Run cp $(CURDIR)/GeneratedJNIWrappers.* $(topsrcdir)/widget/android and repeat the build." && exit 1)
> 
> This is a build system: you should be able to do this automatically. In
> fact, why should we even check in a pre-generated file if we can regenerate
> it at build time?

The same question applies to jni-stubs.inc.

You're right, this sort of sucks. The problem in this case is that the GeneratedJNIWrappers.* files are generated using information only available after compiling the java class files. In the build sequence we have, that happens after the various C++ components are compiled, so it's too late to use the new version in this build.
As far as I can tell, the options are this, this but automatically do the copy, or this but automatically do the copy and restart the build.
All three of these ideas suck. Is there a fourth option?

Comment 14

[Gregory Szorc [:gps]]

We typically have code generation occur in an earlier "tier" of the build system, before C++ compilation. If it's possible to extract the C++-generating code into its own Makefile (or at least its own unique target in this Makefile), we can add a "hook" from one of these precompile stages to build just this target in just this Makefile. I think that would solve all of the problems quite nicely.

Comment 15

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #14)
> We typically have code generation occur in an earlier "tier" of the build
> system, before C++ compilation. If it's possible to extract the
> C++-generating code into its own Makefile (or at least its own unique target
> in this Makefile), we can add a "hook" from one of these precompile stages
> to build just this target in just this Makefile. I think that would solve
> all of the problems quite nicely.

I'm tempted to say just leave it as it is, and we'll fix the mess when we blow up tiers. Because whatever is done now, it will have to be adapted anyways.

Comment 16

[Chris Kitching [:ckitching]]

So, this is my current progress on this patch. Code can be generated from annotations, it works, but:

https://tbpl.mozilla.org/?tree=Try&rev=29d466fc806b

Those last two oranges just won't go away. These aren't JNI errors causing aborts - those typically happen at 0xdead00d - this smells like some genuine memory corruption-esque problem.
This is excessively frustrating. Attempts to cure this are part of the reason for the generated code being so very belt-and-braces. 

When running the offending mochitests locally with a debug build of Fennec, the test fails with a segfault caused by an assertion failure. Namely:

08-23 00:49:55.215: ASSERT/MOZ_Assert(18394): Assertion failure: !pd->minidump, at /home/chris/Fennec/Fennec/toolkit/crashreporter/nsExceptionHandler.cpp:2205
08-23 00:49:58.208: ASSERT/MOZ_Assert(18394): Assertion failure: _mOwningThread.GetThread() == PR_GetCurrentThread() (PowerManagerService not thread-safe), at /home/chris/Fennec/Fennec/dom/power/PowerManagerService.cpp:31
08-23 00:49:58.258: ASSERT/libc(18394): Fatal signal 11 (SIGSEGV) at 0x000047da (code=-6), thread 18418 (Gecko)

This seems.. quite far away from my changes. Very strange.
Advice on how to proceed would be appreciated.

Some miscellany:

Why am I not using AutoLocalJNIFrame in generated code?
Because the generated method stubs often need to return values, as Java references, to the caller. It isn't sensible (And, indeed, segfaults quite spectacularly) if we run all the method stubs in the same JNI frame (The local variable table ultimately overflows, overwrites the method table, and produces a NoSuchMethodException at line -76 of GeckoAppShell.java (Did I mention I've had a bad week with this? :P))
The other way of returning a value would be to return a global reference to the caller. I decided against this, because it becomes the caller's responsibility to explicitly free the reference. Failure to do so, in the long term, leads to segfaults, and it's just overkill.

Why are the generated methods always allocating stack frames with 128 local variable slots?
No good reason - indeed, this is a possible future improvement of this patch. I had written a system which only creates enough slots for the input objects and the return value but this kept segfaulting. (Probably because the Java code allocated references in the frame and overflowed it). Until a better solution is thought of, seems I'm stuck with big numbers.

Comment 17

[Chris Kitching [:ckitching]]

(In reply to Mike Hommey [:glandium] from comment #15)
> (In reply to Gregory Szorc [:gps] from comment #14)
> > We typically have code generation occur in an earlier "tier" of the build
> > system, before C++ compilation. If it's possible to extract the
> > C++-generating code into its own Makefile (or at least its own unique target
> > in this Makefile), we can add a "hook" from one of these precompile stages
> > to build just this target in just this Makefile. I think that would solve
> > all of the problems quite nicely.
> 
> I'm tempted to say just leave it as it is, and we'll fix the mess when we
> blow up tiers. Because whatever is done now, it will have to be adapted
> anyways.

I'm unfamiliar with what's occuring in build-system-land. What is this "blowing up" of tiers you speak of?

Comment 18

[Mike Hommey [:glandium]]

(In reply to Chris Kitching [:ckitching] from comment #17)
> I'm unfamiliar with what's occuring in build-system-land. What is this
> "blowing up" of tiers you speak of?

A build system detail you don't have to worry about. I'm just trying to convince Gregory your current approach is fine for now.

Comment 19

[Chris Kitching [:ckitching]]

Attachment: Part 1: V2 - Annotate Java methods that are to have JNI wrappers generated.

Resolved the segfault issues. Further investigation there indicates something is calling the bridge from the wrong thread and silently failing in our existing code. Once I have a concrete view of that unrelated problem I'll be filing a bug on it.
In any case, that's not relavent to this bug. The V2 patch series is on try at:
https://tbpl.mozilla.org/?tree=Try&rev=bacb29952b66
And is expected to go green (A slightly earlier version of it did so.)

This second iteration adds support for all remaining entry points defined in AndroidBridge.cpp, including non-static ones, to be generated.
One or two of these ended up being slightly contrived. It is nevertheless worth doing, as this facilitates ProGuard.

Hopefully I've made an appropriate choice of reviewer.. Let me know if not.

Let's make ProGuard happen!

Comment 20

[Chris Kitching [:ckitching]]

Attachment: Part 4: V2 - Consistently pass nsAString into the JNI methods.

Comment 21

[Chris Kitching [:ckitching]]

Attachment: Part 3: V2 - Make use of the existing macros to simplify JNI code in all applicable places, and relocate them.

Comment 22

[Chris Kitching [:ckitching]]

Attachment: Part 5.1: V2 - Add the generated code files to version control.

Comment 23

[Chris Kitching [:ckitching]]

Attachment: Part 5.3: V2 - Refactor AndroidBridge to make use of generated code.

Comment 24

[Chris Kitching [:ckitching]]

Attachment: Part 6: Storing a void* for mThread instead of a pthread_t is both nonportable and dangerous.

Comment 25

[Chris Kitching [:ckitching]]

Attachment: Part 7: V2 - Add an annotation processor to the build process to generate the contents of part 5.1

This concludes the upload of the second version of the patch series.

Comment 26

[Chris Kitching [:ckitching]]

A meaningful next step might be to expand the system to auto generate the similarly dull entry points in AndroidJavaWrappers.cpp.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 797089 [details] [diff] [review]
Part 1:  V2 - Annotate Java methods that are to have JNI wrappers generated.

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

::: mobile/android/base/GeckoJavaSampler.java
@@ +6,5 @@
>  package org.mozilla.gecko;
>  
>  import android.os.SystemClock;
>  import android.util.Log;
> +import org.mozilla.gecko.mozglue.GeneratableAndroidBridgeTarget;

Leave a blank line between android* imports and org* imports

::: mobile/android/base/Makefile.in
@@ +1271,5 @@
>  	$(JAVAC) $(JAVAC_FLAGS) -Xlint:all,-serial -d classes/websockets $(addprefix $(srcdir)/,$(WEBSOCKETS_JAVA_FILES))
>  	$(JAR) cMf jars/websockets.jar -C classes/websockets .
>  
>  ifdef MOZ_WEBRTC
>  jars/webrtc.jar: $(addprefix $(srcdir)/, $(WEBRTC_JAVA_FILES)) jars jars/gecko-browser.jar jars/gecko-util.jar

This dependency list needs to include jars/gecko-mozglue.jar as well

::: mobile/android/base/ThumbnailHelper.java
@@ +11,5 @@
>  import org.mozilla.gecko.mozglue.DirectBufferAllocator;
>  
>  import android.graphics.Bitmap;
>  import android.util.Log;
> +import org.mozilla.gecko.mozglue.GeneratableAndroidBridgeTarget;

Move this up to be friendly with the other org.mozilla imports

::: mobile/android/base/mozglue/GeneratableAndroidBridgeTarget.java
@@ +7,5 @@
> +import java.lang.annotation.Retention;
> +import java.lang.annotation.RetentionPolicy;
> +
> +@Retention(RetentionPolicy.RUNTIME)
> +public @interface GeneratableAndroidBridgeTarget {

Add a class-level comment with a brief description of what this class is meant to accomplish.

::: mobile/android/base/mozglue/OptionalGeneratedParameter.java
@@ +10,5 @@
> +/**
> + * Used to annotate parameters which are optional on the C++ side of the bridge. The annotation is
> + * used by the annotation processor to generate the appropriate C++ headers so calls into the Java
> + * method all have the optional params set to the default value.
> + * The default values are zero for numerical types, false for booleans, "" for strings, and null

It seems inconsistent to me to use "" as the default value for strings. I would prefer to use null, so that this lines up with the default values for Java class members.

::: mobile/android/base/util/Clipboard.java
@@ +7,5 @@
>  import android.content.ClipData;
>  import android.content.Context;
>  import android.os.Build;
>  import android.util.Log;
> +import org.mozilla.gecko.mozglue.GeneratableAndroidBridgeTarget;

Add a blank line above this import

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 794531 [details] [diff] [review]
Part 2: Don't pass GeckoAppShell class to Init as a parameter - find it in the usual way.

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

::: widget/android/AndroidBridge.cpp
@@ +85,5 @@
>  {
>      ALOG_BRIDGE("AndroidBridge::Init");
>      jEnv->GetJavaVM(&mJavaVM);
>  
> +    AutoLocalJNIFrame jniFrame(jEnv, 254);

Why 254? This needs to be explained in the code if the default value is insufficient.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 797091 [details] [diff] [review]
Part 3: V2 - Make use of the existing macros to simplify JNI code in all applicable places, and relocate them.

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

Minusing for the namespace pollution issue.

::: widget/android/AndroidBridge.cpp
@@ +66,5 @@
> +    if (!classLocalRef) {
> +        ALOG(">>> FATAL JNI ERROR! FindClass(className=\"%s\") failed. Did "
> +             "ProGuard optimize away a non-public class?", className);
> +        env->ExceptionDescribe();
> +       MOZ_CRASH();

indentation. A few other MOZ_CRASH calls below (or lines around them) are also mis-indented.

@@ +285,3 @@
>  
>      if (!GetStaticIntField("android/os/Build$VERSION", "SDK_INT", &mAPIVersion, jEnv))
>          ALOG_BRIDGE("Failed to find API version");

I would move this version-fetching code up a couple of lines so it doesn't sit between the getClassGlobalRef and its associated getField calls.

@@ +1280,5 @@
>              return false;
>      }
>  
> +    initInit();
> +    jclass cls = getClassGlobalRef(className);

In this function and GetStaticStringField below, do you need to define this cls variable? the initInit and getClassGlobalRef already create and populate "jclass jClass", so you should just be able to use jClass instead of cls.

::: widget/android/AndroidBridge.h
@@ +40,5 @@
> +#define initInit() jclass jClass
> +
> +// note that this also sets jClass
> +#define getClassGlobalRef(cname) \
> +    (jClass = AndroidBridge::GetClassGlobalRef(jEnv, cname))

I don't think these #define's belong in a .h file that is included across random files in m-c. Namespace pollution and all that, specially with generic-sounding things like "getField"

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 797090 [details] [diff] [review]
Part 4: V2 - Consistently pass nsAString into the JNI methods.

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

::: widget/android/AndroidBridge.h
@@ +197,5 @@
>  
>      static void NotifyIMEContext(int aState, const nsAString& aTypeHint,
>                                   const nsAString& aModeHint, const nsAString& aActionHint);
>  
> +    static void NotifyIMEChange(const nsAString& a0, int32_t a1, int32_t a2, int32_t a3);

Why change the variable names from a0..a3? (If this is going away in a future patch then no need to fix it)

@@ +250,3 @@
>  
>      void GetMimeTypeFromExtensions(const nsACString& aFileExt, nsCString& aMimeType);
>      void GetExtensionFromMimeType(const nsACString& aMimeType, nsACString& aFileExt);

What about these guys? It looks like only some of the functions were converted? Maybe this will become more clear in future patches.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 797096 [details] [diff] [review]
Part 7: V2 - Add an annotation processor to the build process to generate the contents of part 5.1

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

It looks good overall but there's enough nits and such that I'd like to see an updated version of this patch. Also I didn't review some of the actual codegen code too closely - I'll look at the generated code and make comments there that you might need to update the generator to deal with.

::: build/annotationProcessors/AnnotationProcessor.java
@@ +29,5 @@
> +
> +        System.out.println("Processing annotations...");
> +
> +        // Start the clock!
> +        long s = System.nanoTime();

No point getting nanoTime() if you're only reporting the result in ms. Just use currentTimeMillis().

@@ +44,5 @@
> +            // Iterate all annotated methods in this class..
> +            while (methodIterator.hasNext()) {
> +                MethodWithAnnotationInfo aMethodTuple = methodIterator.next();
> +
> +                CodeGenerator.generateMethod(aMethodTuple, aClass);

It seems odd that this is a static method on CodeGenerator. It would be better OOP practice to create a CodeGenerator instance and generate methods on it, and then get the file contents from it.

@@ +57,5 @@
> +
> +    private static void writeOutputFiles() {
> +        try {
> +            FileOutputStream outStream = new FileOutputStream(OUTFILE);
> +            outStream.write(CodeGenerator.getFinalWrapperFileContents());

Is there a non-final wrapper file contents? If not then we can remove "Final" from this method name.

::: build/annotationProcessors/CodeGenerator.java
@@ +14,5 @@
> +    // Buffers holding the strings to ultimately be written to the output files.
> +    public static StringBuilder wrapperStartupCode = new StringBuilder(4000);
> +    public static StringBuilder wrapperMethodBodies = new StringBuilder(4000);
> +    public static StringBuilder headerFields = new StringBuilder(4000);
> +    public static StringBuilder headerMethods = new StringBuilder(4000);

These should be private. Also preferably non-static, see my comments in AnnotationProcessor.java

@@ +16,5 @@
> +    public static StringBuilder wrapperMethodBodies = new StringBuilder(4000);
> +    public static StringBuilder headerFields = new StringBuilder(4000);
> +    public static StringBuilder headerMethods = new StringBuilder(4000);
> +
> +    private static final HashMap<String, Boolean> seenClasses = new HashMap<String, Boolean>(10);

Sounds like you want this to be a Set, not a Map

@@ +23,5 @@
> +        // Write the file header things. Includes and so forth.
> +        // GeneratedJNIWrappers.cpp is generated as the concatenation of wrapperStartupCode with
> +        // wrapperMethodBodies. Similarly, GeneratedJNIWrappers.h is the concatenation of headerFields
> +        // with headerMethods.
> +        wrapperStartupCode.append("#include \"nsXPCOMStrings.h\"\n" +

Move this down a line and align it with the rest of the strings

@@ +34,5 @@
> +                "#endif\n" +
> +                "\n" +
> +                "using namespace mozilla;\n" +
> +                "// GENERATED CODE\n" +
> +                "// Based on annotations on corresponding Java methods. Generates wrapper methods here.\n" +

I would flesh out this comment a little bit more so that people who look in the generated file have a better idea of how to go about updating it properly.

@@ +40,5 @@
> +                "    ALOG_BRIDGE(\"%s\", __PRETTY_FUNCTION__);\n" +
> +                "    initInit();\n");
> +        // Now we write the various GetStaticMethodID calls here...
> +
> +        headerFields.append("// GENERATED CODE - JNI wrapper header file, created from annotations on Java methods.\n");

Ditto.

::: build/annotationProcessors/MethodWithAnnotationInfo.java
@@ +12,5 @@
> +public class MethodWithAnnotationInfo {
> +    public Method method;
> +    public String wrapperName;
> +    public boolean isStatic;
> +    public boolean isMultithreaded;

Make all of these final

::: build/annotationProcessors/classloader/IterableJarLoadingURLClassLoader.java
@@ +23,5 @@
> + * contents.
> + * This classloader does not support inner classes. (You probably shouldn't be putting JNI entry
> + * points in inner classes anyway)
> + */
> +public class IterableJarLoadingURLClassLoader extends URLClassLoader implements Iterable<Class<?>> {

Why are you making this implement Iterable if you're never using the feature set that affords? I.e. you never do anything like

IteratableJarLoadingURLClassLoader foo = ...;
for (Class<?> c : foo) {
  ...
}

which is the whole point of making something Iterable. I would say get rid of the "implements Iterable" clause, remove the iterator() function on this class, and have the getIteratorOverJars function just return a new JarClassIterator directly. Or something. This seems over-engineered, at any rate.

@@ +24,5 @@
> + * This classloader does not support inner classes. (You probably shouldn't be putting JNI entry
> + * points in inner classes anyway)
> + */
> +public class IterableJarLoadingURLClassLoader extends URLClassLoader implements Iterable<Class<?>> {
> +    LinkedList<String> classNames = new LinkedList<String>();

Make this private and final, and pass classNames.iterator() to the JarClassIterator constructor instead of "this".

::: build/annotationProcessors/classloader/JarClassIterator.java
@@ +9,5 @@
> +/**
> + * Class for iterating over an IterableJarLoadingURLClassLoader's classes.
> + */
> +public class JarClassIterator implements Iterator<Class<?>> {
> +    private IterableJarLoadingURLClassLoader mTarget;

mTarget is never used

::: build/annotationProcessors/utils/AppropriatelyAnnotatedMethodIterator.java
@@ +18,5 @@
> + * Iterator over the methods in a given method list which have the GeneratableAndroidBridgeTarget
> + * annotation. Returns an object containing both the annotation (Which may contain interesting
> + * parameters) and the argument.
> + */
> +public class AppropriatelyAnnotatedMethodIterator implements Iterator<MethodWithAnnotationInfo> {

Rename this to JNIEntryMethodIterator or something similar. It doesn't add value to have a really long name which is so generic. "Appropriately annotated" for what?

@@ +21,5 @@
> + */
> +public class AppropriatelyAnnotatedMethodIterator implements Iterator<MethodWithAnnotationInfo> {
> +    Method[] mMethods;
> +    MethodWithAnnotationInfo mNextReturnValue;
> +    int mMethodIndex;

Make these all private. mMethods can also be made final.

@@ +28,5 @@
> +        // Sort the methods into alphabetical order by name, to ensure we always iterate methods
> +        // in the same order..
> +        List<Method> tempList = Arrays.asList(aMethods);
> +        Collections.sort(tempList, new AlphabeticMethodComparator());
> +        mMethods = tempList.toArray(new Method[aMethods.length]);

Why bother converting tempList back to an array? You could just keep the List.

@@ +39,5 @@
> +     * one exists. Otherwise cache null, so hasNext returns false.
> +     */
> +    private void findNextValue() {
> +        while (mMethodIndex < mMethods.length) {
> +            Method candidateMethod = mMethods[mMethodIndex];

Better to do mMethods[mMethodIndex++] here so that you don't have to increment it in multiple places near the exits to the loop.

@@ +41,5 @@
> +    private void findNextValue() {
> +        while (mMethodIndex < mMethods.length) {
> +            Method candidateMethod = mMethods[mMethodIndex];
> +            Annotation[] annotations = candidateMethod.getDeclaredAnnotations();
> +            for (int c = 0; c < annotations.length; c++) {

More readable to do:

for (Annotation annotation : candidateMethod.getDeclaredAnnotations()) {

@@ +49,5 @@
> +
> +                if (annotationTypeName.equals("org.mozilla.gecko.mozglue.GeneratableAndroidBridgeTarget")) {
> +                    Method stubNameMethod;
> +                    Method staticStubMethod;
> +                    Method multithreadedStubMethod;

These Method variables are only ever used inside the try block, so you can declare them there. (i.e. keep them to the innermost scope they are used in)

@@ +64,5 @@
> +                        multithreadedStubMethod = annotationType.getDeclaredMethod("allowMultithread");
> +                        multithreadedStubMethod.setAccessible(true);
> +                        isMultithreadedStub = (Boolean) multithreadedStubMethod.invoke(annotations[c]);
> +                    } catch (NoSuchMethodException e) {
> +                        System.err.println("Unable to find stubName field on GeneratableAndroidBridgeTarget annotation. Did the signature change?");

Error messages all refer to "stubName" but there are a few different methods that could all throw these exceptions.

::: build/annotationProcessors/utils/Utils.java
@@ +14,5 @@
> + */
> +public class Utils {
> +
> +    // A collection of lookup tables to simplify the functions to follow...
> +    private static final HashMap<String, String> sBasicCTypes = new HashMap<String, String>(9);

Common mistake: trying to be too clever and sizing your HashMap to the number of items you're going to put into it, rather than the capacity it needs to be in order to hold that many items, accounting for load factor. Solution: don't be too clever. :)

Seriously though - all the places you've initialized StringBuilders and such to specific sizes (see CodeGenerator.java) are really not necessary. It's a prime example of premature optimization. For HashMaps it's particularly bad because HashMaps have terrible performance when they have non-prime capacities.

@@ +126,5 @@
> +            return sBasicCTypes.get(name);
> +        }
> +        // Are we dealing with an array type?
> +        int len = name.length();
> +        if (name.substring(len - 2).equals("[]")) {

This could throw exceptions if len < 2. Or below, if len < 4 and name ends in []. We're unlikely to run into this, but it might be simpler to just use name.endsWith("[]") instead as that avoids the problem and is slightly more readable.

@@ +240,5 @@
> +     *
> +     * @param sb The buffer to write into.
> +     * @param c  The type of the element to write the subsignature of.
> +     */
> +    private static void writeTypeSignature(StringBuilder sb, Class<?> c) {

I'm surprised there's no existing function in the Java API to generate this.

@@ +244,5 @@
> +    private static void writeTypeSignature(StringBuilder sb, Class<?> c) {
> +        String name = c.getCanonicalName().replaceAll("\\.", "/");
> +        // Determine if this is an array type and, if so, peel away the array operators..
> +        int len = name.length();
> +        while (name.substring(len - 2).equals("[]")) {

endsWith

::: mobile/android/base/Makefile.in
@@ +1312,5 @@
> +  build/annotationProcessors/classloader/IterableJarLoadingURLClassLoader.java \
> +  build/annotationProcessors/classloader/JarClassIterator.java                 \
> +  build/annotationProcessors/utils/AlphabeticMethodComparator.java             \
> +  build/annotationProcessors/utils/AppropriatelyAnnotatedMethodIterator.java   \
> +  build/annotationProcessors/utils/Utils.java                                  \

Personally I don't think there's enough classes here to warrant separating them into classloader and utils packages, specially since everything is public anyway. It seems unnecessarily verbose to have it this way, but I don't really care.

@@ +1431,5 @@
>  	$(INSTALL) classes.dex $(FINAL_TARGET)
>  	$(INSTALL) package-name.txt $(FINAL_TARGET)
>  	@(diff jni-stubs.inc $(topsrcdir)/mozglue/android/jni-stubs.inc >/dev/null) || \
>  	 (echo "*** Error: The jni-stubs have changed. Copy $(CURDIR)/jni-stubs.inc to $(topsrcdir)/mozglue/android" && exit 1)
> +	@(diff GeneratedJNIWrappers.cpp $(topsrcdir)/widget/android/GeneratedJNIWrappers.cpp >/dev/null) || \

It would be nice to combine this command into the previous one so that if both jni-stubs.inc and the GeneratedJNIWrappers.cpp change it only errors out once.

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 797092 [details] [diff] [review]
Part 5.1: V2 - Add the generated code files to version control.

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

::: widget/android/GeneratedJNIWrappers.cpp
@@ +126,5 @@
> +        ALOG_BRIDGE("Aborted: No env - %s", __PRETTY_FUNCTION__);
> +        return;
> +    }
> +
> +    env->PushLocalFrame(0);

Push local frame can fail and return an error code. We need to check for that. Also I'm not sure if there's any advantage to doing the call at all if the argument is 0. We could just skip it entirely.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 797093 [details] [diff] [review]
Part 5.3: V2 - Refactor AndroidBridge to make use of generated code.

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

::: widget/android/AndroidBridge.h
@@ +400,5 @@
>      jclass jSurfaceClass;
>      jfieldID jSurfacePointerField;
>  
>      jclass jLayerView;
> +

trailing whitespace

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 794535 [details] [diff] [review]
Part 5.2: Refactoring existing code to accomodate included code.

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

Dropping review flag on this for now because I've spent the last 10 minutes switching between the three "part 5" patches trying to make sense of it. It will be easier if you just squash those three together and I can see how it all ties together.

::: widget/android/AndroidBridge.cpp
@@ +173,5 @@
>      mHasNativeWindowFallback = false;
>  
>      initInit();
> +    InitStubs(jEnv);
> +    jClass = mGeckoAppShellClass;

Not sure what's going on here. You removed mGeckoAppShellClass from the .h file.

::: widget/android/moz.build
@@ +15,5 @@
>  EXPORTS += [
>      'AndroidBridge.h',
>      'AndroidJNIWrapper.h',
>      'AndroidJavaWrappers.h',
> +    'GeneratedJNIWrappers.h'

I don't know exactly what the EXPORTS section implies but I don't think the .h file should be exported to be include'able from other parts of the codebase.

Comment 35

[Chris Kitching [:ckitching]]

Attachment: Part 1: V3 - Annotate Java methods that are to have JNI wrappers generated.

Comment 36

[Chris Kitching [:ckitching]]

Attachment: Part 2: V2 - Don't pass GeckoAppShell class to Init as a parameter - find it in the usual way.

Comment 37

[Chris Kitching [:ckitching]]

Attachment: Part 3: V3 - Make use of the existing macros to simplify JNI code in all applicable places, and relocate them.

Comment 38

[Chris Kitching [:ckitching]]

Attachment: Part 4: V3 - Consistently pass nsAString into the JNI methods.

Comment 39

[Chris Kitching [:ckitching]]

Attachment: Part 5: Add the generated code files to version control.

Comment 40

[Chris Kitching [:ckitching]]

Attachment: Part 6: V3 - Refactor AndroidBridge to make use of generated code.

Comment 41

[Chris Kitching [:ckitching]]

Attachment: Part 7: Storing a void* for mThread instead of a pthread_t is both nonportable and dangerous.

Comment 42

[Chris Kitching [:ckitching]]

Attachment: Part 8: Add an annotation processor to the build process to generate the contents of part 5.1

Okay, revised patch series. Every not-responded-to comment from your reviews has been applied and was not sufficiently interesting to warrant a comment. (Didn't seem pointful to say "Yes, well spotted" a very large number of times :P )

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)
> ::: mobile/android/base/mozglue/OptionalGeneratedParameter.java
> @@ +10,5 @@
> > +/**
> > + * Used to annotate parameters which are optional on the C++ side of the bridge. The annotation is
> > + * used by the annotation processor to generate the appropriate C++ headers so calls into the Java
> > + * method all have the optional params set to the default value.
> > + * The default values are zero for numerical types, false for booleans, "" for strings, and null
> 
> It seems inconsistent to me to use "" as the default value for strings. I
> would prefer to use null, so that this lines up with the default values for
> Java class members.

This actually isn't as easy as you might think - I tried.

Such a change would mandate the addition of extra null checks in Java code which accepts such default values, as the existing code is set up to accept the empty string as its default.

But the real problem happens when you try and represent the null pointer in the C++ wrapper methods. From the spec:

"A reference shall be initialized to refer to a valid object or function. [Note: in particular, a null reference cannot exist in a well-defined program, because the only way to create such a reference would be to bind it to the “object” obtained by dereferencing a null pointer, which causes undefined behavior. As described in 9.6, a reference cannot be bound directly to a bit-field. ]"

The string parameter type of the generated wrappers is an nsAString&. This cannot be null. Using a special sentinel value to represent null seems dangerous and ugly. I'm disinclined to implement this, and the actual gain is essentially nonexistent anyway.


(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> ::: widget/android/AndroidBridge.cpp
> @@ +85,5 @@
> >  {
> >      ALOG_BRIDGE("AndroidBridge::Init");
> >      jEnv->GetJavaVM(&mJavaVM);
> >  
> > +    AutoLocalJNIFrame jniFrame(jEnv, 254);
> 
> Why 254? This needs to be explained in the code if the default value is
> insufficient.

Ah. This change should not be here. For a while, I incorrectly juxtaposed methodIDs and local references in my head and did this. Removed the change.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #29)
> ::: widget/android/AndroidBridge.h
> @@ +40,5 @@
> > +#define initInit() jclass jClass
> > +
> > +// note that this also sets jClass
> > +#define getClassGlobalRef(cname) \
> > +    (jClass = AndroidBridge::GetClassGlobalRef(jEnv, cname))
> 
> I don't think these #define's belong in a .h file that is included across
> random files in m-c. Namespace pollution and all that, specially with
> generic-sounding things like "getField"

Makes sense - moving them into AndroidJavaWrappers.h seems a noninsane approach, I think?

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> ::: widget/android/AndroidBridge.h
> @@ +197,5 @@
> >  
> >      static void NotifyIMEContext(int aState, const nsAString& aTypeHint,
> >                                   const nsAString& aModeHint, const nsAString& aActionHint);
> >  
> > +    static void NotifyIMEChange(const nsAString& a0, int32_t a1, int32_t a2, int32_t a3);
> 
> Why change the variable names from a0..a3? (If this is going away in a
> future patch then no need to fix it)

Whoops - minor error here. So there is in fact no reason to change the names, just that the way I wrote the patches in a slightly funky order (Generated code before the refactoring). Looks like when I adapted this signature I updated the names, when it was only necessary to update the types. In any case, the entire method is deleted later, so changing it seems pointless.

> @@ +250,3 @@
> >  
> >      void GetMimeTypeFromExtensions(const nsACString& aFileExt, nsCString& aMimeType);
> >      void GetExtensionFromMimeType(const nsACString& aMimeType, nsACString& aFileExt);
> 
> What about these guys? It looks like only some of the functions were
> converted? Maybe this will become more clear in future patches.

These two methods weren't found to be entirely generatable - they contained extra logic. As a result, I ended up generating a wrapper method for the Java entry point, but retaining the outer method with its extra logic. Since this was happening, there was no need to change the type of this one to take nsAString, since it isn't subsequently being directly replaced by generated code in the way that NotifyIMEChange is.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #32)
> ::: widget/android/GeneratedJNIWrappers.cpp
> @@ +126,5 @@
> > +        ALOG_BRIDGE("Aborted: No env - %s", __PRETTY_FUNCTION__);
> > +        return;
> > +    }
> > +
> > +    env->PushLocalFrame(0);
> 
> Push local frame can fail and return an error code. We need to check for
> that. Also I'm not sure if there's any advantage to doing the call at all if
> the argument is 0. We could just skip it entirely.

Checking for the error seems sensible.

Not pushing a stack frame on the Java side can cause strangeness, up to and including dynamic scoping-like properties, especially if the Java method subsequently calls back into C (That new native method will inherit the local variable context of this caller). Or something. The actual semantics are barely documented, but it seems sensible to keep the whole "One stack frame per function call" thing going unless we develop a particularly compelling reason not to.
Most compellingly, this happens:
https://tbpl.mozilla.org/?tree=Try&rev=bb61466e64f3

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> @@ +44,5 @@
> > +            // Iterate all annotated methods in this class..
> > +            while (methodIterator.hasNext()) {
> > +                MethodWithAnnotationInfo aMethodTuple = methodIterator.next();
> > +
> > +                CodeGenerator.generateMethod(aMethodTuple, aClass);
> 
> It seems odd that this is a static method on CodeGenerator. It would be
> better OOP practice to create a CodeGenerator instance and generate methods
> on it, and then get the file contents from it.

The CodeGenerator object that would result would be a Singleton. It would also be a Singleton that is assigned on startup and never destroyed until the end of the program. Such Singletons are functionally equivalent to classes containing static members, except they perform very, very slightly less well. There appear to be no upsides to this suggestion, and no significant downsides either, except for a reasonably largeish amount of seemingly meaningless refactoring. I'd appreciate more clarity on how this is conretely better.

> build/annotationProcessors/classloader/IterableJarLoadingURLClassLoader.java
> @@ +23,5 @@
> > + * contents.
> > + * This classloader does not support inner classes. (You probably shouldn't be putting JNI entry
> > + * points in inner classes anyway)
> > + */
> > +public class IterableJarLoadingURLClassLoader extends URLClassLoader implements Iterable<Class<?>> {
> 
> Why are you making this implement Iterable if you're never using the feature
> set that affords?

Abstraction. Writing reusable code. Solving the general problem, and applying it to the specific case, instead of writing something capable of only dealing with this one situation.
Implemented the changes you requested.

> @@ +24,5 @@
> > + * This classloader does not support inner classes. (You probably shouldn't be putting JNI entry
> > + * points in inner classes anyway)
> > + */
> > +public class IterableJarLoadingURLClassLoader extends URLClassLoader implements Iterable<Class<?>> {
> > +    LinkedList<String> classNames = new LinkedList<String>();
> 
> Make this private and final, and pass classNames.iterator() to the
> JarClassIterator constructor instead of "this".

Not possible - JarClassIterator needs a reference to the class loader so it can load classes. The iterator alone is not sufficient.

> ::: build/annotationProcessors/classloader/JarClassIterator.java
> @@ +9,5 @@
> > +/**
> > + * Class for iterating over an IterableJarLoadingURLClassLoader's classes.
> > + */
> > +public class JarClassIterator implements Iterator<Class<?>> {
> > +    private IterableJarLoadingURLClassLoader mTarget;
> 
> mTarget is never used

Yes it is. Line 30.

> :::
> build/annotationProcessors/utils/AppropriatelyAnnotatedMethodIterator.java
> @@ +18,5 @@
> > + * Iterator over the methods in a given method list which have the GeneratableAndroidBridgeTarget
> > + * annotation. Returns an object containing both the annotation (Which may contain interesting
> > + * parameters) and the argument.
> > + */
> > +public class AppropriatelyAnnotatedMethodIterator implements Iterator<MethodWithAnnotationInfo> {
> 
> Rename this to JNIEntryMethodIterator or something similar. It doesn't add
> value to have a really long name which is so generic. "Appropriately
> annotated" for what?

That really is an impressively odd name I chose for that class...

> ::: build/annotationProcessors/utils/Utils.java
> @@ +14,5 @@
> > + */
> > +public class Utils {
> > +
> > +    // A collection of lookup tables to simplify the functions to follow...
> > +    private static final HashMap<String, String> sBasicCTypes = new HashMap<String, String>(9);
> 
> Common mistake: trying to be too clever and sizing your HashMap to the
> number of items you're going to put into it, rather than the capacity it
> needs to be in order to hold that many items, accounting for load factor.
> Solution: don't be too clever. :)

Ugh. Of course. I appear to be forgetting my datastructures courses already.

That said...

The Java implementation of HashMap jumps the capacity given up to the next nearest power of two if it is not already a power of two[1], so most of my hashmaps here are identical to if I'd just not specified a size at all. (The default size is 16) This is still very dumb, but not quite as dumb as it first appears :P.
The startup time of the app will be slightly improved if I create the maps with initial size of 16 and load factor of 1, as no rehashing will subsequently need to take place during the static initialisers. Might as well do it.

[1]http://stackoverflow.com/questions/7115445/what-is-the-optimal-capacity-and-load-factor-for-a-fixed-size-hashmap

> Seriously though - all the places you've initialized StringBuilders and such
> to specific sizes (see CodeGenerator.java) are really not necessary. It's a
> prime example of premature optimization.

This is less true. When a StringBuilder overflows it must allocate a new, larger buffer and copy the contents into it. This is expensive. The performance gain from correctly choosing sizes for StringBuilders can be quite large. The costs of choosing stupid sizes are also potentially quite large. It turns out many of my choices of size were stupid - I've retuned them to more appropriate values. Might save a millisecond of build time :P.

> 
> @@ +126,5 @@
> > +            return sBasicCTypes.get(name);
> > +        }
> > +        // Are we dealing with an array type?
> > +        int len = name.length();
> > +        if (name.substring(len - 2).equals("[]")) {
> 
> This could throw exceptions if len < 2. Or below, if len < 4 and name ends
> in []. We're unlikely to run into this, but it might be simpler to just use
> name.endsWith("[]") instead as that avoids the problem and is slightly more
> readable.

This fault will only occur if we annotate for generation a Java method which takes as an argument a type with a canonical name of length < 4.

If we're doing this, all hope is most certainly lost.

In any case, endsWith is certainly more readable, so let's do it.

> ::: mobile/android/base/Makefile.in
> @@ +1312,5 @@
> > +  build/annotationProcessors/classloader/IterableJarLoadingURLClassLoader.java \
> > +  build/annotationProcessors/classloader/JarClassIterator.java                 \
> > +  build/annotationProcessors/utils/AlphabeticMethodComparator.java             \
> > +  build/annotationProcessors/utils/AppropriatelyAnnotatedMethodIterator.java   \
> > +  build/annotationProcessors/utils/Utils.java                                  \
> 
> Personally I don't think there's enough classes here to warrant separating
> them into classloader and utils packages, specially since everything is
> public anyway. It seems unnecessarily verbose to have it this way, but I
> don't really care.

Similarly, at some point in the dim and distant past there weren't enough classes in mobile/android/base to warrant splitting them into seperate packages. Work continued, and suddenly there was - but nobody did the, by that point, unpleasantly large refactoring job to tidy it up. And now look at the mess we have there.

Such verbosity is not necessary at the moment, but the high probability of feature creep makes it likely to suddenly become so. When it becomes necessary and you haven't had it all along you're in trouble, just like we are with mobile/android/base. 

When someone wants to add a new class, if there exists a package for it they'll almost certainly put it there. If there's just one or a small number of classes related to it in a top-level package they'll likely just put it there. Even if they do attempt to refactor those existing classes into a package, they'll likely have their patch rejected as being "out of scope" (Happened to me :P ). And thus the clutter will grow.

> 
> @@ +1431,5 @@
> >  	$(INSTALL) classes.dex $(FINAL_TARGET)
> >  	$(INSTALL) package-name.txt $(FINAL_TARGET)
> >  	@(diff jni-stubs.inc $(topsrcdir)/mozglue/android/jni-stubs.inc >/dev/null) || \
> >  	 (echo "*** Error: The jni-stubs have changed. Copy $(CURDIR)/jni-stubs.inc to $(topsrcdir)/mozglue/android" && exit 1)
> > +	@(diff GeneratedJNIWrappers.cpp $(topsrcdir)/widget/android/GeneratedJNIWrappers.cpp >/dev/null) || \
> 
> It would be nice to combine this command into the previous one so that if
> both jni-stubs.inc and the GeneratedJNIWrappers.cpp change it only errors
> out once.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #34)
> Comment on attachment 794535 [details] [diff] [review]
> Part 5.2: Refactoring existing code to accomodate included code.
> 
> Review of attachment 794535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Dropping review flag on this for now because I've spent the last 10 minutes
> switching between the three "part 5" patches trying to make sense of it. It
> will be easier if you just squash those three together and I can see how it
> all ties together.
> 
> ::: widget/android/AndroidBridge.cpp
> @@ +173,5 @@
> >      mHasNativeWindowFallback = false;
> >  
> >      initInit();
> > +    InitStubs(jEnv);
> > +    jClass = mGeckoAppShellClass;
> 
> Not sure what's going on here. You removed mGeckoAppShellClass from the .h
> file.

My apologies - in my desperation to try and split the patches into nice bitesized chunks for review I managed to make it even more confusing than it was to start with.
I've folded what were parts 5.2 and 5.3 into one patch - for now I've left the adding of the generated code in its own patch, since this code is largely uninteresting to look at, anyway.

In short, what happens in this patch is all of the methods in AndroidBridge.cpp that were generatable are deleted.
All the partially-generated methods are refactored and the parts of them that were generated are replaced with calls to the generated code.

The thing you pointed to in this particular comment is a particularly idiotic artefact of my somewhat unwise choice of patch splitting point.

> ::: widget/android/moz.build
> @@ +15,5 @@
> >  EXPORTS += [
> >      'AndroidBridge.h',
> >      'AndroidJNIWrapper.h',
> >      'AndroidJavaWrappers.h',
> > +    'GeneratedJNIWrappers.h'
> 
> I don't know exactly what the EXPORTS section implies but I don't think the
> .h file should be exported to be include'able from other parts of the
> codebase.

Likewise, I am unsure what this does. What I do know is that if I remove this then it fails to compile, complaining:

 0:13.45 In file included from /home/chris/Fennec/Fennec/xpcom/base/nsSystemInfo.cpp:24:0:
 0:13.45 ../../dist/include/AndroidBridge.h:418:38: fatal error: GeneratedJNIWrappers.h: No such file or directory
 0:13.45 compilation terminated.
 0:13.45 
 0:13.45 In the directory  /home/chris/Fennec/Fennec/objdir-droid-debug/xpcom/base
 0:13.45 The following command failed to execute properly:
 0:13.46 /usr/bin/ccache /opt/android-ndk/toolchains/arm-linux-androideabi-4.7/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -o nsSystemInfo.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DOS_POSIX=1 -DOS_LINUX=1 -I/home/chris/Fennec/Fennec/ipc/chromium/src -I/home/chris/Fennec/Fennec/ipc/glue -I../../ipc/ipdl/_ipdlheaders -I/home/chris/Fennec/Fennec/xpcom/base/../build -I/home/chris/Fennec/Fennec/xpcom/ds -I/home/chris/Fennec/Fennec/xpcom/base -I. -I../../dist/include -I/home/chris/Fennec/Fennec/objdir-droid-debug/dist/include/nspr -I/home/chris/Fennec/Fennec/objdir-droid-debug/dist/include/nss -fPIC -isystem /opt/android-ndk/platforms/android-9/arch-arm/usr/include -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -mandroid -fno-short-enums -fno-exceptions -Wno-psabi -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -I/home/chris/Fennec/Fennec/build/stlport/stlport -I/opt/android-ndk/sources/cxx-stl/system/include -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -std=gnu++0x -pipe -DDEBUG -D_DEBUG -DTRACING -g -Os -freorder-blocks -fno-reorder-functions -funwind-tables -isystem /opt/android-ndk/platforms/android-9/arch-arm/usr/include -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/nsSystemInfo.o.pp /home/chris/Fennec/Fennec/xpcom/base/nsSystemInfo.cpp
 0:13.46 make[6]: *** [nsSystemInfo.o] Error 1
 0:13.46 make[5]: *** [libs] Error 2
 0:13.46 make[4]: *** [libs_tier_platform] Error 2
 0:13.46 make[3]: *** [tier_platform] Error 2
 0:13.46 make[2]: *** [default] Error 2
 0:13.46 make[1]: *** [realbuild] Error 2
 0:13.46 make: *** [build] Error 2

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 797534 [details] [diff] [review]
Part 3: V3 - Make use of the existing macros to simplify JNI code in all applicable places, and relocate them.

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

::: widget/android/AndroidJavaWrappers.h
@@ +33,5 @@
> +#define initInit() jclass jClass
> +
> +// note that this also sets jClass
> +#define getClassGlobalRef(cname) \
> +    (jClass = AndroidBridge::GetClassGlobalRef(jEnv, cname))

AndroidJavaWrappers.h is included from AndroidBridge.h so moving these #define's here doesn't help too much, I think. It still pollutes the global namespace. I would rather pull them out into a separate .h file that is included directly in AndroidBridge.cpp and AndroidJavaWrappers.cpp, although I'm open to other reasonable solutions too.

Comment 44

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 797537 [details] [diff] [review]
Part 6: V3 - Refactor AndroidBridge to make use of generated code.

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

Looks good. Some comments below. Holding off on r+ until I hear back about the local frame business and the int/float conversion.

::: widget/android/AndroidBridge.cpp
@@ +240,5 @@
> +       return NULL;
> +    }
> +    return ret;
> +}
> +jstring AndroidBridge::NewJavaString(JNIEnv* env, const nsAString& string) {

nit: blank line between functions

@@ +306,5 @@
>      JNIEnv *env = GetJNIEnv();
>      if (!env)
>          return false;
>  
> +    jobjectArray arr = GetHandlersForMimeTypeWrapper(aMimeType, aAction);

So I might be missing something here - please let me know if I am. AIUI, GetHandlersForMimeTypeWrapper calls PushLocalFrame for its local variables and then calls PopLocalFrame with the return value, which returns it as a local variable in the next innermost local frame, having popped the actual innermost local frame. So here, "arr" is in whatever local frame existed before AndroidBridge::GetHandlersForMimeType was called. This has two problems:
(1) In the case of functions that are called from threads (e.g. the compositor thread) that are created in c++, they might not have any local frame at all. I don't know what happens in that scenario, but I imagine it's not pretty.
(2) If AndroidBridge::GetHandlersForMimeType is called a bunch of times in a row, it will eventually fill up the local frame that it is being called with, and will overflow. I also imagine this is not pretty.
Can you check these scenarios to make sure they are handled correctly?

@@ +357,5 @@
>      if (!env)
>          return;
>  
> +    jstring jstrType = GetMimeTypeFromExtensionsWrapper(NS_ConvertUTF8toUTF16(aFileExt));
> +    if (jstrType == nullptr) {

nit: the convention for existing code in the file seems to be to check !jstrType rather than jstrType == nullptr so for consistency that would be better. I don't personally care much either way.

@@ +1587,2 @@
>  
> +    if(!orientation)

nit: space between if and (

@@ +1663,5 @@
>      if (!win)
>          return;
>  
>      CSSRect cssRect = rect / CSSToLayoutDeviceScale(win->GetDefaultScale());
> +    AddPluginViewWrapper(view, (int)cssRect.x, (int)cssRect.y, (int)cssRect.width, (int)cssRect.height, isFullScreen);

Why the cast to ints? These should be floats all the way through.

::: widget/android/AndroidBridge.h
@@ +414,5 @@
>      // to touch it
>      nsTArray<DelayedTask*> mDelayedTaskQueue;
>  
>  public:
> +    #include "GeneratedJNIWrappers.h"

It might be worth having GeneratedJNIWrappers.h have private and public parts to it, so that the methods are exposed but the fields are not.

Comment 45

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 797539 [details] [diff] [review]
Part 8: Add an annotation processor to the build process to generate the contents of part 5.1

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

::: build/annotationProcessors/utils/Utils.java
@@ +394,5 @@
> +    private static String getDefaultParameterValueForType(Class<?> aArgumentType) {
> +        String typeName = aArgumentType.getCanonicalName();
> +        if (sDefaultParameterValues.containsKey(typeName)) {
> +            return sDefaultParameterValues.get(typeName);
> +        } else if (isCharSequence(aArgumentType)){

nit: space before {

::: mobile/android/base/Makefile.in
@@ +1311,5 @@
> +  build/annotationProcessors/MethodWithAnnotationInfo.java                     \
> +  build/annotationProcessors/classloader/IterableJarLoadingURLClassLoader.java \
> +  build/annotationProcessors/classloader/JarClassIterator.java                 \
> +  build/annotationProcessors/utils/AlphabeticMethodComparator.java             \
> +  build/annotationProcessors/utils/GeneratableEntryPointIterator.java   \

Line up the backslashes, or don't line up any of them (which would be my personal preference, and I think more consistent with the rest of the file).

Comment 46

[Chris Kitching [:ckitching]]

Attachment: Part 3: V4 - Make use of the existing macros to simplify JNI code in all applicable places, and relocate them.

Extracting the macros into their own file. Was this more what you were after?

Comment 47

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Kitching [:ckitching] from comment #42)
> Okay, revised patch series. Every not-responded-to comment from your reviews
> has been applied and was not sufficiently interesting to warrant a comment.
> (Didn't seem pointful to say "Yes, well spotted" a very large number of
> times :P )

Thank you! Reviewing the revised patches was quite easy because you responded to everything :) Also, thanks for working on this bug - it's turning out to be much more complex than I had originally envisioned and I'm glad that it's finally getting done.

I have some additional comments below - you can assume that anything I didn't respond to I'm ok with whatever you suggested and/or your rationale for doing things the way you did.

> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> > It seems odd that this is a static method on CodeGenerator. It would be
> > better OOP practice to create a CodeGenerator instance and generate methods
> > on it, and then get the file contents from it.
> 
> The CodeGenerator object that would result would be a Singleton. It would
> also be a Singleton that is assigned on startup and never destroyed until
> the end of the program. Such Singletons are functionally equivalent to
> classes containing static members, except they perform very, very slightly
> less well. There appear to be no upsides to this suggestion, and no
> significant downsides either, except for a reasonably largeish amount of
> seemingly meaningless refactoring. I'd appreciate more clarity on how this
> is conretely better.

For the same reason that you went to the trouble of making reusable IterableJarLoadingURLClassLoader and the like. That is - abstraction, writing reusable code, etc. Currently CodeGenerator is only created and used from the main function but there may be a day in the future where it is not, or where we will want to reuse it elsewhere. That being said I'm ok with leaving it as-is and worrying about it when that day comes.

> > Make this private and final, and pass classNames.iterator() to the
> > JarClassIterator constructor instead of "this".
> 
> Not possible - JarClassIterator needs a reference to the class loader so it
> can load classes. The iterator alone is not sufficient.

> > 
> > mTarget is never used
> 
> Yes it is. Line 30.

Ah, sorry. I missed that.

> The Java implementation of HashMap jumps the capacity given up to the next
> nearest power of two if it is not already a power of two[1], so most of my
> hashmaps here are identical to if I'd just not specified a size at all. (The
> default size is 16) This is still very dumb, but not quite as dumb as it
> first appears :P.
> The startup time of the app will be slightly improved if I create the maps
> with initial size of 16 and load factor of 1, as no rehashing will
> subsequently need to take place during the static initialisers. Might as
> well do it.

I still disagree that this is a good idea. You're basically relying on undocumented implementation-specific features in a particular version or versions of Java. More below.

> When a StringBuilder overflows it must allocate a new,
> larger buffer and copy the contents into it.

Agreed.

> This is expensive.

Agreed.

> The
> performance gain from correctly choosing sizes for StringBuilders can be
> quite large.

Agreed.

> The costs of choosing stupid sizes are also potentially quite
> large.

Agreed.

> It turns out many of my choices of size were stupid - I've retuned
> them to more appropriate values. Might save a millisecond of build time :P.

"Might save a millisecond of build time" - this is exactly what is meant by "premature optimization". You don't know (a) if this actually makes a difference in practice, and (b) if it's on the critical path for the build.

I will concede that it will make things theoretically faster, and that even if you screw up the sizes, the behaviour degrades gracefully. However unless you can show that the increased maintenance headache is justified by actual real-world performance improvements, it's not worth doing. The "maintenance headache" I describe is (1) higher cognitive load when reading the code and (2) when modifying the code, the person has to re-calculate the relevant stringbuffer capacities to ensure they are appropriate, which is non-trivial. Even if they choose not to do it, they still have to spend time on that decision. (This all applies to the HashMap stuff above as well).

> > Personally I don't think there's enough classes here to warrant separating
> > them into classloader and utils packages, specially since everything is
> > public anyway. It seems unnecessarily verbose to have it this way, but I
> > don't really care.
> 
> Similarly, at some point in the dim and distant past there weren't enough
> classes in mobile/android/base to warrant splitting them into seperate
> packages. Work continued, and suddenly there was - but nobody did the, by
> that point, unpleasantly large refactoring job to tidy it up. And now look
> at the mess we have there.
> 
> Such verbosity is not necessary at the moment, but the high probability of
> feature creep makes it likely to suddenly become so. When it becomes
> necessary and you haven't had it all along you're in trouble, just like we
> are with mobile/android/base. 
> 
> When someone wants to add a new class, if there exists a package for it
> they'll almost certainly put it there. If there's just one or a small number
> of classes related to it in a top-level package they'll likely just put it
> there. Even if they do attempt to refactor those existing classes into a
> package, they'll likely have their patch rejected as being "out of scope"
> (Happened to me :P ). And thus the clutter will grow.

I don't think it's particularly useful to compare mobile/android/base with a codegen tool run at build time. I agree that for some codebases it's important to introduce this sort of design and verbosity early enough, or it collapses in a big heap. I don't think that is true of a code generator such as this one. In my experience they tend to grow very slowly and since it's not production code people are more willing to allow large sweeping changes to it, as long as the generated output isn't affected.

That being said, I'm basing my opinion based on my experience (or lack thereof) and you're basing your opinion on your experience or lack thereof. I suggest leaving the code as-is since you've already done it this way, but let's come back to this code in a few years to see if it's changed all that much.

> > I don't know exactly what the EXPORTS section implies but I don't think the
> > .h file should be exported to be include'able from other parts of the
> > codebase.
> 
> Likewise, I am unsure what this does. What I do know is that if I remove
> this then it fails to compile

Ok, we can leave it in. Some of these patches will need at least a cursory review by a build peer anyway so they can let us know if there's a better way to do it.

Comment 48

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 798661 [details] [diff] [review]
Part 3: V4 - Make use of the existing macros to simplify JNI code in all applicable places, and relocate them.

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

(In reply to Chris Kitching [:ckitching] from comment #46)
> Extracting the macros into their own file. Was this more what you were after?

Yup, exactly.

Comment 49

[Chris Kitching [:ckitching]]

Attachment: Part 6: V4 - Refactor AndroidBridge to make use of generated code.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #44)
> @@ +306,5 @@
> >      JNIEnv *env = GetJNIEnv();
> >      if (!env)
> >          return false;
> >  
> > +    jobjectArray arr = GetHandlersForMimeTypeWrapper(aMimeType, aAction);
> 
> So I might be missing something here - please let me know if I am. AIUI,
> GetHandlersForMimeTypeWrapper calls PushLocalFrame for its local variables
> and then calls PopLocalFrame with the return value, which returns it as a
> local variable in the next innermost local frame, having popped the actual
> innermost local frame. So here, "arr" is in whatever local frame existed
> before AndroidBridge::GetHandlersForMimeType was called.

That's right.
> This has two problems:
> (1) In the case of functions that are called from threads (e.g. the
> compositor thread) that are created in c++, they might not have any local
> frame at all. I don't know what happens in that scenario, but I imagine it's
> not pretty.

That can't happen - GetJNIEnv ensures that the call always happens on a particular "Correct" thread. The way this check is done is now slightly less awful as of the part 7 patch (Still need to investigate in detail why that was segfaulting. I smell another bug in there somewhere - something somewhere is calling the bridge from the wrong thread and silently failing (Less silently after my changes with an earlier version of my patch (In particular, reordering the if statements in PumpMessageLoop was segfault-inducing after it had been called very, very many times)))
I digress.

If called from the wrong thread, the GetJNIEnv function returns null and the call aborts before it can cause crazy things to happen in Java-land. You're right that doing this would be extremely bad - there "is" a frame existent at all times, but sticking local references into the lowest-level frame isn't a great idea - they exist for the remainder of your program until you explicitly delete them. They behave essentially like global references, but with the added property of being able to corrupt the stack if you make too many of them - what fun!

> (2) If AndroidBridge::GetHandlersForMimeType is called a bunch of times in a
> row, it will eventually fill up the local frame that it is being called
> with, and will overflow. I also imagine this is not pretty.
> Can you check these scenarios to make sure they are handled correctly?

This can happen. Whoops!
Two obvious solutions: 

1) Push a new stack frame for this call and pop it afterwards (Probably using AutoLocalJNIFrame)
2) Explicitly delete the local reference to arr after it is finished with (This can be done at the end of getHandlersFromStringArray) - slightly computationally cheaper, and involves less boilerplate.

I like two for its neatness. Any preferences? Got a better suggestion?

> @@ +1663,5 @@
> >      if (!win)
> >          return;
> >  
> >      CSSRect cssRect = rect / CSSToLayoutDeviceScale(win->GetDefaultScale());
> > +    AddPluginViewWrapper(view, (int)cssRect.x, (int)cssRect.y, (int)cssRect.width, (int)cssRect.height, isFullScreen);
> 
> Why the cast to ints? These should be floats all the way through.

AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.

> ::: widget/android/AndroidBridge.h
> @@ +414,5 @@
> >      // to touch it
> >      nsTArray<DelayedTask*> mDelayedTaskQueue;
> >  
> >  public:
> > +    #include "GeneratedJNIWrappers.h"
> 
> It might be worth having GeneratedJNIWrappers.h have private and public
> parts to it, so that the methods are exposed but the fields are not.

A nice idea. I'll implement this in the revised versions of parts 8 and 5.

Comment 50

[Chris Kitching [:ckitching]]

Attachment: Part 5: V2 - Add the generated code files to version control.

Cause the method/class references in generated code to have protected visibility.

Comment 51

[Chris Kitching [:ckitching]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #47)
> For the same reason that you went to the trouble of making reusable
> IterableJarLoadingURLClassLoader and the like. That is - abstraction,
> writing reusable code, etc. Currently CodeGenerator is only created and used
> from the main function but there may be a day in the future where it is not,
> or where we will want to reuse it elsewhere. That being said I'm ok with
> leaving it as-is and worrying about it when that day comes.

That day has come - I've been pondering ways to expand this solution to allow us to also generate the entry points in AndroidJavaWrappers.cpp (And, ideally, elsewhere as much as possible)
If we could do that, ProGuard would need a mere 97 annotations sprinkled through the codebase to prevent maiming. (The majority of which are in the monster that is WebRTC - it seems plausible that with excessive refactoring this could be sorted out)

In danger of inducing a mid-bug course correction...
How do we feel about changing the semantics of the annotations to be "Generate a C++ class corresponding to this Java class that wraps the methods/fields I have annotated", possibly with another annotation for just "Wrap everything for C++", for the times where we want that (eg. GeckoEvent.)
The genrated output would be comparable to some of AndroidJavaWrappers.cpp. You'd end up with neat C++ classes that directly correspond to the annotated subset of the Java class you annotated - it's JNI, minus a lot of the evilness. Most of the hard work for doing this has already been done - just need to rejig the generator to be a little more general.
Oh look, that's almost exactly what you suggested.

For now, I'm uploading a revised version of the generator that just addresses your proposed refactoring, as well as the protectedness of generated methodID members.

I'd appreciate input on the propsed expansion. It seems like this extension would allow us to do away with vastly more JNI boilerplate, and moves us even closer to being able to do Proguard (My ulterior motive here). (In fact, I suspect that with this more general wrapping-java-classes approach all our entry points can be nicely refactored.)
Suggestions on exactly how this should go would be appreciated. One generated file per wrapped class? One large file with all the wrapped classes? 

> I will concede that it will make things theoretically faster, and that even
> if you screw up the sizes, the behaviour degrades gracefully. However unless
> you can show that the increased maintenance headache is justified by actual
> real-world performance improvements, it's not worth doing. The "maintenance
> headache" I describe is (1) higher cognitive load when reading the code and
> (2) when modifying the code, the person has to re-calculate the relevant
> stringbuffer capacities to ensure they are appropriate, which is
> non-trivial. Even if they choose not to do it, they still have to spend time
> on that decision. (This all applies to the HashMap stuff above as well).

Fine. I'll remove all the extra parameters from things so people aren't troubled by thinking about them for the sake of ~20ms (Linear in the amount of code we're generating). Let's rehash those maps on startup. It seems that this process runs concurrently with dexing when building anyway - dexing takes approximately 3 times as long as this task, so we're not on the critical path on machines with >=2 CPUs.

> That being said, I'm basing my opinion based on my experience (or lack
> thereof) and you're basing your opinion on your experience or lack thereof.
> I suggest leaving the code as-is since you've already done it this way, but
> let's come back to this code in a few years to see if it's changed all that
> much.

I find myself in the curious position of having had numerous professors at uni talking at length about what you should and shouldn't do on large software projects, but virtually no experience actually working on such a project. 
In theory, theory and practice are the same. In practice, they are not.

Comment 52

[Chris Kitching [:ckitching]]

Attachment: Part 8: V3 Add an annotation processor to the build process to generate the contents of part 5

Comment 53

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Kitching [:ckitching] from comment #49)
> > (1) In the case of functions that are called from threads (e.g. the
> > compositor thread) that are created in c++, they might not have any local
> > frame at all. I don't know what happens in that scenario, but I imagine it's
> > not pretty.
> 
> That can't happen - GetJNIEnv ensures that the call always happens on a
> particular "Correct" thread. The way this check is done is now slightly less
> awful as of the part 7 patch (Still need to investigate in detail why that
> was segfaulting. I smell another bug in there somewhere - something
> somewhere is calling the bridge from the wrong thread and silently failing
> (Less silently after my changes with an earlier version of my patch (In
> particular, reordering the if statements in PumpMessageLoop was
> segfault-inducing after it had been called very, very many times)))
> I digress.
> 
> If called from the wrong thread, the GetJNIEnv function returns null and the
> call aborts before it can cause crazy things to happen in Java-land. You're
> right that doing this would be extremely bad - there "is" a frame existent
> at all times, but sticking local references into the lowest-level frame
> isn't a great idea - they exist for the remainder of your program until you
> explicitly delete them. They behave essentially like global references, but
> with the added property of being able to corrupt the stack if you make too
> many of them - what fun!

What about functions that call GetJNIForThread? So for example the ProvideEGLSurface() function.

> > (2) If AndroidBridge::GetHandlersForMimeType is called a bunch of times in a
> > row, it will eventually fill up the local frame that it is being called
> > with, and will overflow. I also imagine this is not pretty.
> > Can you check these scenarios to make sure they are handled correctly?
> 
> This can happen. Whoops!
> Two obvious solutions: 
> 
> 1) Push a new stack frame for this call and pop it afterwards (Probably
> using AutoLocalJNIFrame)
> 2) Explicitly delete the local reference to arr after it is finished with
> (This can be done at the end of getHandlersFromStringArray) - slightly
> computationally cheaper, and involves less boilerplate.
> 
> I like two for its neatness. Any preferences? Got a better suggestion?

Both of these would have to be done manually, right? And if we go with two, it's always at most one reference (the return value) that will need to be deleted, right? If that is the case, then two sounds fine to me. If we had possibly more things that needed to be deleted then I would lean towards option 1 because it is safer.

Comment 54

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 798792 [details] [diff] [review]
Part 8: V3 Add an annotation processor to the build process to generate the contents of part 5

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

Thanks!

Comment 55

[Chris Kitching [:ckitching]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #53)
> What about functions that call GetJNIForThread? So for example the
> ProvideEGLSurface() function.

Ah. I'll provide a revised patch addressing these.

> Both of these would have to be done manually, right? And if we go with two,
> it's always at most one reference (the return value) that will need to be
> deleted, right? If that is the case, then two sounds fine to me. If we had
> possibly more things that needed to be deleted then I would lean towards
> option 1 because it is safer.

Yes, me too.

Comment 56

[Chris Kitching [:ckitching]]

(In reply to Chris Kitching [:ckitching] from comment #55)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #53)
> > What about functions that call GetJNIForThread? So for example the
> > ProvideEGLSurface() function.
> 
> Ah. I'll provide a revised patch addressing these.

(To clarify: Address by ensuring reference deletion if there's only one, and pushing a frame where there's more than one. When you get the JNIEnv, you've implicitly got yourself access to whatever local java frame is in scope in the execution context represented by that environment pointer. The problem is that the current implementation risks overflowing it)

Comment 57

[Chris Kitching [:ckitching]]

Attachment: Part 6: V5 - Refactor AndroidBridge to make use of generated code.

Comment 58

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 799147 [details] [diff] [review]
Part 6: V5 - Refactor AndroidBridge to make use of generated code.

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

::: widget/android/AndroidBridge.cpp
@@ +295,5 @@
>          aHandlersArray->AppendElement(app, false);
>          if (aDefaultApp && isDefault.Length() > 0)
>              *aDefaultApp = app;
>      }
> +    aJNIEnv->DeleteLocalRef(jArr);

I would prefer to move this line to the caller functions. I would not expect a helper method such as this one to have destructive side effects on an argument like this. The caller functions "own" the jobjectArray and should be responsible for destroying it.

@@ +368,2 @@
>      nsJNIString jniStr(jstrType, env);
>      CopyUTF16toUTF8(jniStr.get(), aMimeType);

Doesn't jstrType need to be DeleteLocalRef'd around here somewhere too? Ditto for some of the other functions below. There are some arrays that are also not deleted.

Comment 59

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 799147 [details] [diff] [review]
Part 6: V5 - Refactor AndroidBridge to make use of generated code.

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

Minusing to clear my queue for now but if I missed something here please let me know and I'll look at it again.

Comment 60

[Chris Kitching [:ckitching]]

Attachment: Part 6: V6 - Refactor AndroidBridge to make use of generated code.

Well spotted. Okay, fixed those and a few similar problems. Locals refs now being killed off at the right times, without strange helper function behavoiur.

Unfortunately, the delay caused other people to bitrot some of the earlier patches - I'll now upload unbitrot'd editions of these (Most interestingly, a new string conversion has become necessary in part 4.)

Comment 61

[Chris Kitching [:ckitching]]

Attachment: Part 4: V4 - Consistently pass nsAString into the JNI methods.

Unbitrot, add the extra conversion to DownloadPlatform.

Comment 62

[Chris Kitching [:ckitching]]

Attachment: Part 7: V2 - Storing a void* for mThread instead of a pthread_t is both nonportable and dangerous.

Trivial unbitrot.

Comment 63

[Chris Kitching [:ckitching]]

Just in case any of the review-driven tweaks were poorly applied:
https://tbpl.mozilla.org/?tree=Try&rev=11649e1c11ad

Comment 64

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 798792 [details] [diff] [review]
Part 8: V3 Add an annotation processor to the build process to generate the contents of part 5

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

I'd like a build peer to also sign off on the Makefile changes here.

Comment 65

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 799969 [details] [diff] [review]
Part 6: V6 - Refactor AndroidBridge to make use of generated code.

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

I found some more things that need deletion. This is tricky - almost there!

::: widget/android/AndroidBridge.cpp
@@ +383,5 @@
>      if (!env)
>          return;
>  
> +    jstring jstrExt = GetExtensionFromMimeTypeWrapper(NS_ConvertUTF8toUTF16(aMimeType));
> +    if (jstrExt == nullptr) {

nit: !jstrExt

Actually maybe as a follow-up cleanup patch (on this or some other bug) we should define a macro that takes the returned string, creates the nsJNIString and copies it to the out-param, and deletes the local ref. This pattern happens a bunch of times in this file and would be cleaner. It's a small refactoring though so I don't want this entire patchset to get bitrotted just because of that.

@@ +418,5 @@
>  {
>      ALOG_BRIDGE("AndroidBridge::ClipboardHasText");
>  
> +    if (GetClipboardTextWrapper()) {
> +        return true;

This function is still returning a jstring which needs to be deleted.

@@ +606,5 @@
>          return;
>  
>      AutoLocalJNIFrame jniFrame(env);
>  
> +    jintArray arr = GetSystemColoursWrapper();

arr needs deletion

@@ +644,5 @@
>          return;
>  
>      AutoLocalJNIFrame jniFrame(env);
>  
> +    jbyteArray arr = GetIconForExtensionWrapper(NS_ConvertUTF8toUTF16(aFileExt), aIconSize);

ditto

@@ +1000,5 @@
>      if (!env)
>          return false;
>  
>      AutoLocalJNIFrame jniFrame(env);
> +    jintArray arr = InitCameraWrapper(NS_ConvertUTF8toUTF16(contentType), (int32_t) camera, (int32_t) width, (int32_t) height);

ditto

@@ +1031,5 @@
>      AutoLocalJNIFrame jniFrame(env);
>  
>      // To prevent calling too many methods through JNI, the Java method returns
>      // an array of double even if we actually want a double and a boolean.
> +    jdoubleArray arr = GetCurrentBatteryInformationWrapper();

ditto

@@ +1062,1 @@
>          return;

returnMessage needs deletion

@@ +1244,2 @@
>  
> +    jdoubleArray arr = GetCurrentNetworkInformationWrapper();

ditto

@@ +1631,5 @@
>      if (!env)
>          return;
>  
>      AutoLocalJNIFrame jniFrame(env);
> +    jstring jstrRet = GetGfxInfoDataWrapper();

ditto

@@ +1652,5 @@
>      if (!env)
>          return NS_ERROR_FAILURE;
>  
>      AutoLocalJNIFrame jniFrame(env);
> +    jstring jstrRet = GetProxyForURIWrapper(NS_ConvertUTF8toUTF16(aSpec),

ditto

@@ +1705,5 @@
>          return false;
>  
>      AutoLocalJNIFrame jniFrame(env);
>  
> +    jstring jstrThreadName = GetThreadNameJavaProfilingWrapper(aThreadId);

ditto

Comment 66

[Chris Kitching [:ckitching]]

Attachment: Part 6: V7 - Refactor AndroidBridge to make use of generated code.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #65)
> Actually maybe as a follow-up cleanup patch (on this or some other bug) we
> should define a macro that takes the returned string, creates the
> nsJNIString and copies it to the out-param, and deletes the local ref. This
> pattern happens a bunch of times in this file and would be cleaner. It's a
> small refactoring though so I don't want this entire patchset to get
> bitrotted just because of that.

Aye. Having done this work it's become clear that we can make more progress in this direction. That said, I think the patches here have reached "Big enough" - can do the other stuff in followup bugs.

> @@ +418,5 @@
> >  {
> >      ALOG_BRIDGE("AndroidBridge::ClipboardHasText");
> >  
> > +    if (GetClipboardTextWrapper()) {
> > +        return true;
> 
> This function is still returning a jstring which needs to be deleted.

Well spotted. Although the fix is sort of a mess, alas.

> 
> @@ +606,5 @@
> >          return;
> >  
> >      AutoLocalJNIFrame jniFrame(env);
> >  
> > +    jintArray arr = GetSystemColoursWrapper();
> 
> arr needs deletion

This will be cleaned up by the AutoLocalJNIFrame in its destructor when the local frame gets popped. (Check out the class definition in AndroidBridge.h. No need to explicitly delete local refs when the frame in which they live is covered by an AutoLocalJNIFrame that goes out of scope at the same time that we are finished with the local variables themselves.)

> @@ +644,5 @@
> >          return;
> >  
> >      AutoLocalJNIFrame jniFrame(env);
> >  
> > +    jbyteArray arr = GetIconForExtensionWrapper(NS_ConvertUTF8toUTF16(aFileExt), aIconSize);
> 
> ditto

ditto

> @@ +1000,5 @@
> >      if (!env)
> >          return false;
> >  
> >      AutoLocalJNIFrame jniFrame(env);
> > +    jintArray arr = InitCameraWrapper(NS_ConvertUTF8toUTF16(contentType), (int32_t) camera, (int32_t) width, (int32_t) height);
> 
> ditto

ditto

> @@ +1031,5 @@
> >      AutoLocalJNIFrame jniFrame(env);
> >  
> >      // To prevent calling too many methods through JNI, the Java method returns
> >      // an array of double even if we actually want a double and a boolean.
> > +    jdoubleArray arr = GetCurrentBatteryInformationWrapper();
> 
> ditto

ditto

> @@ +1062,1 @@
> >          return;
> 
> returnMessage needs deletion
> 
> @@ +1244,2 @@
> >  
> > +    jdoubleArray arr = GetCurrentNetworkInformationWrapper();
> 
> ditto

ditto
 
> @@ +1631,5 @@
> >      if (!env)
> >          return;
> >  
> >      AutoLocalJNIFrame jniFrame(env);
> > +    jstring jstrRet = GetGfxInfoDataWrapper();
> 
> ditto

ditto

> @@ +1652,5 @@
> >      if (!env)
> >          return NS_ERROR_FAILURE;
> >  
> >      AutoLocalJNIFrame jniFrame(env);
> > +    jstring jstrRet = GetProxyForURIWrapper(NS_ConvertUTF8toUTF16(aSpec),
> 
> ditto

ditto

> @@ +1705,5 @@
> >          return false;
> >  
> >      AutoLocalJNIFrame jniFrame(env);
> >  
> > +    jstring jstrThreadName = GetThreadNameJavaProfilingWrapper(aThreadId);
> 
> ditto

ditto

Comment 67

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 800293 [details] [diff] [review]
Part 6: V7 - Refactor AndroidBridge to make use of generated code.

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

Heh whoops. Sorry about that, I totally missed the AutoLocalJNIFrame creations. :/

::: widget/android/AndroidBridge.cpp
@@ +426,4 @@
>  
> +    jstring jStr = GetClipboardTextWrapper();
> +    bool ret = jStr;
> +    env->DeleteLocalRef(jStr);

I guess this one is redundant now since there is also an AutoLocalJNIFrame.

Comment 68

[Chris Kitching [:ckitching]]

Attachment: Part 6: V8 - Refactor AndroidBridge to make use of generated code.

Killed off the redundant deletion.

Comment 69

[Chris Kitching [:ckitching]]

Victory! 

I'll fille the proposed expansions as a followup.

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8a559f4f741
https://hg.mozilla.org/integration/mozilla-inbound/rev/83220e4b96ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/402235f97f68
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd937e69cd40
https://hg.mozilla.org/integration/mozilla-inbound/rev/78aa104740c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e49ee30a8e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/130635e9a2c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea578c504f72

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f8a559f4f741
https://hg.mozilla.org/mozilla-central/rev/83220e4b96ec
https://hg.mozilla.org/mozilla-central/rev/402235f97f68
https://hg.mozilla.org/mozilla-central/rev/dd937e69cd40
https://hg.mozilla.org/mozilla-central/rev/78aa104740c1
https://hg.mozilla.org/mozilla-central/rev/6e49ee30a8e1
https://hg.mozilla.org/mozilla-central/rev/130635e9a2c1
https://hg.mozilla.org/mozilla-central/rev/ea578c504f72

Comment 72

[Kartikaya Gupta (email:[e-mail])]

\o/ nice!

(now we just have to wait for the talos regressions to come crawling out...)

Comment 73

[Chris Kitching [:ckitching]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #72)
> \o/ nice!
> 
> (now we just have to wait for the talos regressions to come crawling out...)

I am skeptical that Talos has the precision required to detect the trivial performance costs associated with this patch. (The extra security checks in the generated code are very much dwarfed by the JNI call overheads)

If this does happen, we can almost certainly win by optimising PumpMessageLoop.

Also, alas, another gargantum patch series will be appearing soon in Bug 913985. But at least we made a start - one step closer to Proguard not being impossible!

Comment 74

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #53)
> Both of these would have to be done manually, right? And if we go with two,
> it's always at most one reference (the return value) that will need to be
> deleted, right? If that is the case, then two sounds fine to me. If we had
> possibly more things that needed to be deleted then I would lean towards
> option 1 because it is safer.

Whoops, bad call on my part. Should have gone with the safer approach, see bug 945327.