java bindings jni memory leak(s)

[Alexander Kushkuley]

1) java/native/Query.cc

A Relase... is needed for the Get... ( arrow in the code below )

JNIEXPORT jlong JNICALL Java_org_xapian_XapianJNI_query_1newI_3J (JNIEnv *env, jclass clazz, jint op, jlongArray qids) {

TRY

jsize len = env->GetArrayLength(qids); Query queries = new Query*[len]; jlong *qid_ptr = env->GetLongArrayElements(qids, NULL); for (int x=0; x<len; x++) {

queries[x] = _query->get(qid_ptr[x]);

} Query *q = new Query(op_table[op-1], queries, queries+len);

---

delete[] queries; return _query->put(q);

CATCH(-1)

}

2) java/native/Enquire.cc

This is more complicated. It seems that the code below is necessary for some reason. However, as it is it creates a memory leak -- when enquiry object (e) is deleted in finilize, it's MyQuery field is not cleaned up. Same is also true for the next jni function in Enquire.cc.

See an additional question ( comment ) in the code below.

JNIEXPORT void JNICALL Java_org_xapian_XapianJNI_enquire_1set_1query (JNIEnv *env, jclass clazz, jlong eid, jlong qid) {

TRY

Enquire *e = _enquire->get(eid); Query *tmp = _query->get(qid);

MyQuery *q = new MyQuery(*tmp); q->setMyID(qid); e->set_query(*q);

why not this instead of three lines above ?

e->set_query(*tmp);

CATCH(;)

}

Both problems can be reproduced by calling corresponding api calls in a tight loop.

[Olly Betts]

The first change is in some code I added - I think I probably assumed we always got a pinned copy, but looks like you're right. We don't change the array, so I committed a fix to call ReleaseLongArrayElements with JNI_ABORT to avoid wasting time copying back the unchanged array if we do get a copy. If that seems wrong, let me know.

I don't see why the code makes a temporary copy in the second case. That code is essentially unchanged from Eric's original version, so I guess he didn't realise that the C++ objects are reference counted so you don't need to make copies like this.

Does it work if you make the change you ask about?

[Olly Betts]

Oh, I see what's going on in the second part.

There's a MyQuery subclass, and we're creating an object of that type and passing it in to set_query, then hoping we get the same object back from get_query. But we won't - it's the internals which will be the same. So this code in set_query is pointless, and get_query won't work correctly. We might as well kill the MyQuery mechanism and find a better way to handle get_query (or declare it's not currently supported in Java - it's not a vital method to have).

[Olly Betts]

Looking more closely, Enquire::getQuery() is implemented entirely in java, so the JNI wrappers for the C++ version aren't used and we can just remove them, and the whole MyQuery mechanism.

So this is all now fixed in SVN HEAD.

[Olly Betts]

This bug will be fixed in 0.9.10 (coming soon).