Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-25599] Android: Fix Java object referencing #9675

Merged
merged 5 commits into from
Feb 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
*/
package org.appcelerator.kroll.runtime.v8;

import java.lang.ref.Reference;
import java.lang.ref.SoftReference;
import java.lang.ref.WeakReference;
import java.util.HashMap;

Expand Down Expand Up @@ -53,8 +55,8 @@ public static void destroyReference(long key)
{
Log.d(TAG, "Destroying reference under key: " + key, Log.DEBUG_MODE);
Object obj = references.remove(key);
if (obj instanceof WeakReference) {
obj = ((WeakReference<?>) obj).get();
if (obj instanceof Reference) {
obj = ((Reference<?>) obj).get();
}
// If it's an V8Object, set the ptr to 0, because the proxy is dead on C++ side
// This *should* prevent the native code from trying to reconstruct the proxy for any reason
Expand All @@ -77,20 +79,35 @@ public static void makeWeakReference(long key)
{
Log.d(TAG, "Downgrading to weak reference for key: " + key, Log.DEBUG_MODE);
Object ref = references.get(key);
references.remove(key);
references.put(key, new WeakReference<Object>(ref));
}

/**
* Make the reference strong again to prevent future collection. This method will return null if the weak reference
* Makes the reference "soft" which which are cleared at the discretion of the garbage collector.
*
* @param key the key for the reference to soften.
*/
public static void makeSoftReference(long key)
{
Log.d(TAG, "Downgrading to soft reference for key: " + key, Log.DEBUG_MODE);
Object ref = references.get(key);
references.remove(key);
references.put(key, new SoftReference<Object>(ref));
}

/**
* Make the reference strong again to prevent future collection. This method will return null if the reference
* was invalidated and the object collected.
*
* @param key the key for the reference.
* @return the referenced object if the reference is still valid.
*/
public static Object clearWeakReference(long key)
public static Object clearReference(long key)
{
Log.d(TAG, "Upgrading weak reference to strong for key: " + key, Log.DEBUG_MODE);
Log.d(TAG, "Upgrading reference to strong for key: " + key, Log.DEBUG_MODE);
Object ref = getReference(key);
references.remove(key);
references.put(key, ref);
return ref;
}
Expand All @@ -105,8 +122,8 @@ public static Object clearWeakReference(long key)
public static Object getReference(long key)
{
Object ref = references.get(key);
if (ref instanceof WeakReference) {
ref = ((WeakReference<?>) ref).get();
if (ref instanceof Reference) {
ref = ((Reference<?>) ref).get();
}
return ref;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,10 @@ public static boolean isEmulator()
@Override
public void initRuntime()
{
boolean useGlobalRefs = true;
boolean useGlobalRefs = false;
KrollApplication application = getKrollApplication();
TiDeployData deployData = application.getDeployData();

if (isEmulator()) {
Log.d(TAG, "Emulator detected, storing global references in a global Map", Log.DEBUG_MODE);
useGlobalRefs = false;
}

if (!libLoaded) {
System.loadLibrary("c++_shared");
System.loadLibrary("kroll-v8");
Expand Down
6 changes: 4 additions & 2 deletions android/runtime/v8/src/native/JNIUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ jmethodID JNIUtil::v8FunctionInitMethod = NULL;
jmethodID JNIUtil::referenceTableCreateReferenceMethod = NULL;
jmethodID JNIUtil::referenceTableDestroyReferenceMethod = NULL;
jmethodID JNIUtil::referenceTableMakeWeakReferenceMethod = NULL;
jmethodID JNIUtil::referenceTableClearWeakReferenceMethod = NULL;
jmethodID JNIUtil::referenceTableMakeSoftReferenceMethod = NULL;
jmethodID JNIUtil::referenceTableClearReferenceMethod = NULL;
jmethodID JNIUtil::referenceTableGetReferenceMethod = NULL;

jint JNIUtil::krollRuntimeDontIntercept = -1;
Expand Down Expand Up @@ -366,7 +367,8 @@ void JNIUtil::initCache()
referenceTableCreateReferenceMethod = getMethodID(referenceTableClass, "createReference", "(Ljava/lang/Object;)J", true);
referenceTableDestroyReferenceMethod = getMethodID(referenceTableClass, "destroyReference", "(J)V", true);
referenceTableMakeWeakReferenceMethod = getMethodID(referenceTableClass, "makeWeakReference", "(J)V", true);
referenceTableClearWeakReferenceMethod = getMethodID(referenceTableClass, "clearWeakReference", "(J)Ljava/lang/Object;", true);
referenceTableMakeSoftReferenceMethod = getMethodID(referenceTableClass, "makeSoftReference", "(J)V", true);
referenceTableClearReferenceMethod = getMethodID(referenceTableClass, "clearReference", "(J)Ljava/lang/Object;", true);
referenceTableGetReferenceMethod = getMethodID(referenceTableClass, "getReference", "(J)Ljava/lang/Object;", true);

jfieldID dontInterceptField = env->GetStaticFieldID(krollRuntimeClass, "DONT_INTERCEPT", "I");
Expand Down
3 changes: 2 additions & 1 deletion android/runtime/v8/src/native/JNIUtil.h
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ class JNIUtil
static jmethodID referenceTableCreateReferenceMethod;
static jmethodID referenceTableDestroyReferenceMethod;
static jmethodID referenceTableMakeWeakReferenceMethod;
static jmethodID referenceTableClearWeakReferenceMethod;
static jmethodID referenceTableMakeSoftReferenceMethod;
static jmethodID referenceTableClearReferenceMethod;
static jmethodID referenceTableGetReferenceMethod;

static jint krollRuntimeDontIntercept;
Expand Down
21 changes: 14 additions & 7 deletions android/runtime/v8/src/native/JavaObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ using namespace v8;

namespace titanium {

bool JavaObject::useGlobalRefs = true;
bool JavaObject::useGlobalRefs = false;

#ifdef TI_DEBUG
static struct {
Expand Down Expand Up @@ -62,8 +62,9 @@ JavaObject::~JavaObject()
}

// Make sure we wipe the persistent, in case we called delete on the proxy and didn't get deleted as a result of the NativeObject WeakCallback
if (persistent().IsEmpty())
if (persistent().IsEmpty()) {
return;
}
persistent().Reset();
}

Expand Down Expand Up @@ -123,14 +124,17 @@ void JavaObject::MakeJSWeak()
// but this time, we say call us back as a finalizer so we can resurrect the
// object (save it from really being GCd by V8) and move it's Java object twin
// to a weak reference in the JVM. (where we can track when that gets GC'd by the JVM to call back and kill this)
persistent().SetWeak(this, DetachCallback, v8::WeakCallbackType::kFinalizer); // MUST BE kFinalizer or our object cannot be resurrected!
persistent().MarkIndependent();
if (!isDetached()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if you're using this quard, it's specifically checking isWeakRef_ is true, which only happens if #MakeJavaWeak() was called (and you've commented that call out from #detach() now). So I'm not sure this guard will ever be false now.

persistent().SetWeak(this, DetachCallback, v8::WeakCallbackType::kFinalizer); // MUST BE kFinalizer or our object cannot be resurrected!
persistent().MarkIndependent();
}
}

void JavaObject::detach()
{
// WAIT A SECOND V8!!! DON'T KILL MY OBJECT YET! THE JVM MAY STILL WANT IT!
persistent().ClearWeak(); // Make JS Strong Again!
persistent().MarkActive();

// if the JVM side is a weak reference or we have no object wrapped, don't do anything else
if (isDetached()) {
Expand Down Expand Up @@ -168,12 +172,13 @@ void JavaObject::MakeJavaStrong()
ASSERT(refTableKey_ != 0);
JNIEnv *env = JNIUtil::getJNIEnv();
ASSERT(env != NULL);
jobject stored = ReferenceTable::clearWeakReference(refTableKey_);
jobject stored = ReferenceTable::clearReference(refTableKey_);
if (stored == NULL) {
// Sanity check. Did we get into a state where it was weak on Java, got GC'd but the C++ proxy didn't get deleted yet?
LOGE(TAG, "!!! OH NO! We tried to move a weak Java object back to strong, but it's aleady been GC'd by JVM! We're in a bad state! Key: %d", refTableKey_);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this fix should definitely be applied. I assume if the store ref is NULL, trying to delete the local ref will be very bad.

env->DeleteLocalRef(stored);
}
env->DeleteLocalRef(stored);
} else {
// New entry, make sure we have no key, have an object, get a new key
ASSERT(javaObject_ != NULL);
Expand Down Expand Up @@ -204,7 +209,9 @@ void JavaObject::MakeJavaWeak()
javaObject_ = weakRef;
} else {
ASSERT(refTableKey_ != 0);
ReferenceTable::makeWeakReference(refTableKey_);

// create a soft reference, which is more resiliant than a weak reference
ReferenceTable::makeSoftReference(refTableKey_);
}

UPDATE_STATS(0, 1); // add one to "detached" counter
Expand Down
13 changes: 11 additions & 2 deletions android/runtime/v8/src/native/ReferenceTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,21 @@ void ReferenceTable::makeWeakReference(jlong key)
key);
}

jobject ReferenceTable::clearWeakReference(jlong key)
void ReferenceTable::makeSoftReference(jlong key)
{
JNIEnv* env = JNIUtil::getJNIEnv();
env->CallStaticVoidMethod(
JNIUtil::referenceTableClass,
JNIUtil::referenceTableMakeSoftReferenceMethod,
key);
}

jobject ReferenceTable::clearReference(jlong key)
{
JNIEnv* env = JNIUtil::getJNIEnv();
return env->CallStaticObjectMethod(
JNIUtil::referenceTableClass,
JNIUtil::referenceTableClearWeakReferenceMethod,
JNIUtil::referenceTableClearReferenceMethod,
key);
}

Expand Down
3 changes: 2 additions & 1 deletion android/runtime/v8/src/native/ReferenceTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ class ReferenceTable
static jlong createReference(jobject object);
static void destroyReference(jlong key);
static void makeWeakReference(jlong key);
static jobject clearWeakReference(jlong key);
static void makeSoftReference(jlong key);
static jobject clearReference(jlong key);
static jobject getReference(jlong key);
};

Expand Down