Bug 31824 - [Android] Upstream Android changes to WebCore/bridge/jni
Summary: [Android] Upstream Android changes to WebCore/bridge/jni
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Ben Murdoch
Depends on:
Reported: 2009-11-24 03:24 PST by Ben Murdoch
Modified: 2009-11-25 03:25 PST (History)
6 users (show)

See Also:

Patch (4.05 KB, patch)
2009-11-24 05:00 PST, Ben Murdoch
eric: review+
benm: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Murdoch 2009-11-24 03:24:59 PST
The Android team has made some edits in the WebCore/bridge/jni directory and we would like to contribute them back upstream.
Comment 1 Ben Murdoch 2009-11-24 05:00:21 PST
Created attachment 43759 [details]
Comment 2 Ben Murdoch 2009-11-24 05:41:51 PST
> 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 3 Eric Seidel (no email) 2009-11-24 10:07:33 PST
Comment on attachment 43759 [details]

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 4 Eric Seidel (no email) 2009-11-24 10:09:27 PST
Comment on 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?
Comment 5 Ben Murdoch 2009-11-24 11:26:06 PST
(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.
Comment 6 Eric Seidel (no email) 2009-11-24 11:32:01 PST
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 7 Eric Seidel (no email) 2009-11-24 11:33:49 PST
Comment on attachment 43759 [details]

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!
Comment 8 Ben Murdoch 2009-11-24 11:46:36 PST
(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.

Comment 9 Ben Murdoch 2009-11-24 11:47:07 PST
Comment on attachment 43759 [details]

cq- for manual landing.
Comment 10 Eric Seidel (no email) 2009-11-24 11:47:55 PST
Comment 11 Ben Murdoch 2009-11-25 03:25:25 PST
Landed as r51379.