The Android team has made some edits in the WebCore/bridge/jni directory and we would like to contribute them back upstream.
Created attachment 43759 [details] Patch
> Index: WebCore/bridge/jni/jni_instance.h > =================================================================== > --- WebCore/bridge/jni/jni_instance.h (revision 51337) > +++ WebCore/bridge/jni/jni_instance.h (working copy) > @@ -47,6 +47,10 @@ friend class JavaField; > friend class JavaInstance; > friend class JavaMethod; > > +public: > + jobject instance() const { return _instance; } > + void setInstance(jobject instance) { _instance = instance; } > + > protected: > JObjectWrapper(jobject instance); > ~JObjectWrapper(); > @@ -89,12 +93,10 @@ public: > JSValue booleanValue() const; > > protected: > + JavaInstance(jobject instance, PassRefPtr<RootObject>); > virtual void virtualBegin(); > virtual void virtualEnd(); > > -private: > - JavaInstance(jobject instance, PassRefPtr<RootObject>); > - > RefPtr<JObjectWrapper> _instance; > mutable JavaClass *_class; > }; FYI, a little background for this change that I can add to the ChangeLog on landing: In WebKit/android (to be upstreamed later) we have a class that inherits from JavaInstance and calls it's parent class constructor. That's why I have moved the constructor from private to protected. Also within that class, we access the internals of the _instance member which is why I added a getter/setter to JObjectWrapper.
Comment on attachment 43759 [details] Patch Looks sane enough, but as you note in the bug, the ChangeLog could be improved. Going to CC some folks who may have worked on this code.
Comment on attachment 43759 [details] Patch So were these leaks before? Should the leak bot have been able to detect these? Or does this just make GC easier for the Java VM?
(In reply to comment #4) > (From update of attachment 43759 [details]) > So were these leaks before? Should the leak bot have been able to detect > these? Or does this just make GC easier for the Java VM? DeleteLocalRef is a hint to the garbage collector (like setting a local variable to null in java), and these were potential leaks before. Normally local references are automatically collected when they go out of scope (i.e. when we return to Java) and this is likely the case now and is why the leak bots detect nothing. But as this is inside a constructor, we have no guarantees where it is being called from. Consider the case that the objects were allocated in a loop - without a call to DeleteLocalRef the references would be leaked until we return to Java, which is a concern on devices with limited memory.
So these were found by inspection? or due to OOM crashes on android in certain uses? I'm ready to r+ this patch, although it would be best if we had a way to test it. Sadly Java is disabled in DumpRenderTree and although we could turn it on on a per-test basis with layoutTestController.overridePreference(), there may be a reason why Java is off that I don't know of.
Comment on attachment 43759 [details] Patch These could have been made separate changes. But overall this change looks fine, and there are certainly enough Java experts CC'd on this bug to correct me if I'm wrong. :) I see you listed in committers.py, so you can either commit this yourself or cq+ it yourself. Thanks for the patch!
(In reply to comment #6) > So these were found by inspection? or due to OOM crashes on android in certain > uses? By inspection, it was added as a safety mechanism. I will land this manually in the morning (night time here :)) so I can update the changelog comments a little and watch the build bots properly. Thanks!
Comment on attachment 43759 [details] Patch cq- for manual landing.
Thanks!
Landed as r51379.