Bug 31824 – [Android] Upstream Android changes to WebCore/bridge/jni

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Murdoch]

The Android team has made some edits in the WebCore/bridge/jni directory and we would like to contribute them back upstream.

[Ben Murdoch]

Created attachment 43759 [details]
Patch

[Ben Murdoch]

> 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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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?

[Ben Murdoch]

(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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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!

[Ben Murdoch]

(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!

[Ben Murdoch]

Comment on attachment 43759 [details]
Patch

cq- for manual landing.

[Eric Seidel (no email)]

Thanks!

[Ben Murdoch]

Landed as r51379.