JNI: Fixed local-refs overflow in Document.insertRevisionWithHistory

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.

Unfortunately the new local frame may not have enough capacity (on
Android it seems to max out at 512), so to work around this, also copy
strings to the heap and immediately release their JNI refs. This is
only done if the number of strings goes beyond a soft limit of 200.

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

Java/jni/native_document.cc

@@ -272,22 +272,24 @@ JNIEXPORT jint JNICALL Java_com_couchbase_cbforest_Document_insertRevisionWithHi
     {
         // Convert jhistory, a Java String[], to a C array of C4Slice:
         jsize n = env->GetArrayLength(jhistory);
+        if (!env->PushLocalFrame(n+1))              // Ask for more local refs
+            return -1;
         std::vector<C4Slice> history(n);
         std::vector<jstringSlice*> historyAlloc;
         for (jsize i = 0; i < n; i++) {
             jstring js = (jstring)env->GetObjectArrayElement(jhistory, i);
             jstringSlice *item = new jstringSlice(env, js);
+            if (i >= MaxLocalRefsToUse)
+                item->copyAndReleaseRef();
             historyAlloc.push_back(item); // so its memory won't be freed
             history[i] = *item;
         }
 
-        // Make sure the body will be released before releasing keeper.
-        // Android ARM device caused memory access error when release jstringSlices (historyAlloc).
-        // It seems the error is caused by `GetPrimitiveArrayCritical` and order of releasing memories
-        // which are allocated through JNI methods.
-        // https://github.com/couchbase/couchbase-lite-java-core/issues/793
         {
-            jbyteArraySlice body(env, jbody, true); // critical
+            // `body` is a "critical" JNI ref. This is the fastest way to access its bytes, but
+            // it's illegal to make any more JNI calls until the critical ref is released.
+            // We declare it in a nested block, so it'll be released immediately. (java-core#793)
+            jbyteArraySlice body(env, jbody, true);
             inserted = c4doc_insertRevisionWithHistory(doc, body, deleted, hasAtt,
                                                        history.data(), n,
                                                        &error);
@@ -296,8 +298,7 @@ JNIEXPORT jint JNICALL Java_com_couchbase_cbforest_Document_insertRevisionWithHi
         // release memory
         for (jsize i = 0; i < n; i++)
             delete historyAlloc.at(i);
-        historyAlloc.clear();
-
+        env->PopLocalFrame(NULL);
     }
     if (inserted >= 0) {
         updateRevIDAndFlags(env, self, doc);

Java/jni/native_glue.cc

@@ -46,21 +46,35 @@ namespace cbforest {
         JavaVM *gJVM;
 
         jstringSlice::jstringSlice(JNIEnv *env, jstring js)
-        :_env(env),
-         _jstr(js)
+        :_env(NULL)
         {
-            if(js == NULL){
-                _cstr = NULL;
-            }else{
+            assert(env != NULL);
+            if (js != NULL) {
                 jboolean isCopy;
-                _cstr = env->GetStringUTFChars(js, &isCopy);
+                const char *cstr = env->GetStringUTFChars(js, &isCopy);
+                if (!cstr)
+                    return; // Would it be better to throw an exception?
+                _slice = slice(cstr);
+                _jstr = js;
+                _env = env;
             }
-            _slice = slice(_cstr);
         }
 
         jstringSlice::~jstringSlice() {
-            if (_cstr)
-                _env->ReleaseStringUTFChars(_jstr, _cstr);
+            if (_env)
+                _env->ReleaseStringUTFChars(_jstr, (const char*)_slice.buf);
+            else if (_slice.buf)
+                free((void*)_slice.buf);        // detached
+        }
+
+        void jstringSlice::copyAndReleaseRef() {
+            if (_env) {
+                auto cstr = (const char*)_slice.buf;
+                _slice = _slice.copy();
+                _env->ReleaseStringUTFChars(_jstr, cstr);
+                _env->DeleteLocalRef(_jstr);
+                _env = NULL;
+            }
         }
 
 

Java/jni/native_glue.hh

@@ -17,6 +17,11 @@
 namespace cbforest {
     namespace jni {
 
+// Soft limit of number of local JNI refs to use. Even using PushLocalFrame(), you may not get as
+// many refs as you asked for. At least, that's what happens on Android: the new frame won't have
+// more than 512 refs available. So 200 is being conservative.
+static const size_t MaxLocalRefsToUse = 200;
+
 extern JavaVM *gJVM;
 
 bool initDatabase(JNIEnv*);     // Implemented in native_database.cc
@@ -31,23 +36,27 @@ public:
     ~jstringSlice();
 
     jstringSlice(jstringSlice&& s) // move constructor
-    :_slice(s._slice), _env(s._env), _jstr(s._jstr), _cstr(s._cstr)
-    { s._slice = slice::null; }
+    :_slice(s._slice), _env(s._env), _jstr(s._jstr)
+    { s._env = NULL; s._slice.buf = NULL; }
 
     operator slice()    {return _slice;}
     operator C4Slice()  {return {_slice.buf, _slice.size};}
 
+    // Copies the string data and releases the JNI local ref.
+    void copyAndReleaseRef();
+
 private:
     slice _slice;
     JNIEnv *_env;
     jstring _jstr;
-    const char *_cstr;
 };
 
 
 // Creates a temporary slice value from a Java byte[], attempting to avoid copying
 class jbyteArraySlice {
 public:
+    // Warning: If `critical` is true, you cannot make any further JNI calls (except other
+    // critical accesses) until this object goes out of scope or is deleted.
     jbyteArraySlice(JNIEnv *env, jbyteArray jbytes, bool critical =false);
     ~jbyteArraySlice();