changeset 9622:42b461505f27

8089681: WebView leaks memory when containing object acts as javascript callback handler Reviewed-by: kcr, azvegint
author mbilla
date Fri, 11 Mar 2016 12:34:31 -0800
parents 33e0a9be1383
children 6ef9d5c0ad46
files modules/web/src/main/native/Source/WebCore/bridge/jni/JNIUtility.cpp modules/web/src/main/native/Source/WebCore/bridge/jni/JNIUtility.h modules/web/src/main/native/Source/WebCore/bridge/jni/JobjectWrapper.cpp modules/web/src/main/native/Source/WebCore/bridge/jni/JobjectWrapper.h modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JNIUtilityPrivate.cpp modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaArrayJSC.cpp modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaClassJSC.cpp modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaFieldJSC.cpp modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp
diffstat 9 files changed, 254 insertions(+), 12 deletions(-) [+]
line wrap: on
line diff
--- a/modules/web/src/main/native/Source/WebCore/bridge/jni/JNIUtility.cpp	Thu Mar 10 14:16:20 2016 -0800
+++ b/modules/web/src/main/native/Source/WebCore/bridge/jni/JNIUtility.cpp	Fri Mar 11 12:34:31 2016 -0800
@@ -127,6 +127,14 @@
     JNIEnv* env = getJNIEnv();
     jmethodID mid = 0;
 
+    // Since obj is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JNIUtility::getMethodID", jlinstance);
+        return mid;
+    }
+
     if (env) {
         jclass cls = env->GetObjectClass(obj);
         if (cls) {
@@ -309,6 +317,15 @@
     jvalue result;
 
     memset(&result, 0, sizeof(jvalue));
+
+    // Since obj is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JNIUtility::getJNIField", jlinstance);
+        return result;
+    }
+
     if (obj && jvm && env) {
         jclass cls = env->GetObjectClass(obj);
         if (cls) {
@@ -363,8 +380,19 @@
 
 jvalue callJNIMethod(jobject object, JavaType returnType, const char* name, const char* signature, jvalue* args)
 {
+    jvalue result;
+    memset(&result, 0, sizeof(jvalue));
+
+    // Since object is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(object, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JNIUtility::callJNIMethod", jlinstance);
+        return result;
+    }
+
     jmethodID methodId = getMethodID(object, name, signature);
-    jvalue result;
+
     switch (returnType) {
     case JavaTypeVoid:
         callJNIMethodIDA<void>(object, methodId, args);
--- a/modules/web/src/main/native/Source/WebCore/bridge/jni/JNIUtility.h	Thu Mar 10 14:16:20 2016 -0800
+++ b/modules/web/src/main/native/Source/WebCore/bridge/jni/JNIUtility.h	Fri Mar 11 12:34:31 2016 -0800
@@ -28,6 +28,7 @@
 
 #if ENABLE(JAVA_BRIDGE)
 
+#include "JavaRef.h"
 #include "JavaType.h"
 
 #if OS(MAC_OS_X)
@@ -188,6 +189,14 @@
     JavaVM* jvm = getJavaVM();
     JNIEnv* env = getJNIEnv();
 
+    // Since obj is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JNIUtility::callJNIMethodV", jlinstance);
+        return 0;
+    }
+
     if (obj && jvm && env) {
         jclass cls = env->GetObjectClass(obj);
         if (cls) {
@@ -230,6 +239,14 @@
     JavaVM* jvm = getJavaVM();
     JNIEnv* env = getJNIEnv();
 
+    // Since obj is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JNIUtility::callJNIMethodV<void>", jlinstance);
+        return;
+    }
+
     if (obj && jvm && env) {
         jclass cls = env->GetObjectClass(obj);
         if (cls) {
@@ -254,6 +271,14 @@
 template<>
 inline void callJNIMethod<void>(jobject obj, const char* methodName, const char* methodSignature, ...)
 {
+    // Since obj is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JNIUtility::callJNIMethod<void>", jlinstance);
+        return;
+    }
+
     va_list args;
     va_start(args, methodSignature);
 
--- a/modules/web/src/main/native/Source/WebCore/bridge/jni/JobjectWrapper.cpp	Thu Mar 10 14:16:20 2016 -0800
+++ b/modules/web/src/main/native/Source/WebCore/bridge/jni/JobjectWrapper.cpp	Fri Mar 11 12:34:31 2016 -0800
@@ -31,7 +31,7 @@
 
 using namespace JSC::Bindings;
 
-JobjectWrapper::JobjectWrapper(jobject instance)
+JobjectWrapper::JobjectWrapper(jobject instance, bool useGlobalRef)
 {
     ASSERT(instance);
 
@@ -39,7 +39,11 @@
     // It'll be used to delete the reference.
     m_env = getJNIEnv();
 
-    m_instance = m_env->NewGlobalRef(instance);
+    if (useGlobalRef) {
+        m_instance = m_env->NewGlobalRef(instance);
+    } else {
+        m_instance = m_env->NewWeakGlobalRef(instance);
+    }
 
     if (!m_instance)
         LOG_ERROR("Could not get GlobalRef for %p", instance);
@@ -47,7 +51,13 @@
 
 JobjectWrapper::~JobjectWrapper()
 {
-    m_env->DeleteGlobalRef(m_instance);
+    jobjectRefType objreftype = m_env->GetObjectRefType(m_instance);
+
+    if(objreftype == JNIWeakGlobalRefType) {
+        m_env->DeleteWeakGlobalRef(m_instance);
+    } else {
+        m_env->DeleteGlobalRef(m_instance);
+    }
 }
 
 #endif // ENABLE(JAVA_BRIDGE)
--- a/modules/web/src/main/native/Source/WebCore/bridge/jni/JobjectWrapper.h	Thu Mar 10 14:16:20 2016 -0800
+++ b/modules/web/src/main/native/Source/WebCore/bridge/jni/JobjectWrapper.h	Fri Mar 11 12:34:31 2016 -0800
@@ -39,14 +39,14 @@
 
 class JobjectWrapper : public RefCounted<JobjectWrapper> {
 public:
-    static PassRefPtr<JobjectWrapper> create(jobject object) { return adoptRef(new JobjectWrapper(object)); }
+    static PassRefPtr<JobjectWrapper> create(jobject object, bool useGlobalRef = false) { return adoptRef(new JobjectWrapper(object, useGlobalRef)); }
     ~JobjectWrapper();
 
     jobject instance() const { return m_instance; }
     void setInstance(jobject instance) { m_instance = instance; }
 
 private:
-    JobjectWrapper(jobject);
+    JobjectWrapper(jobject, bool useGlobalRef);
 
     jobject m_instance;
     JNIEnv* m_env;
--- a/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JNIUtilityPrivate.cpp	Thu Mar 10 14:16:20 2016 -0800
+++ b/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JNIUtilityPrivate.cpp	Fri Mar 11 12:34:31 2016 -0800
@@ -212,12 +212,26 @@
                     // Unwrap a Java instance.
                     JavaRuntimeObject* runtimeObject = static_cast<JavaRuntimeObject*>(object);
                     JavaInstance* instance = runtimeObject->getInternalJavaInstance();
-                    if (instance)
+                    if (instance) {
+                        // Since instance->javaInstance() is WeakGlobalRef, creating a localref to safeguard javaInstance() from GC
+                        JLObject jlinstance(instance->javaInstance(), true);
+                        if (!jlinstance) {
+                            LOG_ERROR("Could not get javaInstance for %p in JNIUtilityPrivate::convertValueToJValue", jlinstance);
+                            return result;
+                        }
                         result.l = instance->javaInstance();
+                    }
                 } else if (object->classInfo() == RuntimeArray::info()) {
                     // Input is a JavaScript Array that was originally created from a Java Array
                     RuntimeArray* imp = static_cast<RuntimeArray*>(object);
                     JavaArray* array = static_cast<JavaArray*>(imp->getConcreteArray());
+
+                    // Since array->javaArray() is WeakGlobalRef, creating a localref to safeguard javaInstance() from GC
+                    JLObject jlinstancearray(array->javaArray(), true);
+                    if (!jlinstancearray) {
+                        LOG_ERROR("Could not get javaArrayInstance for %p in JNIUtilityPrivate::convertValueToJValue", jlinstancearray);
+                        return result;
+                    }
                     result.l = array->javaArray();
                 } else if ((!result.l && (!strcmp(javaClassName, "java.lang.Object")))
                            || (!strcmp(javaClassName, "netscape.javascript.JSObject"))) {
@@ -402,6 +416,15 @@
 }
 
 jthrowable dispatchJNICall(int count, RootObject* rootObject, jobject obj, bool isStatic, JavaType returnType, jmethodID methodId, jobject* args, jvalue& result, jobject accessControlContext) {
+
+    // Since obj is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JNIUtilityPrivate::dispatchJNICall", jlinstance);
+        return NULL;
+    }
+
     JNIEnv* env = getJNIEnv();
     jclass objClass = env->GetObjectClass(obj);
     jobject rmethod = env->ToReflectedMethod(objClass, methodId, isStatic);
--- a/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaArrayJSC.cpp	Thu Mar 10 14:16:20 2016 -0800
+++ b/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaArrayJSC.cpp	Fri Mar 11 12:34:31 2016 -0800
@@ -54,11 +54,19 @@
     : Array(rootObject)
 {
     m_array = JobjectWrapper::create(array);
+    // Since m_array->instance() is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlarrayinstance(m_array->instance(), true);
+
+    if (!jlarrayinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaArray Constructor", jlarrayinstance);
+        return;
+    }
+
     // Java array are fixed length, so we can cache length.
     JNIEnv* env = getJNIEnv();
     m_length = env->GetArrayLength(static_cast<jarray>(m_array->instance()));
     m_type = strdup(type);
-    m_accessControlContext = JobjectWrapper::create(accessControlContext);
+    m_accessControlContext = JobjectWrapper::create(accessControlContext, true);
 }
 
 JavaArray::~JavaArray()
@@ -73,6 +81,14 @@
 
 void JavaArray::setValueAt(ExecState* exec, unsigned index, JSValue aValue) const
 {
+    // Since javaArray() is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(javaArray(), true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaArray::setValueAt", jlinstance);
+        return;
+    }
+
     JNIEnv* env = getJNIEnv();
     char* javaClassName = 0;
 
@@ -149,6 +165,14 @@
 
 JSValue JavaArray::valueAt(ExecState* exec, unsigned index) const
 {
+    // Since javaArray() is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(javaArray(), true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaArray::valueAt", jlinstance);
+        return jsUndefined();
+    }
+
     JNIEnv* env = getJNIEnv();
     JavaType arrayType = javaTypeFromPrimitiveType(m_type[1]);
     switch (arrayType) {
--- a/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaClassJSC.cpp	Thu Mar 10 14:16:20 2016 -0800
+++ b/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaClassJSC.cpp	Fri Mar 11 12:34:31 2016 -0800
@@ -40,6 +40,15 @@
 
 JavaClass::JavaClass(jobject anInstance, RootObject* rootObject, jobject accessControlContext)
 {
+    // Since anInstance is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(anInstance, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaClass::JavaClass", jlinstance);
+        m_name = fastStrDup("<Unknown>");
+        return;
+    }
+
     jobject aClass = callJNIMethod<jobject>(anInstance, "getClass", "()Ljava/lang/Class;");
 
     if (!aClass) {
--- a/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaFieldJSC.cpp	Thu Mar 10 14:16:20 2016 -0800
+++ b/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaFieldJSC.cpp	Fri Mar 11 12:34:31 2016 -0800
@@ -74,7 +74,23 @@
 
     JSValue jsresult = jsUndefined();
     jobject jfield = m_field->instance();
+    // Since jfield is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlfield(jfield, true);
+
+    if (!jlfield) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaField::valueFromInstance", jlfield);
+        return jsresult;
+    }
+
     jobject jinstance = instance->javaInstance();
+    // Since jinstance is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(jinstance, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaField::valueFromInstance", jlinstance);
+        return jsresult;
+    }
+
     switch (m_type) {
     case JavaTypeArray:
     case JavaTypeObject:
@@ -139,7 +155,22 @@
     LOG(LiveConnect, "JavaField::setValueToInstance setting value %s to %s", String(name().impl()).utf8().data(), aValue.toString(exec)->value(exec).ascii().data());
 
     jobject jfield = m_field->instance();
+    // Since jfield is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlfield(jfield, true);
+
+    if (!jlfield) {
+        LOG_ERROR("Could not get Instance for %p in JavaField::setValueToInstance", jlfield);
+        return;
+    }
+
     jobject jinstance = instance->javaInstance();
+    // Since jinstance is WeakGlobalRef, creating a localref to safeguard javaInstance() from GC
+    JLObject jlinstance(jinstance, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaField::setValueToInstance", jlinstance);
+        return;
+    }
 
     switch (m_type) {
     case JavaTypeArray:
--- a/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp	Thu Mar 10 14:16:20 2016 -0800
+++ b/modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaInstanceJSC.cpp	Fri Mar 11 12:34:31 2016 -0800
@@ -56,7 +56,7 @@
 {
     m_instance = JobjectWrapper::create(instance);
     m_class = 0;
-    m_accessControlContext = JobjectWrapper::create(accessControlContext);
+    m_accessControlContext = JobjectWrapper::create(accessControlContext, true);
 }
 
 JavaInstance::~JavaInstance()
@@ -83,6 +83,15 @@
 
 Class* JavaInstance::getClass() const
 {
+    jobject obj = m_instance->instance();
+    // Since m_instance->instance() is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaInstance::getClass", jlinstance);
+        return NULL;
+    }
+
     if (!m_class) {
         jobject acc  = accessControlContext();
         m_class = new JavaClass (m_instance->instance(), rootObject(), acc);
@@ -95,7 +104,16 @@
     JSLockHolder lock(exec);
 
     jobject obj = m_instance->instance();
+    // Since m_instance->instance() is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaInstance::stringValue", jlinstance);
+        return jsUndefined();
+    }
+
     jobject acc  = accessControlContext();
+
     jmethodID methodId = getMethodID(obj, "toString", "()Ljava/lang/String;");
     jvalue result;
     jthrowable ex = dispatchJNICall(0, rootObject(), obj, false,
@@ -121,20 +139,50 @@
 }
 
 static JSValue numberValueForCharacter(jobject obj) {
+
+    // Since obj is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaInstance::numberValueForCharacter", jlinstance);
+        return jsUndefined();
+    }
+
     return jsNumber((int) callJNIMethod<jchar>(obj, "charValue", "()C"));
 }
 
 static JSValue numberValueForNumber(jobject obj) {
-return jsNumber(callJNIMethod<jdouble>(obj, "doubleValue", "()D"));
+
+    // Since obj is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaInstance::numberValueForNumber", jlinstance);
+        return jsUndefined();
+    }
+
+    return jsNumber(callJNIMethod<jdouble>(obj, "doubleValue", "()D"));
 }
 
 
 JSValue JavaInstance::numberValue(ExecState*) const
 {
     jobject obj = m_instance->instance();
+    // Since obj is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaInstance::numberValue", jlinstance);
+        return jsUndefined();
+    }
+
     JavaClass* aClass = static_cast<JavaClass*>(getClass());
+
+    if (!aClass)
+        return jsUndefined();
+
     if (aClass->isCharacterClass())
-      return numberValueForCharacter(obj);
+        return numberValueForCharacter(obj);
     if (aClass->isBooleanClass())
         return jsNumber((int)
                         // Replaced the following line to work around possible GCC bug, see RT-22725
@@ -145,6 +193,14 @@
 
 JSValue JavaInstance::booleanValue() const
 {
+    // Since m_instance->instance() is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(m_instance->instance(), true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaInstance::booleanValue", jlinstance);
+        return jsUndefined();
+    }
+
     // Changed the call to work around possible GCC bug, see RT-22725
     jboolean booleanValue = callJNIMethod(m_instance->instance(), JavaTypeBoolean, "booleanValue", "()Z", 0).z;
     return jsBoolean(booleanValue);
@@ -188,7 +244,12 @@
 
 JSValue JavaInstance::getMethod(ExecState* exec, PropertyName propertyName)
 {
-    Method *method = getClass()->methodNamed(propertyName, this);
+    JavaClass* aClass = static_cast<JavaClass*>(getClass());
+
+    if (!aClass)
+        return jsUndefined();
+
+    Method *method = aClass->methodNamed(propertyName, this);
     return JavaRuntimeMethod::create(exec, exec->lexicalGlobalObject(), propertyName.publicName(), method);
 }
 
@@ -227,6 +288,16 @@
     }
 
     const JavaMethod* jMethod = static_cast<const JavaMethod*>(method);
+
+    jobject obj = m_instance->instance();
+    // Since m_instance->instance() is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaInstance::invokeMethod", jlinstance);
+        return jsUndefined();
+    }
+
     LOG(LiveConnect, "JavaInstance::invokeMethod call %s %s on %p", String(jMethod->name().impl()).utf8().data(), jMethod->signature(), m_instance->instance());
 
     if (jMethod->numParameters() != count) {
@@ -259,6 +330,14 @@
     bool handled = false;
     if (rootObject->nativeHandle()) {
         jobject obj = m_instance->instance();
+        // Since m_instance->instance() is WeakGlobalRef, creating a localref to safeguard instance() from GC
+        JLObject jlinstance(obj, true);
+
+        if (!jlinstance) {
+            LOG_ERROR("Could not get javaInstance for %p in JavaInstance::invokeMethod", jlinstance);
+            return jsUndefined();
+        }
+
         const char *callingURL = 0; // FIXME, need to propagate calling URL to Java
         jmethodID methodId = getMethodID(obj, jMethod->name().utf8().data(), jMethod->signature());
 
@@ -357,10 +436,23 @@
         return stringValue(exec);
     if (hint == PreferNumber)
         return numberValue(exec);
+
     JavaClass* aClass = static_cast<JavaClass*>(getClass());
+    if (!aClass)
+        return jsUndefined();
+
     if (aClass->isStringClass())
         return stringValue(exec);
 
+    jobject obj = m_instance->instance();
+    // Since m_instance->instance() is WeakGlobalRef, creating a localref to safeguard instance() from GC
+    JLObject jlinstance(obj, true);
+
+    if (!jlinstance) {
+        LOG_ERROR("Could not get javaInstance for %p in JavaInstance::defaultValue", jlinstance);
+        return jsUndefined();
+    }
+
     if (aClass->isNumberClass())
         return numberValueForNumber(m_instance->instance());
     if (aClass->isCharacterClass())