Closed Bug 913985 Opened 6 years ago Closed 6 years ago

Automagically generate C++ classes wrapping Java classes.

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
fennec + ---

People

(Reporter: ckitching, Assigned: ckitching)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 53 obsolete files)

232.61 KB, patch
Details | Diff | Splinter Review
142.01 KB, patch
Details | Diff | Splinter Review
58.72 KB, patch
Details | Diff | Splinter Review
4.20 KB, patch
Details | Diff | Splinter Review
26.70 KB, patch
Details | Diff | Splinter Review
92.71 KB, patch
Details | Diff | Splinter Review
15.65 KB, patch
Details | Diff | Splinter Review
Followup to Bug 794981.

The JNI is annoying. To use it, we end up writing an awful lot of fragile, boring, repetative boilerplate code to make the calls into Java from C++.
Additionally, Proguard has a tendancy to delete the things we're trying to call from C++, because it thinks they're dead code - hence why we're not currently using Proguard.

Bug 794981 made a start by generating at compile time the boring wrapper functions from AndroidBridge.cpp from annotations on Java methods. You annotate the Java methods you want access to from C++, and it generates appropriate boilerplate to wrap them for you.

The problem is this solution is not very general. There exist a large number of nongenerated entry points from C++ into Java - an awful lot more of this horrid boilerplate - which are not addressed by the work from that bug.
The objective of this bug is to make the appropriate modifications to the generator added in Bug 794981 so that, instead of just generating wrapper functions in a fixed location of a fixed C++ class for annotated methods, it instead generates wrapper classes, similar to those found in AndroidJavaWrappers.cpp/h, which have a 1-1 correspondance with Java classes.

That way, you just need to tag the Java classes you want to talk to from C++, tag the fields/methods you want access to in C++, and the generator will spit out appropriate code for you to use, thus abstracting away most of the JNI and preventing programmers from making mistakes writing the boilerplate code.
As an added bonus, if this is applied to every entry point from C++ to Java, will will conveniently find ourselves in a position where every Java entry point from C++ has an annotation on it. This would make application of Proguard to our code trivial (Pending appropriate magic to make Robocop stop being stupid).
Summary: Automagiclly generally C++ classes wrapping Java classes. → Automagiclly generate C++ classes wrapping Java classes.
Summary: Automagiclly generate C++ classes wrapping Java classes. → Automagically generate C++ classes wrapping Java classes.
Blocks: Proguard
Assignee: nobody → ckitching
Hardware: ARM → All
The new annotation API allows you to tag individual members of a Java class for wrapping in a C++ class, or to just tag the entire class for wrapping.
Attachment #806230 - Flags: review?(bugmail.mozilla)
This new, more general scheme allows us to generate a bunch more things - let's tag all of those, too.
Attachment #806231 - Flags: review?(bugmail.mozilla)
The new annoataion processor.
Attachment #806233 - Flags: review?(bugmail.mozilla)
Attached patch Part 4: The new generated code. (obsolete) — Splinter Review
The new generated code.
Attachment #806234 - Flags: review?(bugmail.mozilla)
The juicy part - Rejig the C++ code to make use of this new generated code.

End of patch series.

Try run of the whole shaboodle: https://tbpl.mozilla.org/?tree=Try&rev=5d030abeaf61

Enjoy.
Attachment #806237 - Flags: review?(bugmail.mozilla)
Comment on attachment 806231 [details] [diff] [review]
Part 2: Annotate the new things that we can now generate.

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

::: mobile/android/base/GeckoEvent.java
@@ +23,5 @@
>  import android.util.Log;
>  import android.view.KeyEvent;
>  import android.view.MotionEvent;
> +import org.mozilla.gecko.mozglue.generatorannotations.GeneratorOptions;
> +import org.mozilla.gecko.mozglue.generatorannotations.WrapEntireClassForJNI;

These imports need to move up. https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Java_practices

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +259,1 @@
>      private void setPageRect(RectF rect, RectF cssRect) {

This function is not called from JNI

@@ +411,5 @@
>          return mDisplayPort;
>      }
>  
>      /* This is invoked by JNI on the gecko thread */
> +    @WrapElementForJNI

Remove the comment about being invoked by JNI, the annotation makes it obsolete. Same goes for many of the ones in this file.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +25,5 @@
>  import android.opengl.GLES20;
>  import android.os.SystemClock;
>  import android.util.Log;
> +import org.mozilla.gecko.mozglue.generatorannotations.GeneratorOptions;
> +import org.mozilla.gecko.mozglue.generatorannotations.WrapElementForJNI;

Imports need to move up

::: mobile/android/base/sqlite/MatrixBlobCursor.java
@@ +18,5 @@
>  package org.mozilla.gecko.sqlite;
>  
>  import android.database.AbstractCursor;
>  import android.database.CursorIndexOutOfBoundsException;
> +import org.mozilla.gecko.mozglue.generatorannotations.WrapElementForJNI;

Import needs to move up
Attachment #806231 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 806230 [details] [diff] [review]
Part 1: Update existing annotations to the new annotation scheme.

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

I feel like it would have been better to split out the new files into a separate patch. This one is mostly a mechanical change except for those two new files. This review is pretty simple so it doesn't matter but for a more complex review it definitely would have helped.
Attachment #806230 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> ::: mobile/android/base/gfx/GeckoLayerClient.java
> @@ +259,1 @@
> >      private void setPageRect(RectF rect, RectF cssRect) {
> 
> This function is not called from JNI

Yes it is:
http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.cpp#1487


Cleaned up the remainder of the issues.
Attachment #806295 - Flags: review?(bugmail.mozilla)
Attachment #806231 - Attachment is obsolete: true
Blocks: 917608
(In reply to Chris Kitching [:ckitching] from comment #8)
> > This function is not called from JNI
> 
> Yes it is:
> http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.
> cpp#1487
> 
> 
> Cleaned up the remainder of the issues.

http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp#303

Note the parameter types. There are two setPageRect functions in GeckoLayerClient.java.
Ah.
Attachment #806295 - Attachment is obsolete: true
Attachment #806295 - Flags: review?(bugmail.mozilla)
Attachment #806730 - Flags: review?(bugmail.mozilla)
Trimming the unneeded function.
Attachment #806234 - Attachment is obsolete: true
Attachment #806234 - Flags: review?(bugmail.mozilla)
Attachment #806731 - Flags: review?(bugmail.mozilla)
Comment on attachment 806730 [details] [diff] [review]
Part 2: V3 - Annotate the new things that we can now generate.

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

Feel free to carry the r+ when you update the import from the comment below.

::: mobile/android/base/sqlite/MatrixBlobCursor.java
@@ +18,5 @@
>  package org.mozilla.gecko.sqlite;
>  
>  import android.database.AbstractCursor;
>  import android.database.CursorIndexOutOfBoundsException;
> +import org.mozilla.gecko.mozglue.generatorannotations.WrapElementForJNI;

This import needs to be moved up still
Attachment #806730 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 806230 [details] [diff] [review]
Part 1: Update existing annotations to the new annotation scheme.

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

::: mobile/android/base/util/Clipboard.java
@@ +33,1 @@
>      public static String getText() {

You should add a part 6 that removes the JNI goop for Clipboard.getText() and Clipboard.setText(..) since you replace those JNI invocations with Clipboard.hasText() and .clearText() in parts 2-5.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> Comment on attachment 806230 [details] [diff] [review]
> Part 1: Update existing annotations to the new annotation scheme.
> 
> Review of attachment 806230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/util/Clipboard.java
> @@ +33,1 @@
> >      public static String getText() {
> 
> You should add a part 6 that removes the JNI goop for Clipboard.getText()
> and Clipboard.setText(..) since you replace those JNI invocations with
> Clipboard.hasText() and .clearText() in parts 2-5.

But nsClipboard::GetData relies on getText, and nsClipboard::SetData uses setText. What I changed was to make hasText not call getTet, and clearText not call setText.
But this doesn't mean the methods aren't used. I think.
Comment on attachment 806731 [details] [diff] [review]
Part 4: V2 - The new generated code.

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

::: widget/android/GeneratedJNIWrappers.h
@@ +5,5 @@
> +#include "AndroidJavaWrappers.h"
> +#include "AndroidBridgeUtilities.h"
> +
> +namespace mozilla {
> +    void InitStubs(JNIEnv *jEnv);

I would like to move this stuff into another namespace, maybe mozilla::android. I don't really want autogenerated code living in the same namespace as the relatively-well-curated mozilla top-level things.

@@ +93,5 @@
> +    static void UnlockScreenOrientation();
> +    static void UnregisterSurfaceTextureFrameListener(jobject a0);
> +    static void Vibrate1(int64_t a0);
> +    static void VibrateA(jlongArray a0, int32_t a1);
> +    GeckoAppShell() : AutoGlobalWrappedJavaObject() {};

Not obvious to me what the purpose of the default constructor here is. Maybe it will become clear as I look through the rest of the patches but I can't imagine this would be used by anything.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> Comment on attachment 806731 [details] [diff] [review]
> Part 4: V2 - The new generated code.
> 
> Review of attachment 806731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/GeneratedJNIWrappers.h
> @@ +5,5 @@
> > +#include "AndroidJavaWrappers.h"
> > +#include "AndroidBridgeUtilities.h"
> > +
> > +namespace mozilla {
> > +    void InitStubs(JNIEnv *jEnv);
> 
> I would like to move this stuff into another namespace, maybe
> mozilla::android. I don't really want autogenerated code living in the same
> namespace as the relatively-well-curated mozilla top-level things.

I put this here as it is the generted portion of mozilla::InitAndroidJavaWrappers. It didn't seem nice to have these two in different namespaces - should they both be moved?

> @@ +93,5 @@
> > +    static void UnlockScreenOrientation();
> > +    static void UnregisterSurfaceTextureFrameListener(jobject a0);
> > +    static void Vibrate1(int64_t a0);
> > +    static void VibrateA(jlongArray a0, int32_t a1);
> > +    GeckoAppShell() : AutoGlobalWrappedJavaObject() {};
> 
> Not obvious to me what the purpose of the default constructor here is. Maybe
> it will become clear as I look through the rest of the patches but I can't
> imagine this would be used by anything.

This one is indeed not used, but some generated classes use the default constructor and there's no way to know at generation-time which ones do or do not have a use for such a constructor. I could add an extra annotation for this, or we could just leave it - the compiler'll delete t anyway. Thoughts?
(In reply to Chris Kitching [:ckitching] from comment #16)
> > I would like to move this stuff into another namespace, maybe
> > mozilla::android. I don't really want autogenerated code living in the same
> > namespace as the relatively-well-curated mozilla top-level things.
> 
> I put this here as it is the generted portion of
> mozilla::InitAndroidJavaWrappers. It didn't seem nice to have these two in
> different namespaces - should they both be moved?

Sorry I wasn't clear. The main thing I'm after is for the generated classes (e.g. GeckoAppShell) to be inside a mozilla::android namespace. I don't care so much about the InitStubs function although it also makes sense to move that.

> > @@ +93,5 @@
> > Not obvious to me what the purpose of the default constructor here is. Maybe
> > it will become clear as I look through the rest of the patches but I can't
> > imagine this would be used by anything.
> 
> This one is indeed not used, but some generated classes use the default
> constructor and there's no way to know at generation-time which ones do or
> do not have a use for such a constructor. I could add an extra annotation
> for this, or we could just leave it - the compiler'll delete t anyway.
> Thoughts?

Ah I see. I just worry that the existence of the constructor will make people start using it when they really shouldn't be. I might change my mind after going over the patches in some more detail but you can leave it for now.
Comment on attachment 806233 [details] [diff] [review]
Part 3: A more general annotation processor.

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

Pretty good overall. I didn't look too closely at the changes to CodeGenerator; I'll evaluate that mostly based on an examination of the generated code. r=me with the comments below addressed.

::: build/annotationProcessors/AnnotationProcessor.java
@@ +41,5 @@
> +         * Generated methods go into GeneratedJavaWrappers.cpp/h.
> +         * Each Java class has a C++ class generated
> +         *
> +         * Java class X has C++ class named AndroidX.
> +         * Each such class has a method AndroidX::InitXClass(JNIEnv) which gets method/field/class refs.

Is this comment true? From part 4 I don't think the C++ class is called AndroidX, it's just mozilla::X (hopefully soon to be mozilla::android::X)

@@ +81,1 @@
>              }

Easier to read if you do:
if (!methodIterator.hasNext()) {
  continue;
}
generatorInstance = ...

::: build/annotationProcessors/CodeGenerator.java
@@ +27,5 @@
>      private final StringBuilder wrapperMethodBodies = new StringBuilder();
> +    private final StringBuilder headerPublic = new StringBuilder();
> +    private final StringBuilder headerProtected = new StringBuilder();
> +
> +    public static final StringBuilder stubInitialiser = new StringBuilder("void InitStubs(JNIEnv *jEnv) {\n");

I'd rather see this initial string argument moved to a regular append(...) call in the CodeGenerator constructor just before the other stuff is appended to stubInitialiser.

@@ +70,4 @@
>  
> +        stubInitialiser.append("    ");
> +        stubInitialiser.append(mCClassName);
> +        stubInitialiser.append("::InitStubs(jEnv);\n");

Personally I would have formatted this like so:
stubInitialiser.append("    ")
               .append(mCClassName)
               .append("::InitStubs(jEnv);\n");

But I don't really care - I'll leave that to you.

@@ +197,5 @@
> +        boolean isFieldStatic = Utils.isMemberStatic(theField);
> +        boolean isFieldFinal = Utils.isMemberFinal(theField);
> +        boolean shallGenerateStatic = isFieldStatic || aFieldTuple.mAnnotationInfo.isStatic;
> +
> +        String getterName = "get"+CFieldName;

nit: spaces around +

@@ +234,5 @@
> +
> +        String implementationSignature = Utils.getCImplementationMethodSignature(theCtor.getParameterTypes(), Void.class, CMethodName, mCClassName);
> +        String headerSignature = Utils.getCHeaderMethodSignature(theCtor.getParameterTypes(), theCtor.getParameterAnnotations(), Void.class, CMethodName, mCClassName, false);
> +
> +        // Slice off the "void" from the start of the constructor declaration.

s/"void"/"void "/ since you're slicing off 5 characters, not 4

@@ +295,5 @@
> +
> +        wrapperMethodBodies.append(" {\n");
> +
> +        // Static stubs check the bridge instance has been created before trying to run.
> +        // TODO: Consider.

Not sure what this TODO means.

@@ +563,5 @@
>          wrapperStartupCode.append("\");\n");
>      }
>  
> +    private void writeZeroingFor(Member aMember, final String aMemberName) {
> +        if(mTakenMemberNames.contains(aMemberName)) {

Will this condition ever fire? The only call site for this function basically has this exact same check just before calling this. Also nit: space before (.

::: build/annotationProcessors/classloader/AnnotatableEntity.java
@@ +20,5 @@
> +    public enum ENTITY_TYPE {METHOD, FIELD, CONSTRUCTOR}
> +
> +    public Method mMethod;
> +    public Field mField;
> +    public Constructor mConstructor;

I don't really like the fact that these are public non-final variables. I also don't really like the "union" style you've used here by having separate fields in the first place. I think a cleaner approach would be to collapse these into a "private [final] AccessibleObject mEntity". You can also collapse the first three constructors into the fourth one. Then, add three methods ("getMethod()", "getField()", and "getConstructor()") that cast mEntity to the appropriate type and return it.

You'd have to update the call sites to use .getMethod() instead of .mMethod, etc. You could even do this in the switch statement in AnnotationProcessor, if you modify the CodeGenerator.generateXXX methods to take an XXX argument in addition to the AnnotatableEntity. (That way the call to .getMethod() only ever occurs in a case statement where we know the AnnotatableEntity type is METHOD.) Does that make sense?

@@ +24,5 @@
> +    public Constructor mConstructor;
> +
> +    public AnnotationInfo mAnnotationInfo;
> +
> +    public ENTITY_TYPE mEntityType;

I would also like these two members to be private or final (or both).

@@ +61,5 @@
> +
> +    public String toString() {
> +        switch (mEntityType) {
> +            case METHOD:
> +                return "METHOD: "+mMethod.getName()+":"+Utils.getTypeSignatureStringForMember(mMethod);

For all the "+"s in this method, please put a space on either side of the operator.

::: build/annotationProcessors/classloader/ClassWithOptions.java
@@ +5,5 @@
> +package org.mozilla.gecko.annotationProcessors.classloader;
> +
> +public class ClassWithOptions {
> +    public Class<?> mClass;
> +    public String mGeneratedName;

Make these final, remove the m prefix

::: build/annotationProcessors/classloader/JarClassIterator.java
@@ +36,5 @@
> +                return next();
> +            } else {
> +                String generateName = null;
> +                Annotation[] annotations = ret.getAnnotations();
> +                for (Annotation annotation : annotations) {

You can just do "for (Annotation annotation : ret.getAnnotations()) {" rather than having the "annotations" local var

::: build/annotationProcessors/utils/AlphabeticAnnotatableEntityComparator.java
@@ +34,5 @@
> +                return 1;
> +            }
> +        }
> +
> +

Remove one of the two blank lines

::: build/annotationProcessors/utils/GeneratableElementIterator.java
@@ +50,5 @@
> +
> +        // Check for "Wrap ALL the things" flag.
> +        for (Annotation annotation : aClass.getDeclaredAnnotations()) {
> +            Class<? extends Annotation> annotationType = annotation.annotationType();
> +            final String annotationTypeName = annotationType.getName();

Easier to remove the annotationType intermediate variable here and just do
String annotationTypeName = annotation.annotationType().getName();

@@ +128,1 @@
>                          stubName = aMethodName.substring(0, 1).toUpperCase() + aMethodName.substring(1);

s/method/element/ in the comment and "aMethodName"

::: widget/android/AndroidJavaWrappers.h
@@ +33,5 @@
>  class nsIAndroidDisplayport;
>  class nsIAndroidViewport;
>  class nsIWidget;
>  
> +

Spurious new line?
Attachment #806233 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 806731 [details] [diff] [review]
Part 4: V2 - The new generated code.

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

I need to sleep. Some comments below but I'll pick this up again tomorrow.

::: widget/android/GeneratedJNIWrappers.cpp
@@ +1,1 @@
> +#include "GeneratedJNIWrappers.h"

I would like a "this file is generated" comment at the top of the file. You can do that as part of this bug or bug 917608, it doesn't really matter.

@@ +1,5 @@
> +#include "GeneratedJNIWrappers.h"
> +#include "AndroidBridgeUtilities.h"
> +#include "nsXPCOMStrings.h"
> +#include "AndroidBridge.h"
> +namespace mozilla {

Add a blank line after the includes.

@@ +2387,5 @@
> +jmethodID GeckoEvent::jcreateAppBackgroundingEvent = 0;
> +jmethodID GeckoEvent::jcreateAppForegroundingEvent = 0;
> +jmethodID GeckoEvent::jcreateBookmarkLoadEvent = 0;
> +jmethodID GeckoEvent::jcreateBroadcastEvent = 0;
> +jmethodID GeckoEvent::jcreateCallObserverEvent = 0;

Hm. Maybe we shouldn't autogenerate everything in GeckoEvent. There's a lot of functions here that are never invoked from JNI, and it's unnecessary to wrap them in JNI goop (and prevent Proguarding them).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> @@ +2387,5 @@
> > +jmethodID GeckoEvent::jcreateAppBackgroundingEvent = 0;
> > +jmethodID GeckoEvent::jcreateAppForegroundingEvent = 0;
> > +jmethodID GeckoEvent::jcreateBookmarkLoadEvent = 0;
> > +jmethodID GeckoEvent::jcreateBroadcastEvent = 0;
> > +jmethodID GeckoEvent::jcreateCallObserverEvent = 0;
> 
> Hm. Maybe we shouldn't autogenerate everything in GeckoEvent. There's a lot
> of functions here that are never invoked from JNI, and it's unnecessary to
> wrap them in JNI goop (and prevent Proguarding them).

Ah, I should've mentioned - this patch set doesn't move GeckoEvent over to generated code - its use in the C++ code is sufficiently strange and complicated that I didn't think it worth including in this already fairly large group of patches.

Also, annotating each field individually is sort of extremely untidy. It's not clear how best to proceed. Perhaps worth a followup bug to address this one.
Comment on attachment 806237 [details] [diff] [review]
Part 5: Refactor the code the use the new generated code.

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

This definitely makes the code much much cleaner, thanks! (And sorry for the slow review). I expect that moving stuff into the mozilla::android namespace will result a large number of mechanical changes to this patch and there's some other things noted below, but r=me with all these things addressed.

::: dom/ipc/ContentParent.cpp
@@ +1563,5 @@
>      *showPassword = true;
>  #ifdef MOZ_WIDGET_ANDROID
>      NS_ASSERTION(AndroidBridge::Bridge() != nullptr, "AndroidBridge is not available");
>      if (AndroidBridge::Bridge() != nullptr)
> +        *showPassword = GeckoAppShell::GetShowPasswordSetting();

You don't need the AndroidBridge checking and assertions above

::: dom/system/android/AndroidLocationProvider.cpp
@@ +27,5 @@
>  AndroidLocationProvider::Startup()
>  {
>      if (!AndroidBridge::Bridge())
>          return NS_ERROR_NOT_IMPLEMENTED;
> +    GeckoAppShell::EnableLocation(true);

Don't need the AndroidBridge::Bridge() check. Same for the other two calls in this file

::: dom/system/android/nsHapticFeedback.cpp
@@ +15,5 @@
>  nsHapticFeedback::PerformSimpleAction(int32_t aType)
>  {
>      AndroidBridge* bridge = AndroidBridge::Bridge();
>      if (bridge) {
> +        GeckoAppShell::PerformHapticFeedback(aType == LongPress);

Don't need the bridge

::: hal/android/AndroidHal.cpp
@@ +152,1 @@
>      return false;

ditto. I'm going to stop pointing these out but take a look through the rest of the patch and take out the rest as well.

::: widget/android/AndroidBridge.cpp
@@ +648,5 @@
>  AndroidBridge::RegisterCompositor(JNIEnv *env)
>  {
>      ALOG_BRIDGE("AndroidBridge::RegisterCompositor");
> +    if (mGLControllerObj != NULL && !mGLControllerObj->isNull()) {
> +    ALOG_BRIDGE("NULL");

Remove the ALOG_BRIDGE calls in this function (they seem like they're intermediate debugging prints) or clean them up so they are proper messages and indented properly

@@ +1408,5 @@
>  }
>  
> +bool
> +AndroidBridge::CreateFrame(AutoLocalJNIFrame *jniFrame, LayerRendererFrame* aFrame)
> +{

See my comment in nsWindow; I would rather you just inlined this one

@@ +1845,5 @@
>  void
>  AndroidBridge::GetDisplayPort(bool aPageSizeUpdate, bool aIsBrowserContentDisplayed, int32_t tabId, nsIAndroidViewport* metrics, nsIAndroidDisplayport** displayPort)
>  {
> +
> +    ALOG_BRIDGE("Enter: %s", __PRETTY_FUNCTION__);

Again with the blank lines above the log lines (see my other comment later)

@@ +1873,5 @@
> +    metrics->GetCssPageBottom(&cssPageBottom);
> +    metrics->GetZoom(&zoom);
> +
> +    ImmutableViewportMetrics jmetrics = ImmutableViewportMetrics(pageLeft, pageTop, pageRight, pageBottom,
> +                                                                               cssPageLeft, cssPageTop, cssPageRight, cssPageBottom,

indent the carryover lines a bit less

@@ +1932,5 @@
>  
> +    jobject progressiveUpdateDataJObj = client->ProgressiveUpdateCallback(aHasPendingNewThebesContent,
> +                                                                     (float)aDisplayPort.x,
> +                                                                     (float)aDisplayPort.y,
> +                                                                     (float)aDisplayPort.width,

Indent carryover lines less

@@ +1948,5 @@
> +    aViewport.height = progressiveUpdateData->getheight();
> +    aScaleX = aScaleY = progressiveUpdateData->getscale();
> +
> +    return progressiveUpdateData->getabort();
> +    ALOG_BRIDGE("Exit: %s", __PRETTY_FUNCTION__);

Logging after return, not so useful

::: widget/android/AndroidJavaWrappers.h
@@ +101,5 @@
> +        Init(jobj, env);
> +    }
> +
> +    ~AutoGlobalWrappedJavaObject();
> +    void Dispose();

Any particular reason you put the Dispose and destructor in the .cpp file instead of here? Just curious since I have yet to determine a good default heuristic for making this decision.

::: widget/android/moz.build
@@ +13,5 @@
>  MODULE = 'widget'
>  
>  EXPORTS += [
>      'AndroidBridge.h',
> +    'AndroidBridgeUtilities.h',

I would like this to not be exported. IIRC it should only be being included by .cpp files in this folder, so there should be no need to export it. I suspect the problem is that you're including this file from GeneratedJNIWrappers.h when you shouldn't be.

::: widget/android/nsWindow.cpp
@@ +1851,5 @@
>      } else if (ae->Action() == AndroidGeckoEvent::IME_UPDATE_CONTEXT) {
> +        GeckoAppShell::NotifyIMEContext(mInputContext.mIMEState.mEnabled,
> +                                               mInputContext.mHTMLInputType,
> +                                               mInputContext.mHTMLInputInputmode,
> +                                               mInputContext.mActionHint);

Update indentation

@@ +2257,5 @@
>  
> +        GeckoAppShell::NotifyIMEChange(event.mReply.mString,
> +                                              change.mStart,
> +                                              change.mOldEnd,
> +                                              change.mNewEnd);

Update indentation

@@ +2363,5 @@
>  void
>  nsWindow::DrawWindowUnderlay(LayerManager* aManager, nsIntRect aRect)
>  {
> +
> +    ALOG_BRIDGE("Enter: %s", __PRETTY_FUNCTION__);

Not sure why you added the ALOG_BRIDGE calls to this function, but please remove the blank lines above them. Also, should the ALOG_BRIDGE really be ALOG? I'm not sure if ALOG_BRIDGE is only meant to be used from AndroidBridge. I note that in the DrawWindowOverlay function a little below you used ALOG instead of ALOG_BRIDGE event though these functions are roughly symmetrical.

@@ +2373,1 @@
>          return;

Indentation

@@ +2376,5 @@
>      AutoLocalJNIFrame jniFrame(env);
>  
> +    GeckoLayerClient* client = AndroidBridge::Bridge()->GetLayerClient();
> +    if (!AndroidBridge::Bridge()->CreateFrame(&jniFrame, &mLayerRendererFrame)) {
> +        ALOG_BRIDGE("Exceptional exit: %s", __PRETTY_FUNCTION__);

I would rather inline the call to CreateFrame here. It doesn't make sense to be on AndroidBridge because it operates on the specific GeckoLayerClient instance.
Attachment #806237 - Flags: review?(bugmail.mozilla) → review+
(In reply to Chris Kitching [:ckitching] from comment #20)
> Ah, I should've mentioned - this patch set doesn't move GeckoEvent over to
> generated code - its use in the C++ code is sufficiently strange and
> complicated that I didn't think it worth including in this already fairly
> large group of patches.

Do you need to generate wrappers for GeckoEvent at all then? I didn't notice any use of the wrappers but I might have missed something. I'm fine with deferring that to a follow-up bug or something but I'd rather not generate the wrappers here then if we don't need them.

> Also, annotating each field individually is sort of extremely untidy. It's
> not clear how best to proceed. Perhaps worth a followup bug to address this
> one.

Yeah, a followup bug would be best. I would prefer annotating each field to annotating the entire class when many of the things there are not used. But maybe we could do something like add a parameter to the WrapEntireClassForJNI annotation that says "fields only".
Comment on attachment 806731 [details] [diff] [review]
Part 4: V2 - The new generated code.

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

Minusing for the namespace thing, but otherwise this looks fine modulo the comments I already made.

::: widget/android/GeneratedJNIWrappers.cpp
@@ +1,1 @@
> +#include "GeneratedJNIWrappers.h"

I would like a "this file is generated" comment at the top of the file. You can do that as part of this bug or bug 917608, it doesn't really matter.

@@ +1,5 @@
> +#include "GeneratedJNIWrappers.h"
> +#include "AndroidBridgeUtilities.h"
> +#include "nsXPCOMStrings.h"
> +#include "AndroidBridge.h"
> +namespace mozilla {

Add a blank line after the includes.

@@ +2387,5 @@
> +jmethodID GeckoEvent::jcreateAppBackgroundingEvent = 0;
> +jmethodID GeckoEvent::jcreateAppForegroundingEvent = 0;
> +jmethodID GeckoEvent::jcreateBookmarkLoadEvent = 0;
> +jmethodID GeckoEvent::jcreateBroadcastEvent = 0;
> +jmethodID GeckoEvent::jcreateCallObserverEvent = 0;

Hm. Maybe we shouldn't autogenerate everything in GeckoEvent. There's a lot of functions here that are never invoked from JNI, and it's unnecessary to wrap them in JNI goop (and prevent Proguarding them).
Attachment #806731 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> ::: build/annotationProcessors/AnnotationProcessor.java
> @@ +41,5 @@
> > +         * Generated methods go into GeneratedJavaWrappers.cpp/h.
> > +         * Each Java class has a C++ class generated
> > +         *
> > +         * Java class X has C++ class named AndroidX.
> > +         * Each such class has a method AndroidX::InitXClass(JNIEnv) which gets method/field/class refs.
> 
> Is this comment true? From part 4 I don't think the C++ class is called
> AndroidX, it's just mozilla::X (hopefully soon to be mozilla::android::X)

This comment should not exist. It was rambling added during the planning phase. Sorry about that.

> @@ +70,4 @@
> >  
> > +        stubInitialiser.append("    ");
> > +        stubInitialiser.append(mCClassName);
> > +        stubInitialiser.append("::InitStubs(jEnv);\n");
> 
> Personally I would have formatted this like so:
> stubInitialiser.append("    ")
>                .append(mCClassName)
>                .append("::InitStubs(jEnv);\n");
> 
> But I don't really care - I'll leave that to you.

Unclear why I didn't think of this. Plausibly makes somewhat smaller bytecode as well as being much neater.

> @@ +295,5 @@
> > +
> > +        wrapperMethodBodies.append(" {\n");
> > +
> > +        // Static stubs check the bridge instance has been created before trying to run.
> > +        // TODO: Consider.
> 
> Not sure what this TODO means.

Aye. It seems I missed my "Go clean up working comments" pass before submitting.

Although, it does mention an important point - what's the deal with the singleton Bridge object? Why isn't everything on AndroidBridge just static? Why do we muck about getting this instance? Is it just style, or is there an actual reason?

> ::: build/annotationProcessors/classloader/JarClassIterator.java
> @@ +36,5 @@
> > +                return next();
> > +            } else {
> > +                String generateName = null;
> > +                Annotation[] annotations = ret.getAnnotations();
> > +                for (Annotation annotation : annotations) {
> 
> You can just do "for (Annotation annotation : ret.getAnnotations()) {"
> rather than having the "annotations" local var

Ah the joy of different reviewers having different ideas of what constitutes "Too complex an expression" :P

> 
> @@ +128,1 @@
> >                          stubName = aMethodName.substring(0, 1).toUpperCase() + aMethodName.substring(1);
> 
> s/method/element/ in the comment and "aMethodName"

... And stop spuriously using the a prefix...
Attachment #806233 - Attachment is obsolete: true
Attachment #808116 - Flags: review?(bugmail.mozilla)
Blocks: 919138
Attachment #806730 - Attachment is obsolete: true
Attachment #808117 - Flags: review+
Attachment #808116 - Attachment is obsolete: true
Attachment #808116 - Flags: review?(bugmail.mozilla)
Applying review comments, adding desired comment.
Attachment #808119 - Flags: review?(bugmail.mozilla)
The resulting generated code. GeckoEvent has been removed, Bug 919138.
Attachment #806731 - Attachment is obsolete: true
Attachment #808120 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> Comment on attachment 806237 [details] [diff] [review]
> Part 5: Refactor the code the use the new generated code.
> 
> Review of attachment 806237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This definitely makes the code much much cleaner, thanks! (And sorry for the
> slow review). I expect that moving stuff into the mozilla::android namespace
> will result a large number of mechanical changes to this patch and there's
> some other things noted below, but r=me with all these things addressed.

Let's talk namespaces.

Is it preferable to use explicit mozilla::android::GeckoAppShell::SomeMethod() style syntax, or to use `using mozilla::android`, or to apply common sense and use `using` where the number of usages is large enough to justify the namespace pollution?

Should it be exclusively the generated code in mozilla::android? Would it not make more sense to shunt all the AndroidBridge and related gubbins in there?
Funfact - now I've worked out how to get IntelliJ IDEA's deep inspection utilities for C++ working on our codebase such widespread refactoring is trivial to perform.

There exists a ::android namespace of our creation. Should this be shunted into mozilla::android, or should leading ::s be added where applicable to keep them seperate?

Once you answer the above I can spit out an appropriate patch.
The other things you mentioned that I found worthy of discussion -
 
> ::: dom/ipc/ContentParent.cpp
> @@ +1563,5 @@
> >      *showPassword = true;
> >  #ifdef MOZ_WIDGET_ANDROID
> >      NS_ASSERTION(AndroidBridge::Bridge() != nullptr, "AndroidBridge is not available");
> >      if (AndroidBridge::Bridge() != nullptr)
> > +        *showPassword = GeckoAppShell::GetShowPasswordSetting();
> 
> You don't need the AndroidBridge checking and assertions above

Omitting the check is not quite functionally equivalent.

The generated code will return false if !AndroidBridge::Bridge().

But the existing implementation has showPassword assigned to true in this case. Your suggestion would flip the result in this case. Similar issues exist in the other places where I did not remove the check.

This sort of motivates my earlier comment about what is Bridge() all about, anyway? Can we kill it off? What does it mean when !Bridge()? Doesn't that mean our program is unrecoverably broken anyway? Refactor it all to static, perhaps and delete ALL the checks?

> ::: widget/android/AndroidJavaWrappers.h
> @@ +101,5 @@
> > +        Init(jobj, env);
> > +    }
> > +
> > +    ~AutoGlobalWrappedJavaObject();
> > +    void Dispose();
> 
> Any particular reason you put the Dispose and destructor in the .cpp file
> instead of here? Just curious since I have yet to determine a good default
> heuristic for making this decision.

The heuristic used was "It won't compile if I do it the other way.".

> ::: widget/android/moz.build
> @@ +13,5 @@
> >  MODULE = 'widget'
> >  
> >  EXPORTS += [
> >      'AndroidBridge.h',
> > +    'AndroidBridgeUtilities.h',
> 
> I would like this to not be exported. IIRC it should only be being included
> by .cpp files in this folder, so there should be no need to export it. I
> suspect the problem is that you're including this file from
> GeneratedJNIWrappers.h when you shouldn't be.

I agree.
Once again, and as mentioned in the previous bug in this series, the heuristic used was "It won't compile if I do it the other way.".

Unclear how to proceed.
Attachment #808119 - Attachment is obsolete: true
Attachment #808119 - Flags: review?(bugmail.mozilla)
Attachment #808120 - Attachment is obsolete: true
Attachment #808120 - Flags: review?(bugmail.mozilla)
Attachment #808249 - Flags: review?(bugmail.mozilla)
Unbreaking the generated code, tidying up the generated comments.
Look, only 250kb now!
Attachment #808250 - Flags: review?(bugmail.mozilla)
Applying the namespace shift for generated code only.

While doing this, I may have encountered the cause of the segfaults - 
"If the object being deleted has incomplete class type at the point of deletion and the complete class has a non-trivial destructor or a deallocation function, the behavior is undefined."

This was happening, causing a compiler warning I wasn't noticing and, perhaps, causing the wrapped objects not to go away when the wrapper object does, leading to:
https://tbpl.mozilla.org/?tree=Try&rev=14d50ca5c5be

(With that in mind, if you do end up r+ing this before the problems get ironed out please don't land it - putting it through review concurrently with debugging the segfaults in the vague hope you might spot something I missed.)
Latest run at:
https://tbpl.mozilla.org/?tree=Try&rev=a1ab50dcab74

Hopefully that's the end of it.
Attachment #806237 - Attachment is obsolete: true
Attachment #808251 - Flags: review?(bugmail.mozilla)
Fixing the segfaulting problem. Double-free - whoops.

https://tbpl.mozilla.org/?tree=Try&rev=8901d2a3e442

Now green on try - can actually land once you're happy with it.
Attachment #808251 - Attachment is obsolete: true
Attachment #808251 - Flags: review?(bugmail.mozilla)
Attachment #808273 - Flags: review?(bugmail.mozilla)
Comment on attachment 808117 [details] [diff] [review]
Part 2: V4 - Annotate the new things that we can now generate.

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

::: mobile/android/base/sqlite/MatrixBlobCursor.java
@@ +15,5 @@
>   * limitations under the License.
>   */
>  
>  package org.mozilla.gecko.sqlite;
> +import org.mozilla.gecko.mozglue.generatorannotations.WrapElementForJNI;

nit: Add a blank line between package and import
(In reply to Chris Kitching [:ckitching] from comment #28)
> 
> Is it preferable to use explicit
> mozilla::android::GeckoAppShell::SomeMethod() style syntax, or to use `using
> mozilla::android`, or to apply common sense and use `using` where the number
> of usages is large enough to justify the namespace pollution?

I like the "apply common sense" approach.

> Should it be exclusively the generated code in mozilla::android? Would it
> not make more sense to shunt all the AndroidBridge and related gubbins in
> there?
> Funfact - now I've worked out how to get IntelliJ IDEA's deep inspection
> utilities for C++ working on our codebase such widespread refactoring is
> trivial to perform.
> 
> There exists a ::android namespace of our creation. Should this be shunted
> into mozilla::android, or should leading ::s be added where applicable to
> keep them seperate?
> 
> Once you answer the above I can spit out an appropriate patch.
> The other things you mentioned that I found worthy of discussion -

Oh, interesting. I forgot about the ::android namespace. That looks like it contains things that are relevant to both Fennec and B2G, which makes sense because they are both android-based. So it makes sense to keep this stuff separate from the existing ::android namespace. Having mozilla::android and ::android might be confusing though, so maybe we should use something else for the new one we're adding. I'm leaning towards mozilla::widget::android. This would be parallel to how we have ANDROID and MOZ_WIDGET_ANDROID #defines where the former is defined on both B2G and Fennec, and the latter is only defined on Fennec. Add leading ::s where applicable, although hopefully that won't be a lot of places.

Finally, I have no objections to shunting AndroidBridge and related "gubbins" into mozilla::widget::android if that makes the code simpler. It's hard for me to tell right now how many cross-namespace references there are by looking at the patches but a good rule of thumb is that in order to conform as best possible to "high cohesion, low coupling" try to move classes so as to minimize cross-namespace references.

> > ::: dom/ipc/ContentParent.cpp
> > You don't need the AndroidBridge checking and assertions above
> 
> Omitting the check is not quite functionally equivalent.
> 
> The generated code will return false if !AndroidBridge::Bridge().
> 
> But the existing implementation has showPassword assigned to true in this
> case. Your suggestion would flip the result in this case. Similar issues
> exist in the other places where I did not remove the check.
> 

Yeah. I think that if !AndroidBridge::Bridge, then we're kind of hosed anyway so it doesn't really matter what gets returned. I think that was true of many if not all of the similar checks I saw in the patches. That being said it's probably safer to leave the checks in for now (specially since I'd like you to land this before you leave) and file a follow-up to investigate if they can be safely removed.

> This sort of motivates my earlier comment about what is Bridge() all about,
> anyway? Can we kill it off? What does it mean when !Bridge()? Doesn't that
> mean our program is unrecoverably broken anyway? Refactor it all to static,
> perhaps and delete ALL the checks?

Yeah, generally if !Bridge() then the program is pretty much unrecoverable. At least as far as I know. There is one exception, which is if the ::Bridge() call is run on a non-Fennec platform (i.e. it's outside a MOZ_WIDGET_ANDROID ifdef) in which case Bridge() can (and should) return null and the program should continue.

I think the reason we didn't make things static in AndroidBridge before is largely "historical". At some point somebody made the decision to have the JNI ref to GeckoAppShell be a member variable of AndroidBridge and be set on AndroidBridge construction and from then onwards we pretty much always need to check and get an instance of AndroidBridge before calling the wrappers. We quite probably will be able to kill a whole bunch of code now with the new architecture but I would rather get this landed first and file follow-ups for that.

> > ::: widget/android/AndroidJavaWrappers.h
> > @@ +101,5 @@
> > > +        Init(jobj, env);
> > > +    }
> > > +
> > > +    ~AutoGlobalWrappedJavaObject();
> > > +    void Dispose();
> > 
> > Any particular reason you put the Dispose and destructor in the .cpp file
> > instead of here? Just curious since I have yet to determine a good default
> > heuristic for making this decision.
> 
> The heuristic used was "It won't compile if I do it the other way.".

I suppose that's heuristic is fine for this case.

> 
> > ::: widget/android/moz.build
> > @@ +13,5 @@
> > >  MODULE = 'widget'
> > >  
> > >  EXPORTS += [
> > >      'AndroidBridge.h',
> > > +    'AndroidBridgeUtilities.h',
> > 
> > I would like this to not be exported. IIRC it should only be being included
> > by .cpp files in this folder, so there should be no need to export it. I
> > suspect the problem is that you're including this file from
> > GeneratedJNIWrappers.h when you shouldn't be.
> 
> I agree.
> Once again, and as mentioned in the previous bug in this series, the
> heuristic used was "It won't compile if I do it the other way.".
> 
> Unclear how to proceed.

For this one I would like to dig into why it won't compile. What error messages do you get if you only include AndroidBridgeUtilities.h from .cpp files in widget/android/?
Comment on attachment 808249 [details] [diff] [review]
Part 3: V4 - A more general annotation processor.

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

::: build/annotationProcessors/CodeGenerator.java
@@ +29,5 @@
> +    private final StringBuilder headerProtected = new StringBuilder();
> +
> +    public static final StringBuilder stubInitialiser = new StringBuilder();
> +    static {
> +        stubInitialiser.append("void InitStubs(JNIEnv *jEnv) {\n");

Sorry if I wasn't clear before. I meant to move this down to line 65 so that it is in the constructor, not a static initializer block.

::: build/annotationProcessors/classloader/AnnotatableEntity.java
@@ +17,5 @@
> + */
> +public class AnnotatableEntity {
> +    public enum ENTITY_TYPE {METHOD, FIELD, CONSTRUCTOR}
> +
> +    private final Member mMember;

Nice! Member is much more appropriate than AccessibleObject :)

@@ +20,5 @@
> +
> +    private final Member mMember;
> +    public final ENTITY_TYPE mEntityType;
> +
> +    public AnnotationInfo mAnnotationInfo;

Make this final.

::: build/annotationProcessors/utils/GeneratableElementIterator.java
@@ +59,5 @@
>  
>          findNextValue();
>      }
>  
> +    //TODO: INLINE.

Inline this would be a good first bug for a contributor if you don't want to do it here. :)
Attachment #808249 - Flags: review?(bugmail.mozilla) → review+
Attachment #808250 - Flags: review?(bugmail.mozilla) → review+
I'll look at part 5 once we finalize the namespace issues.
Amazing. Three roundtrips to get "Move an import" done correctly. :P
Attachment #808117 - Attachment is obsolete: true
Attachment #808946 - Flags: review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35)
> ::: build/annotationProcessors/CodeGenerator.java
> @@ +29,5 @@
> > +    private final StringBuilder headerProtected = new StringBuilder();
> > +
> > +    public static final StringBuilder stubInitialiser = new StringBuilder();
> > +    static {
> > +        stubInitialiser.append("void InitStubs(JNIEnv *jEnv) {\n");
> 
> Sorry if I wasn't clear before. I meant to move this down to line 65 so that
> it is in the constructor, not a static initializer block.

But what you suggest isn't functionally equivalent, and will break the generator. This is a static field. Appending the method definition line to it once for every class we generate is going to give us a bad time.

> ::: build/annotationProcessors/utils/GeneratableElementIterator.java
> @@ +59,5 @@
> >  
> >          findNextValue();
> >      }
> >  
> > +    //TODO: INLINE.
> 
> Inline this would be a good first bug for a contributor if you don't want to
> do it here. :)

Another instance of my entirely forgetting to do my "Things to do before sending to the reviewer" pass before sending to the reviewer.

Actually doing it isn't hard. CTRL+ALT+N. Done. Refactoring should never be nontrivial if you're using the right tools :P.

> Yeah, generally if !Bridge() then the program is pretty much unrecoverable.
> At least as far as I know. There is one exception, which is if the
> ::Bridge() call is run on a non-Fennec platform (i.e. it's outside a
> MOZ_WIDGET_ANDROID ifdef) in which case Bridge() can (and should) return
> null and the program should continue.

I'll delete the checks and see how much orange ensues.

> I think the reason we didn't make things static in AndroidBridge before is
> largely "historical". At some point somebody made the decision to have the
> JNI ref to GeckoAppShell be a member variable of AndroidBridge and be set on
> AndroidBridge construction and from then onwards we pretty much always need
> to check and get an instance of AndroidBridge before calling the wrappers.
> We quite probably will be able to kill a whole bunch of code now with the
> new architecture but I would rather get this landed first and file
> follow-ups for that.

So I had a go at eliminating the singleton and just using static things - makes the C++ a lot neater, but it turns out we can't (I think) do this, because Javascript uses the nsWeirdnessStuff to get and use the bridge object.

> > I agree.
> > Once again, and as mentioned in the previous bug in this series, the
> > heuristic used was "It won't compile if I do it the other way.".
> > 
> > Unclear how to proceed.
> 
> For this one I would like to dig into why it won't compile. What error
> messages do you get if you only include AndroidBridgeUtilities.h from .cpp
> files in widget/android/?

None at all, it seems. Problem solved.
Attachment #808249 - Attachment is obsolete: true
Attachment #809542 - Flags: review?(bugmail.mozilla)
Attachment #808250 - Attachment is obsolete: true
Attachment #809543 - Flags: review?(bugmail.mozilla)
Attachment #809544 - Flags: review?(bugmail.mozilla)
Attachment #808273 - Attachment is obsolete: true
Attachment #808273 - Flags: review?(bugmail.mozilla)
Removing checks for if (Bridge()) from, essentialy, everywhere.

https://tbpl.mozilla.org/?tree=Try&rev=825e98ec59fd

If this turns green, I'll merge it into the appropriate patches (Or it could be landed as-is, provided you want no more changes to parts 3 or 4.)

It does tidy things up a lot.
Attachment #809546 - Flags: feedback?(bugmail.mozilla)
Stop generating redundant checks - Try run looks fruity.
Attachment #809542 - Attachment is obsolete: true
Attachment #809542 - Flags: review?(bugmail.mozilla)
Attachment #809583 - Flags: review?(bugmail.mozilla)
Generated code sans pointless checks.
Attachment #809543 - Attachment is obsolete: true
Attachment #809546 - Attachment is obsolete: true
Attachment #809543 - Flags: review?(bugmail.mozilla)
Attachment #809546 - Flags: feedback?(bugmail.mozilla)
Attachment #809584 - Flags: review?(bugmail.mozilla)
Attachment #809544 - Attachment is obsolete: true
Attachment #809544 - Flags: review?(bugmail.mozilla)
Attachment #809588 - Flags: review?(bugmail.mozilla)
And finally, the patch to remove the checks.
Attachment #809589 - Flags: review?(bugmail.mozilla)
Rebasing.
Attachment #806230 - Attachment is obsolete: true
Attachment #809590 - Flags: review+
(In reply to Chris Kitching [:ckitching] from comment #38)
> But what you suggest isn't functionally equivalent, and will break the
> generator. This is a static field. Appending the method definition line to
> it once for every class we generate is going to give us a bad time.

Huh. Apparently I can't read. Sorry about that. I still don't like that stubInitialiser is public static in CodeGenerator while everything else is an instance variable. It seems to me that it would be better to move stubInitialiser out to just be a StringBuilder in main(), and tack on each CodeGenerator instance's bit in the codegen loop in main(). Something like this:

StringBuilder stubInitialiser = new StringBuilder("void InitStubs(JNIEnv *jEnv) {\n");
while (jarClassIterator.hasNext()) {
    ...
    CodeGenerator generatorInstance;
    ...
    stubInitialiser.append(generatorInstance.getInitialiser());
    ...
}
// write stubInitialiser to a file.

If you want to save on handful of extra strings that get created this way you can pass the StringBuilder in to the generatorInstance.getInitialiser function and do the appends inside the function.

> > ::: build/annotationProcessors/utils/GeneratableElementIterator.java
> Actually doing it isn't hard. CTRL+ALT+N. Done. Refactoring should never be
> nontrivial if you're using the right tools :P.

Overgeneralizations should always be avoided. :)

> > I think the reason we didn't make things static in AndroidBridge before is
> > largely "historical". At some point somebody made the decision to have the
> > JNI ref to GeckoAppShell be a member variable of AndroidBridge and be set on
> > AndroidBridge construction and from then onwards we pretty much always need
> > to check and get an instance of AndroidBridge before calling the wrappers.
> > We quite probably will be able to kill a whole bunch of code now with the
> > new architecture but I would rather get this landed first and file
> > follow-ups for that.
> 
> So I had a go at eliminating the singleton and just using static things -
> makes the C++ a lot neater, but it turns out we can't (I think) do this,
> because Javascript uses the nsWeirdnessStuff to get and use the bridge
> object.
> 

Ah yes, the nsIAndroidBridge hook will make it impossible to get rid of the (lone) instance. That's fine then.

(In reply to Chris Kitching [:ckitching] from comment #42)
> Created attachment 809546 [details] [diff] [review]
> Part 6: Remove ALL the bridge checks.
> 
> Removing checks for if (Bridge()) from, essentialy, everywhere.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=825e98ec59fd

I would also do a try push for all platforms. The risk here is that some non-Android platform was using one of those AndroidBridge::Bridge() checks and will now break.

(In reply to Chris Kitching [:ckitching] from comment #43)
> 
> Stop generating redundant checks - Try run looks fruity.

I would have thought "fruity" would mean "orange" but apparently you mean green?
Comment on attachment 809583 [details] [diff] [review]
Part 3: V6 - A more general annotation processor.

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

Feel free to leave or fix the stubInitialiser thing as I noted above. I'm not going to block landing based on that.
Attachment #809583 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #48)
> (In reply to Chris Kitching [:ckitching] from comment #38)
> > But what you suggest isn't functionally equivalent, and will break the
> > generator. This is a static field. Appending the method definition line to
> > it once for every class we generate is going to give us a bad time.
> 
> Huh. Apparently I can't read. Sorry about that. I still don't like that
> stubInitialiser is public static in CodeGenerator while everything else is
> an instance variable. It seems to me that it would be better to move
> stubInitialiser out to just be a StringBuilder in main(), and tack on each
> CodeGenerator instance's bit in the codegen loop in main(). Something like
> this:
> 
> StringBuilder stubInitialiser = new StringBuilder("void InitStubs(JNIEnv
> *jEnv) {\n");
> while (jarClassIterator.hasNext()) {
>     ...
>     CodeGenerator generatorInstance;
>     ...
>     stubInitialiser.append(generatorInstance.getInitialiser());
>     ...
> }
> // write stubInitialiser to a file.
> 
> If you want to save on handful of extra strings that get created this way
> you can pass the StringBuilder in to the generatorInstance.getInitialiser
> function and do the appends inside the function.

Ah yes, what I had before was rather poor OOP.

> (In reply to Chris Kitching [:ckitching] from comment #42)
> > Created attachment 809546 [details] [diff] [review]
> > Part 6: Remove ALL the bridge checks.
> > 
> > Removing checks for if (Bridge()) from, essentialy, everywhere.
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=825e98ec59fd
> 
> I would also do a try push for all platforms. The risk here is that some
> non-Android platform was using one of those AndroidBridge::Bridge() checks
> and will now break.

Very well.
https://tbpl.mozilla.org/?tree=Try&rev=f1702fdcd977

> (In reply to Chris Kitching [:ckitching] from comment #43)
> > 
> > Stop generating redundant checks - Try run looks fruity.
> 
> I would have thought "fruity" would mean "orange" but apparently you mean
> green?

Fruity: Formalised version of "froody":
http://hitchhikers.wikia.com/wiki/Froody

Clearly. Read more Adams. :P
Attachment #809583 - Attachment is obsolete: true
Attachment #809652 - Flags: review+
Comment on attachment 809588 [details] [diff] [review]
Part 5: V5 - Refactor the code the use the new generated code.

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

::: mobile/android/base/GeckoAppShell.java
@@ +295,5 @@
>          return sLayerView;
>      }
>  
>      public static void runGecko(String apkPath, String args, String url, String type) {
> +        Log.e("Fennec", "ARGS FOR GECKO: "+args);

remove debug(?) logging

::: widget/android/AndroidBridge.cpp
@@ +1487,5 @@
> +    if (!viewTransformJObj) {
> +        return;
> +    }
> +
> +    NS_ABORT_IF_FALSE(viewTransformJObj, "No view transform object!");

This abort statement is useless because the above if condition will cause an early return. I would move the abort to before the if condition, so that debug builds will abort and release builds will silent-return.

@@ +1531,5 @@
> +    if (!viewTransformJObj) {
> +        return;
> +    }
> +
> +    NS_ABORT_IF_FALSE(viewTransformJObj, "No view transform object!");

Ditto

@@ +1902,5 @@
>  
>  void
>  AndroidBridge::ContentDocumentChanged()
>  {
> +    mLayerClient->ContentDocumentChanged();

Do you not still need the !mLayerClient check here?
Attachment #809588 - Flags: review?(bugmail.mozilla) → review+
Attachment #809584 - Flags: review?(bugmail.mozilla) → review+
https://tbpl.mozilla.org/?tree=Try&rev=f1702fdcd977

That was not a triumph.

Need to make smarter use of ifdefs.
Comment on attachment 809589 [details] [diff] [review]
Part 6 - Remove redundant AndroidBridge::Bridge() checks.

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

I may have mis-explained what I meant in comment 21. I would like the !Bridge() checks left in for code that actually does use AndroidBridge::Bridge(). The code I was reviewing in comment 21 had a bunch of stuff like so:

if (!AndroidBridge::Bridge()) return or whatever;
GeckoAppShell::doSomething();

In this case the Bridge() check is redundant because GeckoAppShell::doSomething() already has built-in error checking for anything that might fail. However in pieces of code like so:

if (!AndroidBridge::Bridge()) return or whatever;
AndroidBridge::Bridge()->DoSomething();

the check is not redundant, because if Bridge() is null this will segfault hard. So for these cases I would like to leave the check in. Does that make sense?
Attachment #809589 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #53)
> Comment on attachment 809589 [details] [diff] [review]
> Part 6 - Remove redundant AndroidBridge::Bridge() checks.
> 
> Review of attachment 809589 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I may have mis-explained what I meant in comment 21. I would like the
> !Bridge() checks left in for code that actually does use
> AndroidBridge::Bridge(). The code I was reviewing in comment 21 had a bunch
> of stuff like so:
> 
> if (!AndroidBridge::Bridge()) return or whatever;
> GeckoAppShell::doSomething();
> 
> In this case the Bridge() check is redundant because
> GeckoAppShell::doSomething() already has built-in error checking for
> anything that might fail. However in pieces of code like so:
> 
> if (!AndroidBridge::Bridge()) return or whatever;
> AndroidBridge::Bridge()->DoSomething();
> 
> the check is not redundant, because if Bridge() is null this will segfault
> hard. So for these cases I would like to leave the check in. Does that make
> sense?

Aye, but such code, I think, doesn't ever run until after the bridge has been constructed.
Anyway, new patch time.

Also, try run of old patch:
https://tbpl.mozilla.org/?tree=Try&rev=0464ec5fee32

Seems to have broken XPCShell tests on Android. Drat.

Try run of this patch:
https://tbpl.mozilla.org/?tree=Try&rev=26e23dc9901f
Attachment #809589 - Attachment is obsolete: true
Attachment #810241 - Flags: review?(bugmail.mozilla)
Dealing with Comment 51.
Attachment #809588 - Attachment is obsolete: true
Attachment #810265 - Flags: review+
Comment on attachment 810241 [details] [diff] [review]
Part 6 - V2 - Remove redundant AndroidBridge::Bridge() checks.

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

::: netwerk/system/android/nsAndroidNetworkLinkService.cpp
@@ -29,5 @@
> -  if (!mozilla::AndroidBridge::Bridge()) {
> -    // Fail soft here and assume a connection exists
> -    NS_WARNING("GetIsLinkUp is not supported without a bridge connection");
> -    *aIsUp = true;
> -    return NS_OK;

Looking at the history of this hunk, it might be needed to make the xpcshell tests pass. Which might also explain your xpcshell failures.
Attachment #810241 - Flags: review?(bugmail.mozilla) → review+
To save you some time I put back the hunk in nsAndroidNetworkLinkService.cpp and pushed it to try on top of your most recent try push from comment 54.

https://tbpl.mozilla.org/?tree=Try&rev=28df50324f21
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #57)
> To save you some time I put back the hunk in nsAndroidNetworkLinkService.cpp
> and pushed it to try on top of your most recent try push from comment 54.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=28df50324f21

Grand. That appears to have solved the problem.

Thanks for being so helpful - don't fret too much about landing before my departure - more important that it's right than it's quick.
Since you now have the latest version of the patch, I'll leave it to you to upload and review it. Seems like we're done here.
I think some of the patches still need to be rebased against latest code; I ran into a bit of trouble there which is why I ended up pulling your patches from try and just tacking on top of that. But you have my r+ for gluing https://hg.mozilla.org/try/rev/28df50324f21 onto part 6.
Gluing.
Attachment #810241 - Attachment is obsolete: true
Attachment #810816 - Flags: review+
Ah yes. Other developers have been actually doing work. How dare they. :P

Unbitrot time!
Attachment #809590 - Attachment is obsolete: true
Attachment #810817 - Flags: review+
Someone did large changes to the getContext code to no longer need to use the JNI on our code, but to instead use Android libraries to get the same effect.
Switched it back to using GeckoAppShell::GetContext() for neatness - the underlying Java function is used in a large number of places in Java, anyway, so we don't lose anything by doing this. 
(You may want to peruse this new change as it deviates slightly from "Trivially unbitrot everything".)

Otherwise, this should conclude the needed unbitrotting and make us landable. Try run at:
https://tbpl.mozilla.org/?tree=Try&rev=7b2e78a100a6

Just in case I managed to screw this step up.
Attachment #810265 - Attachment is obsolete: true
Attachment #810855 - Flags: review?(bugmail.mozilla)
Rebasing.

That ought to do it. Let's see what happens on try and then land.
Attachment #810816 - Attachment is obsolete: true
Attachment #810858 - Flags: review+
Comment on attachment 810855 [details] [diff] [review]
Part 5: V7 - Refactor the code the use the new generated code.

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

::: widget/android/AndroidBridge.cpp
@@ +1425,2 @@
>  
> +        jobject appContext = GeckoAppShell::GetContext();

I don't know if this is quite the same. The modifications gcp made to this code explicitly get the application context (which is long-lived) rather than just any old context (such as the GeckoApp instance, which is not long-lived). What you're doing is effectively reverting his change back to return a short-lived context rather than a long-lived one.

In Java code it doesn't matter so much because the context object returned from GeckoAppShell.getContext() is not cached anywhere. The code that calls AndroidBridge::GetGlobalContextRef() however does cache the object, and so your change could cause it to leak the GeckoApp. I think if you take the new code and just change GetContext() to GeckoAppShell::GetContext and leave all the other stuff (the call to getApplicationContext()) it would be better. Or you can get gcp to r+ this - he'll probably be coming online around the time you'll be going to bed :)
Attachment #810855 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #64)
> Comment on attachment 810855 [details] [diff] [review]
> Part 5: V7 - Refactor the code the use the new generated code.
> 
> Review of attachment 810855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/AndroidBridge.cpp
> @@ +1425,2 @@
> >  
> > +        jobject appContext = GeckoAppShell::GetContext();
> 
> I don't know if this is quite the same. The modifications gcp made to this
> code explicitly get the application context (which is long-lived) rather
> than just any old context (such as the GeckoApp instance, which is not
> long-lived). What you're doing is effectively reverting his change back to
> return a short-lived context rather than a long-lived one.
> 
> In Java code it doesn't matter so much because the context object returned
> from GeckoAppShell.getContext() is not cached anywhere. The code that calls
> AndroidBridge::GetGlobalContextRef() however does cache the object, and so
> your change could cause it to leak the GeckoApp. I think if you take the new
> code and just change GetContext() to GeckoAppShell::GetContext and leave all
> the other stuff (the call to getApplicationContext()) it would be better. Or
> you can get gcp to r+ this - he'll probably be coming online around the time
> you'll be going to bed :)

Surely the GeckoApp should exist for the entire duration of the app? Let's ask him what he thinks - there must've been a reason for the change - at least I'm curious to see what it was before I revert my change :P.
Flags: needinfo?(gpascutto)
(In reply to Chris Kitching [:ckitching] from comment #65)
> Surely the GeckoApp should exist for the entire duration of the app? Let's
> ask him what he thinks - there must've been a reason for the change - at
> least I'm curious to see what it was before I revert my change :P.

GeckoApp is poorly named, as it is actually an Activity rather than an Application. Activities can come and go, particularly if you have the "don't keep activities" debugging option set in the android development options. We've had many many bugs in the past because people assumed a particular GeckoApp instance lives for the entire duration of the app. But yes, I would like to see what gcp says.
kats pretty much explained it perfectly. The global reference created there needs to be guaranteed to say alive for as long as Gecko is. This used to be pretty much true for GeckoApp (minus Don't Keep Activities which probably broke it already), it's less clear for GeckoView and in any case we shouldn't depend on it. The Application context on the other hand is guaranteed to persist no matter what.
Flags: needinfo?(gpascutto)
Jolly good - thanks for the info.

Charge!
Attachment #810855 - Attachment is obsolete: true
Attachment #811090 - Flags: review?(bugmail.mozilla)
Attachment #811090 - Flags: review?(bugmail.mozilla) → review+
Hmm.

So we're dying with a null pointer dereference at the point where we try to do the call into java from the wrapped object.

The candidates for what might be null at the call are the wrapped object and the underlying method id. If memory serves, asking JNI to call a null method ID causes an 0xdeadd00d abort, not a null pointer dereference.
So - the wrapped object is null at the point of the call.

The calls to the crashing functions can be summarised as:

3 at DrawBackground in DrawWindowUnderlay
1 at DrawForeground in DrawWindowOverlay
1 at DrawBackground in DrawWindowUnderlay
1 at EndDrawing     in DrawWindowOverlay

DrawBackground in DrawWindowUnderlay is immediately preceded by a call to BeginDrawing on the same wrapped object - this call didn't segfault, so why should this one?
Could another thread be jumping in, calling DrawWindowOverlay, and reaching the call to Dispose before DrawWindowUnderlay is done? Wouldn't that cause incorrect drawing? The old code was no more thread safe than the new code (I think?) - so why was it unbroken if this is the case?
Maybe we want mutual exclusion around these two sequences of calls, to prevent a race condition? (Doubtful - it's not clear that this could be the problem and not be a problem that existed before I came along and broke everything.)

Looking for functional differences between old and new code, then. A null check of the result of CreateFrame had been removed - that would certainly cause a party - but why would it crash on the second call, not the first?
Also - the use of an NS_ABORT_IF_FALSE macro here is supect. This macro kills the program if the input expression is false, right? Why, then, would you have one of these followed by a "real" check and return on the same condition? If the condition is such a "What a Terrible Failure" moment that you'd want to crash a debug build so you notice the problem, surely it's a condition that should never occur at runtime?
If this is the problem - smells like Talos is doing something odd.

The other functional difference seems to be that the old code would abort the chain of calls if one of them threw an exception, whereas mine continues (The wrappers will discard exceptions and abort as appropriate). I'm skeptical that this difference can cause a crash of this sort.

Here's a revised patch that replaces the missing null check. I'm skeptical this is the cause of the problem. I'm also unable to run Talos tests locally and my LDAP 
Any thoughts?
Attachment #811090 - Attachment is obsolete: true
Attachment #811459 - Flags: review?(bugmail.mozilla)
That is to say, my LDAP expired, so I can't run things on try. Sorry - trailed off in mid sentence there.
Unbitrotting.
Attachment #810817 - Attachment is obsolete: true
Attachment #811490 - Flags: review+
Unbitrotting.
Attachment #811459 - Attachment is obsolete: true
Attachment #811459 - Flags: review?(bugmail.mozilla)
Attachment #811493 - Flags: review?(bugmail.mozilla)
Many thanks for the try push, gcp.

It would appear that the replacement of that null check has indeed solved the problem (At least, gcp's try push looks okay..)
Shall we try landing again?
09-30 02:32:14.823 E/GeckoConsole( 3176): [JavaScript Warning: "TypeError: asm.js type error: Disabled by lack of floating point support" {file: "http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_asmjs.html" line: 0}]
[...]
09-30 02:32:14.833 I/GeckoDump( 3176): 16250 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_asmjs.html | f1 is an asm.js module

Digging a bit through the code, and comparing with past logs, the asm.js tests seems to be failing because it thinks the testing device has no VFP. The code that checks for VFP is internal to the JS engine and seems to be straight C++ code until it reads /proc off the filesystem. It's not clear to me why this is now failing because I don't see any JNI involved here.
(In reply to Chris Kitching [:ckitching] from comment #73)
> Could another thread be jumping in, calling DrawWindowOverlay, and reaching
> the call to Dispose before DrawWindowUnderlay is done?

DrawWindowOverlay/Underlay should only ever be getting called from the compositor thread.

> Looking for functional differences between old and new code, then. A null
> check of the result of CreateFrame had been removed - that would certainly
> cause a party - but why would it crash on the second call, not the first?
> Also - the use of an NS_ABORT_IF_FALSE macro here is supect. This macro
> kills the program if the input expression is false, right? Why, then, would
> you have one of these followed by a "real" check and return on the same
> condition? If the condition is such a "What a Terrible Failure" moment that
> you'd want to crash a debug build so you notice the problem, surely it's a
> condition that should never occur at runtime?

The NS_ABORT_IF_FALSE is there to crash debug builds so that we notice the problem, as you said. The "real" check is there so that we don't crash release builds, and so that we allow for the possibility of a graceful handling. Often errors like this will clear themselves after some time, so if we don't crash it's quite possible that there will just be a momentary glitch where that one frame didn't render properly but after that we're back to normal. Crashing in such a scenario is not really preferable.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #81)
> 09-30 02:32:14.823 E/GeckoConsole( 3176): [JavaScript Warning: "TypeError:
> asm.js type error: Disabled by lack of floating point support" {file:
> "http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_asmjs.html"
> line: 0}]
> [...]
> 09-30 02:32:14.833 I/GeckoDump( 3176): 16250 ERROR TEST-UNEXPECTED-FAIL |
> /tests/js/xpconnect/tests/mochitest/test_asmjs.html | f1 is an asm.js module
> 
> Digging a bit through the code, and comparing with past logs, the asm.js
> tests seems to be failing because it thinks the testing device has no VFP.
> The code that checks for VFP is internal to the JS engine and seems to be
> straight C++ code until it reads /proc off the filesystem. It's not clear to
> me why this is now failing because I don't see any JNI involved here.

I also don't see how these patches could cause this failure. Marty, do you know if there is known flakiness with VFP detection on Android 2.2 opt builds?
Flags: needinfo?(mrosenberg)
Attachment #811493 - Flags: review?(bugmail.mozilla) → review+
Could it also be a mismatch between the code in [1] which runs during the isAsmJSCompilationAvailable() test at the top of js/xpconnect/tests/mochitest/test_asmjs.test and the code at [2] which appears to be triggering the warning? The latter has an additional WTF_ARM_ARCH_VERSION == 6 check that the former does not.

[1] http://mxr.mozilla.org/mozilla-central/source/js/src/jit/AsmJS.cpp#6503
[2] http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Runtime.cpp#303
so, it looks like before we check WTF_ARM_ARCH_VERSION == 6, there is the call:
if (!JSC::MacroAssembler::supportsFloatingPoint())
    return false;
which on ARM, is:
    static bool supportsFloatingPoint()
    {
        return s_isVFPPresent;
    }

so perhaps something is going wrong with this static initializer. I don't seem to have access to an android device atm, I'll poke at one in gdb tomorrow when I'm back at home.
Flags: needinfo?(mrosenberg)
I'm not convinced it's possible that this failure is caused by this patch, but I'm also not familiar enough with the code that's failing to be able to prove this.
I'm possibly being dumb, but I thought patchsets got bisected to find the offending patch before things were backed out?
>I'm not convinced it's possible that this failure is caused by this patch, but I'm also not familiar enough with the code that's failing to be able to prove this.

I agree. I'm trying to investigate but running into bugs in the ARMv6 support of GCC (it fails to compile Firefox with optimization *disabled*) and other things like bug 922701 that are interfering.

>I'm possibly being dumb, but I thought patchsets got bisected to find the offending patch before things were backed out?

That's not really possible because there's a delay of several hours for all tests to finish running, potentially more if the issue is intermittent. Rather than leave the tree burning for hours (or days...) and block other people from doing their work, the sherrifs will back out anything suspicious with extreme prejudice.

If you wonder why *your* patch was backed out, it's easy: this failure was never seen before this patch landed, affected all mochitests while it was in, and immediately disappeared after it was backed out.

Practical evidence (this patch makes the tree burn) goes before theory (we can't explain from the code how that could happen).
And so the dance begins...
Attachment #811493 - Attachment is obsolete: true
Attachment #812887 - Flags: review+
Attachment #810858 - Attachment is obsolete: true
Attachment #811490 - Attachment is obsolete: true
Attachment #813635 - Flags: review+
The dance continues.
Attachment #812887 - Attachment is obsolete: true
Attachment #813637 - Flags: review+
(In reply to Marty Rosenberg [:mjrosenb] from comment #85)
> so, it looks like before we check WTF_ARM_ARCH_VERSION == 6, there is the
> call:
> if (!JSC::MacroAssembler::supportsFloatingPoint())
>     return false;
> which on ARM, is:
>     static bool supportsFloatingPoint()
>     {
>         return s_isVFPPresent;
>     }
> 
> so perhaps something is going wrong with this static initializer. I don't
> seem to have access to an android device atm, I'll poke at one in gdb
> tomorrow when I'm back at home.

Hi Marty, were you able to get ahold of a device?
Chris, can you break this up into separately landable chunks? For instance, refactoring the code to use the new generated code and landing that along with generated code (but not actually generating it at build time). Also, it would seem like you could land the new annotations independently.

This would save you from bitrotting as we figure out the test failures. It also would help in diagnosing the failures.
Unbitrotting.
Attachment #808946 - Attachment is obsolete: true
Attachment #816298 - Flags: review+
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #93)
> Chris, can you break this up into separately landable chunks? For instance,
> refactoring the code to use the new generated code and landing that along
> with generated code (but not actually generating it at build time). Also, it
> would seem like you could land the new annotations independently.
> 
> This would save you from bitrotting as we figure out the test failures. It
> also would help in diagnosing the failures.

Part 2 can be landed at any time. It effectively does nothing, but is highly bitrot-prone.

Part 1 can be landed if I create a small patch for the annotation processor to accompany it to make it deal with the new annotation names. I'll do that shortly.

That'd mitigate the bitrotting somewhat, certainly.

As for landing the generated code seperately to the generator - this doesn't seem pointful. The patch adding the new generated code isn't prone to bitrot, neither is the generator patch.
In addition, the generator will output the exact same code as what was checked in, else the build will fail. That is to say, if the generated code by itself works, then the generated code with the generator also works.
As for landing the refactoring of the C++ code to use the new generated code - that's all dependant upon the generated code having been checked in, else it won't build. This is also almost certainly the patch in which any problem which may exist will lie, as it's the one making interesting changes to the code.
So, really, there's no meaningful way of landing parts 3, 4, or 5 except as a group.

Part 6 can be omitted entirely. That approach might provide an extremely simple way to possibly find the problem, if it lies there.

Has this failure mode been observed elsewhere since it was first reported here? It seems really strange that some apparently entirely unrelated thing is failing due to these changes - the problem is either very interesting or very unrelated.
With this patch, it is now possible to harmlessly land parts 1, 1.5 and 2 to mitigate bitrot.
Removing the changes from this patch that were moved into Part 1.5. Also, unbitrotting.
Attachment #809652 - Attachment is obsolete: true
Attachment #816400 - Flags: review+
Unbitrotting.
Attachment #813637 - Attachment is obsolete: true
Unbitrotting.

(I've not completely lost my mind, but there's work happening on the Progurd patch that depends on this lot, so I have to unbitrot them when I go to do work)
Attachment #812888 - Attachment is obsolete: true
Flagging for tracking, 'cos it'd be awesome if we could get this, Proguard, and a bunch of memory improvements into 28, yielding an incredibly small and fast browser… not to mention getting some bitrotting patches out of the queue.
tracking-fennec: --- → ?
(In reply to Richard Newman [:rnewman] from comment #100)
> Flagging for tracking, 'cos it'd be awesome if we could get this, Proguard,
> and a bunch of memory improvements into 28, yielding an incredibly small and
> fast browser… not to mention getting some bitrotting patches out of the
> queue.

For some value of "Incredibly small". :P
Assignee: chriskitching → bnicholson
tracking-fennec: ? → +
Can someone elaborate on the failures from comment 80? The tbpl link there is no longer valid, so I did a try run (with many mochitest-7 retriggers), but everything looks fine: https://tbpl.mozilla.org/?tree=Try&rev=33f89e86b6b4
The failures in comment 80 were asm.js misdetecting the presence of VFP. I analyzed that there was no relation to this code in comment 81. If it's green then by all means land it.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #103)
> The failures in comment 80 were asm.js misdetecting the presence of VFP. I
> analyzed that there was no relation to this code in comment 81. If it's
> green then by all means land it.

I'll get this unbitrotted. Chances are some more things that can be generated have now appeared and need inclusion.
(In reply to Chris Kitching [:ckitching] from comment #104)
> (In reply to Gian-Carlo Pascutto (:gcp) from comment #103)
> > The failures in comment 80 were asm.js misdetecting the presence of VFP. I
> > analyzed that there was no relation to this code in comment 81. If it's
> > green then by all means land it.
> 
> I'll get this unbitrotted. Chances are some more things that can be
> generated have now appeared and need inclusion.

Note that the try push in comment 102 contains all of the patches rebased onto a tree from this Monday, so feel free to use those if you want.
(In reply to Brian Nicholson (:bnicholson) from comment #105)
> Note that the try push in comment 102 contains all of the patches rebased
> onto a tree from this Monday, so feel free to use those if you want.

Ah. You did my job for me, then. :P

Sorry, I had assumed you'd just done a try push against the old revision on which these patches were originally based. If it's already rebased then by all means land this!
Seems the post-tree-closure flurry added a few new things to be generated and bitrotted all these patches.

Another try run:
https://tbpl.mozilla.org/?tree=Try&rev=2b09f461b076
Attachment #813635 - Attachment is obsolete: true
Attachment #816393 - Attachment is obsolete: true
Attachment #816400 - Attachment is obsolete: true
Attachment #809584 - Attachment is obsolete: true
Attachment #8334111 - Attachment is patch: true
It sounds like this is ready to land. It's been reviewed, and current try builds don't give any indication of someone else's orange.

So let's land it.
Assignee: bnicholson → chriskitching
Status: NEW → ASSIGNED
So this seems to be the problem from Comment 73 all over again. 

I'm exactly as confused about this now as I was in Comment 73, although now there's not even an apparent functional difference any more, and in any case I can't run local tests to try and figure it out.

Unless anyone has any suggestions, we could perhaps undo the wrapping of these problematic methods and sort this problem out in a smaller followup bug that isn't as prone to extreme bitrot?
I've reserved and prepared Fig to work on landing this and the Proguard work.

ssh://hg.mozilla.org/projects/fig

I suggest we incrementally land the safe bits. If they're really safe, we can merge back and then iterate on the rocky bits.

We can, if necessary, land extra debugging support and simply not merge it back to m-c. Use Fig to repro, retrigger, and fix.
Target Milestone: Firefox 28 → ---
(In reply to Richard Newman [:rnewman] from comment #118)
> I've reserved and prepared Fig to work on landing this and the Proguard work.
> 
> ssh://hg.mozilla.org/projects/fig
> 
> I suggest we incrementally land the safe bits. If they're really safe, we
> can merge back and then iterate on the rocky bits.
> 
> We can, if necessary, land extra debugging support and simply not merge it
> back to m-c. Use Fig to repro, retrigger, and fix.

Nifty!

I've prepared a version of the patches that does not generate the code for LayerRenderer$Frame. Since that seems to be the problematic region, hopefully this will make the problem go away (And we can debug the much smaller change).
Once the trees open, I'll stick it on Fig and see what blows up. (I guess this isn't an insane plan?)

If successful, this should get this problem out of Proguard's way at the cost of 4 extra annotations. (As well highlighting the region of interest)
(In reply to Chris Kitching [:ckitching] from comment #119)

> Once the trees open, I'll stick it on Fig and see what blows up. (I guess
> this isn't an insane plan?)

Sounds good.
https://tbpl.mozilla.org/?tree=Fig&rev=62e55cb1e30b

That is fairly conclusively green. Apparently my commit deviating from the commit message pattern means this isn't mergable - so I'll produce equivalent patches for fx-team.

I assume the plan is to land this nowish before it rots? Did I miss anything or should I proceed to dump this on fx-team and see if the world ends?
(In reply to Chris Kitching [:ckitching] from comment #121)

> I assume the plan is to land this nowish before it rots? Did I miss anything
> or should I proceed to dump this on fx-team and see if the world ends?

Land it.
Attachment #8334114 - Attachment is obsolete: true
Attachment #8334108 - Attachment is obsolete: true
Attachment #8334115 - Attachment is obsolete: true
Blocks: 941846
You need to log in before you can comment on or make changes to this bug.