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

JNI: Fixed local-refs overflow in Document.insertRevisionWithHistory #64

Merged
merged 2 commits into from
Feb 17, 2016

Conversation

snej
Copy link
Contributor

@snej snej commented Feb 17, 2016

This function can use an arbitrary number of local JNI refs (one for
each item in the history array), so preallocate a new local frame with
enough room for them.

(Also clarified a comment about critical allocation just below, to make
it more clear that JNI allocations are illegal until the critical ref
is released.)

Fixes couchbase/couchbase-lite-android#782 (I think; needs testing)

@snej
Copy link
Contributor Author

snej commented Feb 17, 2016

This builds, but I don't have anything set up to run/test the JNI interface. Could you try this out, @hideki ?

@hideki
Copy link

hideki commented Feb 17, 2016

Unfortunately PushLocalFrame(n+1) solution still causes JNI ERROR (app bug): local reference table overflow (max=512)

Note: I cherry-picked the commit to 1.2.0-java branch to test.

@snej snej mentioned this pull request Feb 17, 2016
@snej
Copy link
Contributor Author

snej commented Feb 17, 2016

Crap. Well, it looks like, at least on Android, PushLocalFrame doesn't actually guarantee you the capacity you asked for. Look at the function PushLocalFrame here:

    if (!ensureLocalCapacity(ts.self(), capacity) ||
            !dvmPushLocalFrame(ts.self(), dvmGetCurrentJNIMethod()))

So if there isn't enough capacity in the current frame it pushes a new one ... but with whatever the default capacity is, not the capacity you asked for.

@hideki
Copy link

hideki commented Feb 17, 2016

Thank you for link to the code.

It seems 512 is maximum local reference table size.

#define kJniLocalRefMax         512     /* arbitrary; should be plenty */

https://android.googlesource.com/platform/dalvik/+/kitkat-release/vm/Thread.h#74

@snej
Copy link
Contributor Author

snej commented Feb 17, 2016

OK, here's another try. I've added a new jstringSlice method copyAndReleaseRef() that copies the string data to the heap, then releases the JNI local ref.

@hideki
Copy link

hideki commented Feb 17, 2016

New solution throws RuntimeException.

java.lang.RuntimeException: too many PopLocalFrame calls
at com.couchbase.cbforest.Document.insertRevisionWithHistory(Native Method)
at com.couchbase.cbforest.Document.insertRevisionWithHistory(Document.java:73)
at com.couchbase.cbforest.C4DatabaseTest._testInsertRevisionWithHistory(C4DatabaseTest.java:264)
at com.couchbase.cbforest.C4DatabaseTest.testInsertRevisionWith512History(C4DatabaseTest.java:234)

This function can use an arbitrary number of local JNI refs (one for
each item in the history array), so request capacity to handle them.
Unfortunately the maximum local-ref capacity is limited, and the actual
limit is unknown (implementation-specific but not available via API),
so we'll restrict ourselves to 200 (MaxLocalRefsToUse.) Android's limit
is 512.

If we have too many strings, copy the excess ones to the heap and
immediately release their JNI refs during the iteration.

Fixes couchbase/couchbase-lite-android#782
hideki pushed a commit that referenced this pull request Feb 17, 2016
JNI: Fixed local-refs overflow in Document.insertRevisionWithHistory
@hideki hideki merged commit 74736b6 into master Feb 17, 2016
@hideki
Copy link

hideki commented Feb 17, 2016

All Unit tests can pass. Thank you for fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants