You may have noticed Proguard printing a bunch of warnings like this: 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Tabs { void closeTab(org.mozilla.gecko.Tab); }', but not the descriptor class 'org.mozilla.gecko.Tab' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void startUISession(org.mozilla.gecko.TelemetryContract$Session,java.lang.String); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Session' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void startUISession(org.mozilla.gecko.TelemetryContract$Session); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Session' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void stopUISession(org.mozilla.gecko.TelemetryContract$Session,java.lang.String,org.mozilla.gecko.TelemetryContract$Reason); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Session' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void stopUISession(org.mozilla.gecko.TelemetryContract$Session,java.lang.String,org.mozilla.gecko.TelemetryContract$Reason); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Reason' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void stopUISession(org.mozilla.gecko.TelemetryContract$Session,org.mozilla.gecko.TelemetryContract$Reason); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Session' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void stopUISession(org.mozilla.gecko.TelemetryContract$Session,org.mozilla.gecko.TelemetryContract$Reason); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Reason' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void stopUISession(org.mozilla.gecko.TelemetryContract$Session,java.lang.String); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Session' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void stopUISession(org.mozilla.gecko.TelemetryContract$Session); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Session' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { java.lang.String getSessionName(org.mozilla.gecko.TelemetryContract$Session,java.lang.String); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Session' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(java.lang.String,org.mozilla.gecko.TelemetryContract$Method,long,java.lang.String); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Method' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event,org.mozilla.gecko.TelemetryContract$Method,long,java.lang.String); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Event' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event,org.mozilla.gecko.TelemetryContract$Method,long,java.lang.String); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Method' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event,org.mozilla.gecko.TelemetryContract$Method,long); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Event' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event,org.mozilla.gecko.TelemetryContract$Method,long); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Method' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event,org.mozilla.gecko.TelemetryContract$Method,java.lang.String); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Event' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event,org.mozilla.gecko.TelemetryContract$Method,java.lang.String); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Method' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event,org.mozilla.gecko.TelemetryContract$Method); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Event' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event,org.mozilla.gecko.TelemetryContract$Method); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Method' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Event' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.Telemetry { void sendUIEvent(org.mozilla.gecko.TelemetryContract$Event,boolean); }', but not the descriptor class 'org.mozilla.gecko.TelemetryContract$Event' 0:38.75 Note: the configuration keeps the entry point 'org.mozilla.gecko.background.healthreport.upload.AndroidSubmissionClient$SubmissionsFieldName { int getID(org.mozilla.gecko.background.healthreport.HealthReportStorage); }', but not the descriptor class 'org.mozilla.gecko.background.healthreport.HealthReportStorage' These are, it turns out, a slightly scary oddity of Proguard. At the moment, our Proguard configuration says "Keep any entry points that we've annotated (plus a few special android-specific ones that aren't interesting to discuss here)". That's what all those @JNITarget annotations are: requests to have Proguard not delete important parts of our app. Unfortunately, my recent work attempting to enable Proguard's obfuscator (Bug 1041852) produced an interesting result. The obfuscator, since it tries to rename absolutely *everything*, will of course immediately break anything that isn't properly protected by Proguard's configuration (as opposed to the optimisation step, that merely destroys things if it finds it beneficial to do so). What I discovered is that, when you tell Proguard to keep an entry point, for example: public synchronized Tab getTab(int id); Proguard doesn't promise not to rename or delete the "Tab" class. As a result, if you run the obfuscator Tab gets renamed to something short, breaking JNI/Reflective access to this entry point. This is a really strange feature of Proguard, since it means that the "keep" directive doesn't *really* keep. I posted on Stack Overflow about this and got a reply by the developer of Proguard, who told me that "It used to keep return types, but developers found it confusing so I took it out": http://stackoverflow.com/questions/24882343/how-to-stop-proguard-from-deleting-return-types There are no words. These warnings, then, are effectively saying "I've detected that you want to keep an entry point that relies on class X but I'm not going to promise not to rename it unless you explicitly add a keep directive for this class, too". We're unlikely to suddenly encounter a problem due to this if we continue to use just the optimiser (and not the obfuscator), unless we start having return types which are classes only used from JNI (so Proguard will wish to delete them). This does, however, block any future attempt to save binary size with the obfuscator, as well as representing something of a danger we need to be aware of should we someday start getting mystery NoSuchMethodExceptions. The good news is an obvious and fairly straightforward solution to this problem exists: use an annotation processor to scan all the annotated elements we want to keep, find their return types, and spit out the explicit Proguard configuration necessary to give the annotations the behaviour that makes some sort of sense.

Augment the new annotation processor with the ability to generate Proguard configuration. This fixes all the scary descriptor class messages, so we should be safe to run the obfuscator from here (sans crashreport parsing madness). The output looks something like: https://pastebin.mozilla.org/6531637 . There is duplication (particularly of return types), but Proguard ignores duplicate directives and there seems to be no point adding the extra cost (and compute time) to ensure deduplication.

Comment on attachment 8491133 [details] [diff] [review] Generate proguard configuration to avoid Proguard bug Review of attachment 8491133 [details] [diff] [review]: ----------------------------------------------------------------- This is looking pretty great. I have two asks: 1) Separate the changes to the AP from the meet of this patch. It can be a pre, but it would be better to just fold into the AP-rewrite ticket. 2) Recognize that this processor will be the model for the next one. Think about making the code in AnnotationProcess.java and ProguardConfigGenerator.java even more verbose/explanatory to help the next person. You're most of the way there. I'd like to see an updated version, on top of everything else, and a shiny green try build. I'll want to kick the tires so that I understand the build system integration better. And I really hope we're not painting ourselves into tight corners when it comes to doing this magic in Gradle. Speaking of, what happens if you compile with the -annotation processing lines to javac? Do we silently succeed? Fail in a blaze of glory? If we fail in a blaze, we'll need to understand what our Eclipse/Gradle story looks like going forward. ::: mobile/android/annotations/definitions/WebRTCJNITarget.java @@ +1,3 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ nit: don't kill the newline, plz. ::: mobile/android/annotations/definitions/generatorannotations/OptionalGeneratedParameter.java @@ +16,5 @@ > * The default values are zero for numerical types, false for booleans, "" for strings, and null > * for all other reference types. > */ > +@Target(ElementType.PARAMETER) > +@Retention(RetentionPolicy.SOURCE) Would anybody be upset if we didn't support optional parameters? Would it simplify things? ::: mobile/android/annotations/processors/AnnotationProcessor.java @@ +35,5 @@ > > /** > * An annotation processor to generate the C++ code wrapping Java methods that are used as entry > + * points from the android bridge, as well as to generate Proguard config. > + * If the omitRobocop option is set, the generated Proguard config does not keep entites annotated sp: entites should be entities. @@ +42,2 @@ > */ > +@SupportedOptions("omitRobocop") Why bother? I'd like to see this die, but not as part of this sequence of things. @@ +280,5 @@ > + private boolean finishAnnotationProcessing(boolean b) { > + try { > + proguardThread.join(); > + } catch (InterruptedException e) { > + e.printStackTrace(); This is an error, right? Shouldn't we do something stronger? And return failure? @@ +398,5 @@ > for (Element enclosedElement : enclosedElements) { > accumulateChildElements(enclosedElement, targetList); > } > + } else if (kind != ElementKind.ENUM_CONSTANT) { > + System.out.println("Warning: Generator ignoring in-scope element: " + element + " of unsupported kind " + kind); Things like this weird me out. Why didn't this cause problems in the original translation to the AP? ::: mobile/android/annotations/processors/ProguardConfigGenerator.java @@ +23,5 @@ > +import java.util.List; > +import java.util.Set; > + > +public class ProguardConfigGenerator implements Runnable { > + private static final String PROGUARD_INCLUDES_FILE = "proguard_keeps.cfg"; Is it possible to get this from the environment? Is this file touched every javac compilation? It's really a part of the input to Proguard, so if it's dirty, we should re-run Proguard. I think your LockedIndexed whatever protects against spurious writes, but I want to be clear. @@ +32,5 @@ > + StringBuilder output = new StringBuilder(); > + > + // We tend to find these as we go along, and it's too much hassle to fit them in the right > + // place, so this buffer accumulates them and we glue them on the end when we're done. > + StringBuilder returnTypesBuffer = new StringBuilder(); This seems like the wrong data structure. What stops repeated return types? Why is this not a set? @@ +56,5 @@ > + > + output.append(returnTypesBuffer); > + > + // Write output. > + try (LockedIndexedRandomAccessFile outfile = new LockedIndexedRandomAccessFile(PROGUARD_INCLUDES_FILE, "", "", "#", "#", "# ")) { These don't seem like good sentinels. # START and # FINISH? Something else? @@ +84,5 @@ > + output.append("}\n\n"); > + } else { > + // A class without any elements at this point generally indicates an actually empty > + // class in the program that we for some reason are trying to keep. > + System.err.println("Warning: Empty class: " + targetClass.wrappedClass); Let me know what you're doing. "Keeping empty class...". ::: mobile/android/annotations/processors/utils/io/GuideTree.java @@ +11,5 @@ > + String start; > + String end; > + String prefix; > + > + public GuideTree(String headerStart, String headerEnd, String indexPrefix) { Any reason to not fold these incremental improvements into the main rewriting ticket? Or into a different part of this one? The more you split things, the easier it is to review them. ::: mobile/android/annotations/processors/utils/io/LockedIndexedRandomAccessFile.java @@ +35,5 @@ > + private final String indexStart; > + private final String indexEnd; > + private final String indexPrefix; > + > + public LockedIndexedRandomAccessFile(File file, String header, String footer, String indexStart, String indexEnd, String indexPrefix) throws IOException { Ditto: pre part, or into the other ticket. ::: mobile/android/annotations/processors/utils/tree/ElementToProcess.java @@ +64,5 @@ > } > > if (wrapAnno == null) { > // If no annotation found, we might be expected to generate anyway using default > + // arguments. Fold this into the other ticket. ::: mobile/android/annotations/processors/utils/tree/Utils.java @@ +643,5 @@ > > return ret; > } > > + /** This kind of answers a question I had in the other ticket. Some tests that provide examples of the issues you're handling would go a long way to improving my confidence in this code. ::: mobile/android/base/Makefile.in @@ +144,5 @@ > .proguard.deps: $(ALL_JARS) > $(REPORT_BUILD) > @$(TOUCH) $@ > java -jar $(ANDROID_SDK_ROOT)/tools/proguard/lib/proguard.jar \ > + @$(topsrcdir)/mobile/android/config/proguard.cfg \ Kind of looks like the indentation changed here, but it might just be BZ. @@ +145,5 @@ > $(REPORT_BUILD) > @$(TOUCH) $@ > java -jar $(ANDROID_SDK_ROOT)/tools/proguard/lib/proguard.jar \ > + @$(topsrcdir)/mobile/android/config/proguard.cfg \ > + @proguard_keeps.cfg \ Could we make this proguard_generated.cfg or something? It's not just things to keep; we might include things to delete, etc. ::: mobile/android/config/proguard.cfg @@ -110,5 @@ > -optimizations !class/merging/horizontal > -optimizations !class/merging/vertical > > -# Keep miscellaneous targets. > - \o/ ::: widget/android/GeneratedJNIWrappers.cpp @@ +1,2 @@ > /* > + * +InitStubs:688 Mmm, I see, I like it. Ditto: re folding.

(In reply to Nick Alexander :nalexander from comment #3) > 1) Separate the changes to the AP from the meet of this patch. It can be a > pre, but it would be better to just fold into the AP-rewrite ticket. hg qcrecord to the rescue! > 2) Recognize that this processor will be the model for the next one. Think > about making the code in AnnotationProcess.java and > ProguardConfigGenerator.java even more verbose/explanatory to help the next > person. You're most of the way there. I could always (probably in another ticket) refactor this as a nice extensible platform for annotation processor writing (one that avoids the need for writing another actual annotation processor, but instead has you write extra modules that plug into the one we have to do arbitrary tasks... if that makes sense). I wrote such a mechanism for my optimiser: the API was that you provide Runnable-like things to be run at certain phases during javac's execution, avoiding the need for the boilerplate. If you like, I could provide that (or, more likely, a cut-down annotation-processing-only version of it). ... Of course, this needs more documentation, too, but I want to try and avoid people's default behaviour being what happened over in the other generator ticket where someone essentially copy-pasted the code from AnnotationProcessor.java to do what they wanted. That someone feels the need to do that indicates that the architecture isn't sufficiently nice to extend yet. > I'd like to see an updated version, on top of everything else, and a shiny > green try build. I'll want to kick the tires so that I understand the build > system integration better. And I really hope we're not painting ourselves > into tight corners when it comes to doing this magic in Gradle. Speaking > of, what happens if you compile with the -annotation processing lines to > javac? Do we silently succeed? Fail in a blaze of glory? If we fail in a > blaze, we'll need to understand what our Eclipse/Gradle story looks like > going forward. I am yet to try this. I very much expect this to "magically just work". > ::: > mobile/android/annotations/definitions/generatorannotations/ > OptionalGeneratedParameter.java > @@ +16,5 @@ > > * The default values are zero for numerical types, false for booleans, "" for strings, and null > > * for all other reference types. > > */ > > +@Target(ElementType.PARAMETER) > > +@Retention(RetentionPolicy.SOURCE) > > Would anybody be upset if we didn't support optional parameters? Would it > simplify things? A wee bit. The amount of simplification it allows is pretty negligible - there's a mapping from datatypes to their default values, plus a bit of logic that decides if we want to append " = $default_value" when we're generating the parameters. I'll file a followup if it turns out to be straightforward to exterminate. A possible danger of removing it is that people will start writing manual wrappers around the generated code to get a nicer API in C++-land. That's a possible argument against removing the feature. > @@ +42,2 @@ > > */ > > +@SupportedOptions("omitRobocop") > > Why bother? I'd like to see this die, but not as part of this sequence of > things. Because it was about 30 seconds of work for me to add now, but will be more expensive to do in the future when the code isn't in my working memory (or if someone less familiar with it has to do it). It's a fairly low-complexity change to a relatively complex part of the program. Something with an annoyingly high "read-in" factor to implement, so I might as well bolt it on now if we're ever plausibly going to want it. The changes are almost entirely constrained to the input-aggregation phase. In case it wasn't clear what the use-case for this was, the idea isn't to exterminate @RobocopTarget annotations entirely (though that would be a worthy goal), the idea is that this can be used to perform an optimised release build that strips @RobocopTarget symbols. You just tack '-AomitRobocop' onto your javac command and bam, Proguard now ignores the @RobocopTarget annotations. You could then have a testing build with @RobocopTargets preserved, and a more efficient release build that has them stripped out. I thought I read somewhere that this was something you'd been considering doing, so I solved the Java-ey side of the problem for you. All you need to do is solve the political, build-system, and do-we-even-want-to-do-this-anyway parts. :P > @@ +398,5 @@ > > for (Element enclosedElement : enclosedElements) { > > accumulateChildElements(enclosedElement, targetList); > > } > > + } else if (kind != ElementKind.ENUM_CONSTANT) { > > + System.out.println("Warning: Generator ignoring in-scope element: " + element + " of unsupported kind " + kind); > > Things like this weird me out. Why didn't this cause problems in the > original translation to the AP? This chunk definitely should be in the other patch. I blended the two together horribly. Cleaned up patches will be available shortly. This warning is mostly a WTF condition. You'll probably never see it fire until you increase language level again and start seeing new, exotic element types. > ::: mobile/android/annotations/processors/ProguardConfigGenerator.java > @@ +23,5 @@ > > +import java.util.List; > > +import java.util.Set; > > + > > +public class ProguardConfigGenerator implements Runnable { > > + private static final String PROGUARD_INCLUDES_FILE = "proguard_keeps.cfg"; > > Is it possible to get this from the environment? It is. So should we do that, or just rename it to something like proguard_generated.cfg (as you suggested later on)? I guess both? > Is this file touched every javac compilation? It's really a part of the > input to Proguard, so if it's dirty, we should re-run Proguard. I think > your LockedIndexed whatever protects against spurious writes, but I want to > be clear. Remember: Annotation processors run *during* javac's execution. (Remember that the pre-Bug 1067217 "Annotation processor" *wasn't* an annotation processor. I should never have named it that, because it just confuses everyone now I'm trying to convert it. :P ) What's actually happening here is javac is launching, lexing, parsing, and then running our processor. It's not even done type-checking yet and it's handing us bits of the AST to do annotation processing on. Proguard isn't run until we've finished all the javac executions, so we're going to have finished writing to the generated configuration file before Proguard runs, so this is a non-issue. > @@ +32,5 @@ > > + StringBuilder output = new StringBuilder(); > > + > > + // We tend to find these as we go along, and it's too much hassle to fit them in the right > > + // place, so this buffer accumulates them and we glue them on the end when we're done. > > + StringBuilder returnTypesBuffer = new StringBuilder(); > > This seems like the wrong data structure. What stops repeated return types? > Why is this not a set? To avoid the breathtakingly idiotic way javac handles concatenation. It's just a performance optimisation, since duplicates don't matter to Proguard - telling it to preserve something twice has no effect on how it behaves. We'd insert things like this: returnTypesSet.add("-keep class " + descriptorName + '\n'); unfortunately, that is translated by the compiler to something equivalent to: StringBuilder temp = new StringBuilder("-keep class "); temp.append(descriptorName).append('\n'); returnTypesSet.add(temp.toString()); See how we're allocating a StringBuilder for each insertion to the set? Then we end up making yet another StringBuilder at the end to run through the set and build our actual output. On top of all that, we have the hashCode calculations (and maybe an occasional equality check) internal to the set to ensure element uniqueness. So many buffers... By managing the StringBuilder ourselves in the way I did avoids such hidden complexity. There's one buffer, we add to it, all is happy. This ridiculous behaviour is also the reason why you should never pass a concatenation as the argument of StringBuilder.append - you end up using a StringBuilder to append to a StringBuilder. If you want to make things more human readable then I can translate it to a set, of course, but it seems to look okay as-is. The return-type blob is always at the end, so it doesn't mess things up too badly: https://pastebin.mozilla.org/6858649 > @@ +56,5 @@ > > + > > + output.append(returnTypesBuffer); > > + > > + // Write output. > > + try (LockedIndexedRandomAccessFile outfile = new LockedIndexedRandomAccessFile(PROGUARD_INCLUDES_FILE, "", "", "#", "#", "# ")) { > > These don't seem like good sentinels. # START and # FINISH? Something else? Could do if you want. I think it looks fine as it is: it manifests as a blank comment line to either side of the index. See exemplar output: https://pastebin.mozilla.org/6858649 > ::: mobile/android/annotations/processors/utils/io/GuideTree.java > @@ +11,5 @@ > > + String start; > > + String end; > > + String prefix; > > + > > + public GuideTree(String headerStart, String headerEnd, String indexPrefix) { > > Any reason to not fold these incremental improvements into the main > rewriting ticket? Or into a different part of this one? The more you split > things, the easier it is to review them. Merely incompetence ;) > ::: > mobile/android/annotations/processors/utils/io/LockedIndexedRandomAccessFile. > java > @@ +35,5 @@ > > + private final String indexStart; > > + private final String indexEnd; > > + private final String indexPrefix; > > + > > + public LockedIndexedRandomAccessFile(File file, String header, String footer, String indexStart, String indexEnd, String indexPrefix) throws IOException { > > Ditto: pre part, or into the other ticket. Not least because the other ticket is *deeply broken* without this output mechanism.

NB: The exemplar output appears to have been truncated my the way I pasted it. Don't panic, the generator doesn't produce syntactically-invalid Proguard configuration. (At least, not like that)

Comment on attachment 8510047 [details] [diff] [review] Generate proguard configuration to avoid Proguard bug Review of attachment 8510047 [details] [diff] [review]: ----------------------------------------------------------------- ckitching: I'm closing these reviews because we've both lost energy for this project. Sorry that I was some of that stop energy :( I would like to pick this up again and hope your strong work doesn't die on the vine.