Fix memory leaks in Java binding and some code cleanup.

WebCoreFrameBridge.cpp needs to release NPObject to get proper ref count
because the way V8 binding works.

JObjectWraper holds a weak reference to the Java object to break cycles.

Also fix ScriptController::haveWindowShell to match KJS build, this should be
upstreamed to Chrome project (I am going to do it).

The binding does not use weak reference to hold Java objects, currently
it does not create cycles between Java and JavaScript objects. I didn't see
it is an issue here. A future work is to add logging/debugging code to
make sure global Java references are dropped.

8 files changed, 68 insertions, 43 deletions

diff --git a/V8Binding/jni/jni_class.cpp b/V8Binding/jni/jni_class.cpp
index 1c68d66..a1d321a 100644
--- a/V8Binding/jni/jni_class.cpp
+++ b/V8Binding/jni/jni_class.cpp
@@ -78,11 +78,9 @@ JavaClass::JavaClass(jobject anInstance)
         methodList->append(aMethod);
         env->DeleteLocalRef(aJMethod);
     }    
-#ifdef ANDROID_FIX
     env->DeleteLocalRef(fields);
     env->DeleteLocalRef(methods);
     env->DeleteLocalRef(aClass);
-#endif
 }
 
 JavaClass::~JavaClass() {
diff --git a/V8Binding/jni/jni_instance.cpp b/V8Binding/jni/jni_instance.cpp
index 9204ba7..e2151a9 100644
--- a/V8Binding/jni/jni_instance.cpp
+++ b/V8Binding/jni/jni_instance.cpp
@@ -24,6 +24,8 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
  */
 
+#define LOG_TAG "v8binding"
+
 #include "config.h"
 
 #include "jni_class.h"
@@ -32,33 +34,30 @@
 #include "jni_utility.h"
 
 #include <assert.h>
+#include <utils/Log.h>
 
-#ifdef NDEBUG
-#define JS_LOG(formatAndArgs...) ((void)0)
-#else
-#define JS_LOG(formatAndArgs...) { \
-    fprintf (stderr, "%s:%d -- %s:  ", __FILE__, __LINE__, __FUNCTION__); \
-    fprintf(stderr, formatAndArgs); \
-}
-#endif
- 
 using namespace JSC::Bindings;
 
 JavaInstance::JavaInstance (jobject instance)
 {
     _instance = new JObjectWrapper(instance);
     _class = 0;
+    _refCount = 0;
 }
 
 JavaInstance::~JavaInstance () 
 {
+    _instance = 0;
     delete _class;
 }
 
 JavaClass* JavaInstance::getClass() const 
 {
-    if (_class == 0)
-        _class = new JavaClass(_instance->_instance);
+    if (_class == 0) {
+        jobject local_ref = getLocalRef();
+        _class = new JavaClass(local_ref);
+        getJNIEnv()->DeleteLocalRef(local_ref);
+    }
     return _class;
 }
 
@@ -88,7 +87,7 @@ bool JavaInstance::invokeMethod(const char* methodName, const NPVariant* args, u
         }
     }
     if (method == 0) {
-        JS_LOG ("unable to find an appropiate method\n");
+        LOGW("unable to find an appropiate method\n");
         return false;
     }
     
@@ -110,7 +109,7 @@ bool JavaInstance::invokeMethod(const char* methodName, const NPVariant* args, u
     // The following code can be conditionally removed once we have a Tiger update that
     // contains the new Java plugin.  It is needed for builds prior to Tiger.
     {    
-        jobject obj = _instance->_instance;
+        jobject obj = getLocalRef();
         switch (jMethod->JNIReturnType()){
             case void_type:
                 callJNIMethodIDA<void>(obj, jMethod->methodID(obj), jArgs);
@@ -147,6 +146,7 @@ bool JavaInstance::invokeMethod(const char* methodName, const NPVariant* args, u
             default:
                 break;
         }
+        getJNIEnv()->DeleteLocalRef(obj);
     }
     
     convertJValueToNPVariant(result, jMethod->JNIReturnType(), resultValue);
@@ -163,17 +163,35 @@ JObjectWrapper::JObjectWrapper(jobject instance)
     // Cache the JNIEnv used to get the global ref for this java instanace.
     // It'll be used to delete the reference.
     _env = getJNIEnv();
-        
-    _instance = _env->NewGlobalRef (instance);
+
+    jclass localClsRef = _env->FindClass("java/lang/ref/WeakReference");
+    jmethodID weakRefInit = _env->GetMethodID(localClsRef, "<init>",
+                                    "(Ljava/lang/Object;)V");
+    mWeakRefGet = _env->GetMethodID(localClsRef, "get",
+                                   "()Ljava/lang/Object;");
+
+    jobject weakRef = _env->NewObject(localClsRef, weakRefInit, instance);
+
+    _instance = _env->NewGlobalRef(weakRef);
     
-    JS_LOG ("new global ref %p for %p\n", _instance, instance);
+    LOGV("new global ref %p for %p\n", _instance, instance);
 
     if  (_instance == NULL) {
         fprintf (stderr, "%s:  could not get GlobalRef for %p\n", __PRETTY_FUNCTION__, instance);
     }
+
+    _env->DeleteLocalRef(weakRef);
+    _env->DeleteLocalRef(localClsRef);
 }
 
 JObjectWrapper::~JObjectWrapper() {
-    JS_LOG ("deleting global ref %p\n", _instance);
-    _env->DeleteGlobalRef (_instance);
+    LOGV("deleting global ref %p\n", _instance);
+    _env->DeleteGlobalRef(_instance);
+}
+
+jobject JObjectWrapper::getLocalRef() const {
+    jobject real = _env->CallObjectMethod(_instance, mWeakRefGet);
+    if (!real)
+        LOGE("The real object has been deleted");
+    return _env->NewLocalRef(real);
 }
diff --git a/V8Binding/jni/jni_instance.h b/V8Binding/jni/jni_instance.h
index 0ee487e..26f316d 100644
--- a/V8Binding/jni/jni_instance.h
+++ b/V8Binding/jni/jni_instance.h
@@ -44,7 +44,6 @@ class JavaClass;
 
 class JObjectWrapper
 {
-friend class JavaInstance;
 public:
     JObjectWrapper(jobject instance);    
     ~JObjectWrapper();
@@ -56,10 +55,14 @@ public:
             delete this; 
     }
 
+    jobject getLocalRef() const;
+
 private:
     JNIEnv *_env;
     unsigned int _refCount;
-    jobject _instance;
+    jobject _instance;  // it is a global weak reference.
+
+    jmethodID mWeakRefGet;  // cache WeakReference::Get method id
 };
 
 class JavaInstance
@@ -68,11 +71,22 @@ public:
     JavaInstance(jobject instance);
     ~JavaInstance();
 
+    void ref() { _refCount++; }
+    void deref() 
+    { 
+        if (--_refCount == 0) 
+            delete this; 
+    }
+
     JavaClass* getClass() const;
 
     bool invokeMethod(const char* name, const NPVariant* args, uint32_t argsCount, NPVariant* result);
 
-    jobject javaInstance() const { return _instance->_instance; }
+    // Returns a local reference, and the caller must delete
+    // the returned reference after use.
+    jobject getLocalRef() const {
+        return _instance->getLocalRef();
+    }
 
 private:
     RefPtr<JObjectWrapper> _instance;
diff --git a/V8Binding/jni/jni_npobject.cpp b/V8Binding/jni/jni_npobject.cpp
index 7b315ab..662ea7c 100644
--- a/V8Binding/jni/jni_npobject.cpp
+++ b/V8Binding/jni/jni_npobject.cpp
@@ -35,12 +35,18 @@
 namespace JSC { namespace Bindings {
 static NPObject* AllocJavaNPObject(NPP, NPClass*)
 {
-    return static_cast<NPObject*>(malloc(sizeof(JavaNPObject)));
+    JavaNPObject* obj =
+        static_cast<JavaNPObject*>(malloc(sizeof(JavaNPObject)));
+    if (obj == 0)
+        return 0;
+    bzero(obj, sizeof(JavaNPObject));
+    return reinterpret_cast<NPObject*>(obj);
 }
 
 static void FreeJavaNPObject(NPObject* npobj)
 {
     JavaNPObject* obj = reinterpret_cast<JavaNPObject*>(npobj);
+    obj->_instance = 0;  // free does not call the destructor
     free(obj);
 }
 
@@ -71,7 +77,7 @@ NPObject* JavaInstanceToNPObject(JavaInstance* instance) {
 // Returns null if obj is not a wrapper of JavaInstance
 JavaInstance* ExtractJavaInstance(NPObject* obj) {
     if (obj->_class == &JavaNPObjectClass) {
-        return reinterpret_cast<JavaNPObject*>(obj)->_instance;
+        return reinterpret_cast<JavaNPObject*>(obj)->_instance.get();
     }
     return 0;
 }
@@ -137,10 +143,13 @@ bool JavaNPObject_GetProperty(NPObject* obj, NPIdentifier identifier, NPVariant*
         return false;
     }
 
-    jvalue value = getJNIField(instance->javaInstance(),
+    jobject local_ref = instance->getLocalRef();
+    jvalue value = getJNIField(local_ref,
                                field->getJNIType(),
                                field->name(),
                                field->type());
+    getJNIEnv()->DeleteLocalRef(local_ref);
+
     convertJValueToNPVariant(value, field->getJNIType(), result);
 
     return true;
diff --git a/V8Binding/jni/jni_npobject.h b/V8Binding/jni/jni_npobject.h
index 9ba8ced..943b661 100644
--- a/V8Binding/jni/jni_npobject.h
+++ b/V8Binding/jni/jni_npobject.h
@@ -29,15 +29,14 @@
 #include "npruntime.h"
 #include "jni_runtime.h"
 
+#include <wtf/RefPtr.h>
 #include <JavaVM/jni.h>
 
 namespace JSC { namespace Bindings {
 
 struct JavaNPObject {
     NPObject _object;
-    JavaInstance* _instance;
-
-    ~JavaNPObject() { delete _instance; }
+    RefPtr<JavaInstance> _instance;
 };
 
 NPObject* JavaInstanceToNPObject(JavaInstance* instance);
diff --git a/V8Binding/jni/jni_runtime.cpp b/V8Binding/jni/jni_runtime.cpp
index 9c87c54..2fbcc5d 100644
--- a/V8Binding/jni/jni_runtime.cpp
+++ b/V8Binding/jni/jni_runtime.cpp
@@ -29,15 +29,6 @@
 #include "jni_runtime.h"
 #include "jni_utility.h"
 
-#ifdef NDEBUG
-#define JS_LOG(formatAndArgs...) ((void)0)
-#else
-#define JS_LOG(formatAndArgs...) { \
-    fprintf (stderr, "%s:%d -- %s:  ", __FILE__, __LINE__, __FUNCTION__); \
-    fprintf(stderr, formatAndArgs); \
-}
-#endif
-
 using namespace JSC::Bindings;
 
 JavaParameter::JavaParameter (JNIEnv *env, jstring type)
@@ -100,9 +91,7 @@ JavaMethod::JavaMethod (JNIEnv *env, jobject aMethod)
     jclass modifierClass = env->FindClass("java/lang/reflect/Modifier");
     int modifiers = callJNIMethod<jint>(aMethod, "getModifiers", "()I");
     _isStatic = (bool)callJNIStaticMethod<jboolean>(modifierClass, "isStatic", "(I)Z", modifiers);
-#ifdef ANDROID_FIX
     env->DeleteLocalRef(modifierClass);
-#endif
 }
 
 JavaMethod::~JavaMethod() 
diff --git a/V8Binding/jni/jni_utility.cpp b/V8Binding/jni/jni_utility.cpp
index 5ab2041..90cf603 100644
--- a/V8Binding/jni/jni_utility.cpp
+++ b/V8Binding/jni/jni_utility.cpp
@@ -359,7 +359,7 @@ jvalue convertNPVariantToJValue(NPVariant value, JNIType _JNIType, const char* j
             if (type == NPVariantType_Object) {
                 NPObject* objectImp = NPVARIANT_TO_OBJECT(value);
                 if (JavaInstance* instance = ExtractJavaInstance(objectImp))
-                    result.l = instance->javaInstance();
+                    result.l = instance->getLocalRef();
             }
             
             // Now convert value to a string if the target type is a java.lang.string, and we're not
diff --git a/V8Binding/jni/jni_utility.h b/V8Binding/jni/jni_utility.h
index dfa97ef..bb755bf 100644
--- a/V8Binding/jni/jni_utility.h
+++ b/V8Binding/jni/jni_utility.h
@@ -211,9 +211,7 @@ static T callJNIMethodV(jobject obj, const char *name, const char *sig, va_list
             jmethodID mid = env->GetMethodID(cls, name, sig);
             if ( mid != NULL )
             {
-#ifdef ANDROID_FIX // Avoids references to cls without popping the local frame.
                 env->DeleteLocalRef(cls);
-#endif
                 return JNICaller<T>::callV(obj, mid, args);
             }
             else