Issue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback

libadblockplus-android/jni/JniFileSystem.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include "JniCallbacks.h"
 #include "AdblockPlus/FileSystem.h"
 #include "Utils.h"
-#include <sstream>
+#include <istream>
+#include <streambuf>
+
+class RuntimeErrorWithErrno : public std::runtime_error
+{
+public:
+  explicit RuntimeErrorWithErrno(const std::string& message)
+    : std::runtime_error(message + " (" + strerror(errno) + ")")
+  {
+  }
+};
+
+struct membuf: std::streambuf {
+    membuf(char const* base, size_t size) {
+        char* p(const_cast<char*>(base));
+        this->setg(p, p, p + size);
+    }
+};
+
+struct imemstream: virtual membuf, std::istream {
+    imemstream(char const* base, size_t size)
+        : membuf(base, size)
+        , std::istream(static_cast<std::streambuf*>(this)) {
+    }
+};

[sergei, 2017/08/07 12:50:00]

All these things should be in the anonymous namespace, however fortunately all
these C++ streams will go away after rabasing.

 
 // precached in JNI_OnLoad and released in JNI_OnUnload
 JniGlobalReference<jclass>* statResultClass;
 jmethodID existsMethod;
 jmethodID isDirectoryMethod;
 jmethodID isFileMethod;
 jmethodID getLastModifiedMethod;
 
 void JniFileSystem_OnLoad(JavaVM* vm, JNIEnv* env, void* reserved)
 {
   statResultClass = new JniGlobalReference<jclass>(env, env->FindClass(PKG("FileSystem$StatResult")));
   existsMethod = env->GetMethodID(statResultClass->Get(), "exists", "()Z");
   isDirectoryMethod = env->GetMethodID(statResultClass->Get(), "isDirectory", "()Z");
   isFileMethod = env->GetMethodID(statResultClass->Get(), "isFile", "()Z");
   getLastModifiedMethod = env->GetMethodID(statResultClass->Get(), "getLastModified", "()J");
 }
 
 void JniFileSystem_OnUnload(JavaVM* vm, JNIEnv* env, void* reserved)
 {
   if (statResultClass)
   {
     delete statResultClass;
     statResultClass = NULL;

[sergei, 2017/08/07 12:50:00]

nullptr?

   }
 }
 
 static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz, jobject callbackObject)

[sergei, 2017/08/07 12:49:59]

I think that it should be done a little bit differently. JniFileSystemCallback
inherits JniCallbackBase which holds a strong reference to a corresponding Java
class, the lifetime of JniFileSystemCallback is managed by JsEngine and there
are no calls from Java class of methods of JniFileSystemCallback. So, there is
no need to have a pointer to C++ class in Java, instead there should be an
interface in Java which is passed into some Java JsEngine.SetFileSystem (BTW, it
will reduce the number of further changes if it's done in the constructor of
JsEngine) and finally as jobject into some JniJsEngine.cpp::JniSetFileSystem (or
in JniCtor). In JniSetFileSystem we simply create JniFileSystemCallback
accepting jobject and holding it. The implementation of methods of
AdblockPlus::FileSystem should simply call corresponding methods of the stored
Java interface.
In addition the most recent changes don't use a std::shared_ptr<FileSystem>, the
ownership is passed to a corresponding class via std::unique_ptr what basically
implies and forces the described above approach.

 {
   try
   {
-    return JniPtrToLong(new JniFileSystemCallback(env, callbackObject));
+    return JniPtrToLong(new AdblockPlus::FileSystemPtr(new JniFileSystemCallback(env, callbackObject)));
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 static void JNICALL JniDtor(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  delete JniLongToTypePtr<JniFileSystemCallback>(ptr);
+  delete JniLongToTypePtr<AdblockPlus::FileSystemPtr>(ptr);
 }
 
 JniFileSystemCallback::JniFileSystemCallback(JNIEnv* env, jobject callbackObject)
-  : JniCallbackBase(env, callbackObject),
-  AdblockPlus::FileSystem()

[sergei, 2017/05/22 12:09:07]

The call of AdblockPlus::FileSystem() is not required.

[anton, 2017/05/22 13:04:37]

Acknowledged.

+  : JniCallbackBase(env, callbackObject)
 {
 }
 
 std::shared_ptr<std::istream> JniFileSystemCallback::Read(const std::string& path) const
 {
   JNIEnvAcquire env(GetJavaVM());
 
   jmethodID method = env->GetMethodID(
     *JniLocalReference<jclass>(*env, env->GetObjectClass(GetCallbackObject())),
     "read",
-    "(Ljava/lang/String;)Ljava/lang/String;");
-
-  JniLocalReference<jstring> jPath(*env, env->NewStringUTF(path.c_str()));
-  jstring jData = (jstring)env->CallObjectMethod(GetCallbackObject(), method, *jPath);
-  CheckAndLogJavaException(*env);
-
-  if (!jData)
-    return NULL;
-
-  std::string cData = JniJavaToStdString(*env, jData);
-  std::shared_ptr<std::istream> cSharedStream(new std::istringstream(cData));
+    "(Ljava/lang/String;)[B");
+
+  JniLocalReference<jstring> jPath(*env, env->NewStringUTF(path.c_str()));
+  jbyteArray jData = (jbyteArray)env->CallObjectMethod(GetCallbackObject(), method, *jPath);
+  if (CheckAndLogJavaException(*env))
+    throw new RuntimeErrorWithErrno("Failed to open file (File not found)");
+
+  int dataLength = env->GetArrayLength(jData);
+  char* cData = new char[dataLength];
+  env->GetByteArrayRegion(jData, 0, dataLength, reinterpret_cast<jbyte*>(cData));
+
+  std::shared_ptr<std::istream> cSharedStream(new imemstream(cData, dataLength));
   return cSharedStream;
 }
 
-void JniFileSystemCallback::Write(const std::string& path, std::shared_ptr<std::istream> data)
+void JniFileSystemCallback::Write(const std::string& path, std::shared_ptr<std::istream> dataStreamPtr)
 {
   JNIEnvAcquire env(GetJavaVM());
 
   jmethodID method = env->GetMethodID(
     *JniLocalReference<jclass>(*env, env->GetObjectClass(GetCallbackObject())),
     "write",
-    "(Ljava/lang/String;Ljava/lang/String;)V");
+    "(Ljava/lang/String;[B)V");
 
   JniLocalReference<jstring> jPath(*env, env->NewStringUTF(path.c_str()));
 
   // read all the data from the stream into buffer (no appropriate way to pass streams over JNI)
-  std::string cData = AdblockPlus::Utils::Slurp(*data.get());
-  JniLocalReference<jstring> jData(*env, env->NewStringUTF(cData.c_str()));
-
-  env->CallVoidMethod(GetCallbackObject(), method, *jPath, *jData);
-  CheckAndLogJavaException(*env);
+  std::istream* dataStream = dataStreamPtr.get();
+  dataStream->seekg(0, std::ios::end);
+  int dataLength = dataStream->tellg();
+  char* cData = new char[dataLength];
+  dataStream->seekg(0, std::ios::beg);
+  dataStream->read(cData, dataLength);
+
+  jbyteArray jData = env->NewByteArray(dataLength);
+  env->SetByteArrayRegion(jData, 0, dataLength, reinterpret_cast<jbyte*>(cData));
+
+  env->CallVoidMethod(GetCallbackObject(), method, *jPath, jData);
+  CheckAndLogJavaException(*env);
+  delete[] cData;
 }
 
 void JniFileSystemCallback::Move(const std::string& fromPath, const std::string& toPath)
 {
   JNIEnvAcquire env(GetJavaVM());
 
   jmethodID method = env->GetMethodID(
     *JniLocalReference<jclass>(*env, env->GetObjectClass(GetCallbackObject())),
     "move",
     "(Ljava/lang/String;Ljava/lang/String;)V");
@@ skipping 71 matching lines @@
 static JNINativeMethod methods[] =
 {
   { (char*)"ctor", (char*)"(Ljava/lang/Object;)J", (void*)JniCtor },
   { (char*)"dtor", (char*)"(J)V", (void*)JniDtor }
 };
 
 extern "C" JNIEXPORT void JNICALL Java_org_adblockplus_libadblockplus_FileSystem_registerNatives(JNIEnv *env, jclass clazz)
 {
   env->RegisterNatives(clazz, methods, sizeof(methods) / sizeof(methods[0]));
 }