ABP/Android libadblockplus integration

jni/AndroidWebRequest.cpp

+/*
+ * This file is part of Adblock Plus <http://adblockplus.org/>,
+ * Copyright (C) 2006-2013 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 <AdblockPlus.h>
+#include "AndroidWebRequest.h"
+#include "Utils.h"
+#include "debug.h"
+
+struct JavaException : public std::exception
+{
+  jthrowable ex;
+
+  JavaException(jthrowable e) : ex(e)
+  {
+  }
+
+  const char* what() const throw()
+  {
+    return "Java Exception";

[Wladimir Palant, 2013/09/12 11:31:14]

I think that this should return the actual exception message rather than
requiring the message to be extracted when the exception is caught. This means
that the constructor parameter should be an environment pointer - the
constructor can then get the current extension and store the corresponding
message.

I think that we also need to capture the exception class in order to process the
exception better.

+  }
+};
+
+AndroidWebRequest::AndroidWebRequest(JavaVM*& gJvm) : globalJvm(gJvm)

[Felix Dahlke, 2013/09/13 14:40:39]

You can actually call the parameter globalJvm as well here, won't be an issue
with the initialiser list. Or better just vm, it's not a global.

+{
+  JNIEnv* jniEnv = NULL;
+  int stat = globalJvm->GetEnv((void **)&jniEnv, JNI_VERSION_1_6);
+  if (stat == JNI_EDETACHED)
+  {
+    if (globalJvm->AttachCurrentThread(&jniEnv, NULL) != 0)
+      throw std::runtime_error("Failed to get JNI environment");
+  }

[Wladimir Palant, 2013/09/12 11:31:14]

Seeing that this block of code is a recurring pattern, it would be better to
move it into a helper function. In fact, ideally we would have a stack-allocated
class that could be used like this:

JNIEnvHolder jniEnv(globalJvm);
jniEnv->newGlobalRef(...);

The constructor of that class would execute the code above, the destructor would
call DetachCurrentThread(). You could also overload the JNIEnv* cast and the ->
operator to ensure that this class can be used exactly like a regular
environment pointer.

+
+  jUrlClass = reinterpret_cast<jclass>(jniEnv->NewGlobalRef(jniEnv->FindClass("java/net/URL")));
+  jUrlConstructorID = jniEnv->GetMethodID(jUrlClass, "<init>", "(Ljava/lang/String;)V");
+  jUrlOpenConnectionID = jniEnv->GetMethodID(jUrlClass, "openConnection", "()Ljava/net/URLConnection;");
+  jConnectionClass = reinterpret_cast<jclass>(jniEnv->NewGlobalRef(jniEnv->FindClass("java/net/HttpURLConnection")));
+  jConnectionSetMethodID = jniEnv->GetMethodID(jConnectionClass, "setRequestMethod", "(Ljava/lang/String;)V");
+  jConnectionSetRequestPropertyID = jniEnv->GetMethodID(jConnectionClass, "setRequestProperty", "(Ljava/lang/String;Ljava/lang/String;)V");
+  jConnectionConnectID = jniEnv->GetMethodID(jConnectionClass, "connect", "()V");
+  jConnectionGetContentLengthID = jniEnv->GetMethodID(jConnectionClass, "getContentLength", "()I");
+  jConnectionGetResponseCodeID = jniEnv->GetMethodID(jConnectionClass, "getResponseCode", "()I");
+  jConnectionGetContentEncodingID = jniEnv->GetMethodID(jConnectionClass, "getContentEncoding", "()Ljava/lang/String;");
+  jConnectionGetInputStreamID = jniEnv->GetMethodID(jConnectionClass, "getInputStream", "()Ljava/io/InputStream;");
+  jInputStreamReaderClass = reinterpret_cast<jclass>(jniEnv->NewGlobalRef(jniEnv->FindClass("java/io/InputStreamReader")));
+  jInputStreamReaderConstructorID = jniEnv->GetMethodID(jInputStreamReaderClass, "<init>", "(Ljava/io/InputStream;Ljava/lang/String;)V");
+  jBufferedReaderClass = reinterpret_cast<jclass>(jniEnv->NewGlobalRef(jniEnv->FindClass("java/io/BufferedReader")));
+  jBufferedReaderConstructorID = jniEnv->GetMethodID(jBufferedReaderClass, "<init>", "(Ljava/io/Reader;)V");
+  jStringBuilderClass = reinterpret_cast<jclass>(jniEnv->NewGlobalRef(jniEnv->FindClass("java/lang/StringBuilder")));
+  jStringBuilderConstructorID = jniEnv->GetMethodID(jStringBuilderClass, "<init>", "()V");
+  jBufferedReaderReadID = jniEnv->GetMethodID(jBufferedReaderClass, "read", "([CII)I");
+  jStringBuilderAppendID = jniEnv->GetMethodID(jStringBuilderClass, "append", "([CII)Ljava/lang/StringBuilder;");
+  jStringBuilderToStringID = jniEnv->GetMethodID(jStringBuilderClass, "toString", "()Ljava/lang/String;");
+  jBufferedReaderCloseID = jniEnv->GetMethodID(jBufferedReaderClass, "close", "()V");
+  jConnectionGetHeaderFieldKeyID = jniEnv->GetMethodID(jConnectionClass, "getHeaderField", "(I)Ljava/lang/String;");
+  jConnectionGetHeaderFieldID = jniEnv->GetMethodID(jConnectionClass, "getHeaderFieldKey", "(I)Ljava/lang/String;");
+
+  if (stat == JNI_EDETACHED)
+    globalJvm->DetachCurrentThread();
+}
+
+AndroidWebRequest::~AndroidWebRequest()
+{
+  JNIEnv* jniEnv = NULL;
+  int stat = globalJvm->GetEnv((void **)&jniEnv, JNI_VERSION_1_6);
+  if (stat == JNI_EDETACHED)
+  {
+    if (globalJvm->AttachCurrentThread(&jniEnv, NULL) != 0)
+      throw std::runtime_error("Failed to get JNI environment");
+  }
+
+  jniEnv->DeleteGlobalRef(jUrlClass);
+  jniEnv->DeleteGlobalRef(jConnectionClass);
+  jniEnv->DeleteGlobalRef(jInputStreamReaderClass);
+  jniEnv->DeleteGlobalRef(jBufferedReaderClass);
+  jniEnv->DeleteGlobalRef(jStringBuilderClass);
+
+  if (stat == JNI_EDETACHED)
+    globalJvm->DetachCurrentThread();
+}
+
+AdblockPlus::ServerResponse AndroidWebRequest::GET(
+  const std::string& url, const AdblockPlus::HeaderList& requestHeaders) const
+{
+  JNIEnv* jniEnv = NULL;
+  int stat = globalJvm->GetEnv((void **)&jniEnv, JNI_VERSION_1_6);
+  if (stat == JNI_EDETACHED)
+  {
+    if (globalJvm->AttachCurrentThread(&jniEnv, NULL) != 0)
+      throw std::runtime_error("Failed to get JNI environment");
+  }
+
+  AdblockPlus::ServerResponse result;
+  result.status = NS_ERROR_FAILURE;
+  result.responseStatus = 0;
+
+  try
+  {
+    // URL jUrl = new URL(url)

[Felix Dahlke, 2013/09/13 14:40:39]

Using JNI for all this is a bit cumbersome, I think I'd have preferred this code
to just call a Java function. Then again, I don't think it's really worth
rewriting, just a remark.

+    jstring jUrlStr = jniEnv->NewStringUTF(url.c_str());
+
+    jobject jUrl = jniEnv->NewObject(jUrlClass, jUrlConstructorID, jUrlStr);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());

[Wladimir Palant, 2013/09/12 11:31:14]

Seeing this block of code repeated all over the place, it is probably better to
move it into a helper function like CheckExceptions().

[Felix Dahlke, 2013/09/13 14:40:39]

I'd vote for HandleExceptions();

I also think we need to detach the current thread before we throw. The
destructor will only detach it if it wasn't previously attached. Won't be
necessary if we get Wladimir's JNIEnvHolder though.

+    jniEnv->DeleteLocalRef(jUrlStr);

[Felix Dahlke, 2013/09/13 14:40:39]

DeleteLocalRef shouldn't be necessary here. It'll be garbage collected once we
detach the thread. Same goes for the other DeleteLocalRef calls below.

+
+    // HttpURLConnection connection = (HttpURLConnection) jUrl.openConnection();
+    jobject jConnection = jniEnv->CallObjectMethod(jUrl, jUrlOpenConnectionID);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    // connection.setRequestMethod("GET");
+    jstring jMethod = jniEnv->NewStringUTF("GET");
+    jniEnv->CallVoidMethod(jConnection, jConnectionSetMethodID, jMethod);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+    jniEnv->DeleteLocalRef(jMethod);
+
+    for (int i = 0; i < requestHeaders.size(); i++)
+    {
+      // connection.setRequestProperty(requestHeaders[i].first, requestHeaders[i].second);
+      jstring jHeader = jniEnv->NewStringUTF(requestHeaders[i].first.c_str());
+      jstring jValue = jniEnv->NewStringUTF(requestHeaders[i].second.c_str());
+      jniEnv->CallVoidMethod(jConnection, jConnectionSetRequestPropertyID, jHeader, jValue);
+      if (jniEnv->ExceptionCheck())
+        throw JavaException(jniEnv->ExceptionOccurred());
+      jniEnv->DeleteLocalRef(jHeader);
+      jniEnv->DeleteLocalRef(jValue);
+    }
+
+    // connection.connect();
+    jniEnv->CallVoidMethod(jConnection, jConnectionConnectID);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    // int lenghtOfFile = connection.getContentLength();
+    jint lenghtOfFile = jniEnv->CallIntMethod(jConnection, jConnectionGetContentLengthID);

[Wladimir Palant, 2013/09/12 11:31:14]

This variable's name is misspelled, and it isn't actually used anyway - remove?

+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    D(D_WARN, "Size: %d", lenghtOfFile);
+
+    // result.responseStatus = connection.getResponseCode();
+    result.responseStatus = jniEnv->CallIntMethod(jConnection, jConnectionGetResponseCodeID);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    /* Read response data */
+
+    // String jEncoding = connection.getContentEncoding();
+    jstring jEncoding = (jstring) jniEnv->CallObjectMethod(jConnection, jConnectionGetContentEncodingID);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    if (jEncoding == NULL)
+      jEncoding = jniEnv->NewStringUTF("utf-8");
+
+    // InputStream jis = connection.getInputStream();
+    jobject jis = jniEnv->CallObjectMethod(jConnection, jConnectionGetInputStreamID);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    // InputStreamReader jisr = new InputStreamReader(jis, jEncoding);
+    jobject jisr = jniEnv->NewObject(jInputStreamReaderClass, jInputStreamReaderConstructorID, jis, jEncoding);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    jniEnv->DeleteLocalRef(jEncoding);
+
+    // BufferedReader jin = new BufferedReader(jisr);
+    jobject jin = jniEnv->NewObject(jBufferedReaderClass, jBufferedReaderConstructorID, jisr);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    // char[] jBuffer = new char[0x10000];
+    jcharArray jBuffer = jniEnv->NewCharArray(0x10000);
+
+    // StringBuilder jout = new StringBuilder();
+    jobject jout = jniEnv->NewObject(jStringBuilderClass, jStringBuilderConstructorID);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    jlong total = 0;
+    jint read;
+
+    jint jBufferLength = (jint) jniEnv->GetArrayLength(jBuffer);
+
+    do
+    {
+      // read = jin.read(buffer, 0, buffer.length);
+      read = jniEnv->CallIntMethod(jin, jBufferedReaderReadID, jBuffer, 0, jBufferLength);
+      if (jniEnv->ExceptionCheck())
+        throw JavaException(jniEnv->ExceptionOccurred());
+
+      if (read > 0)
+      {
+        // jout.append(buffer, 0, read);
+        jniEnv->CallObjectMethod(jout, jStringBuilderAppendID, jBuffer, 0, jBufferLength);
+        if (jniEnv->ExceptionCheck())
+          throw JavaException(jniEnv->ExceptionOccurred());
+
+        total += read;
+      }
+    }
+    while (read >= 0);
+
+    // String jData = out.toString();
+    jstring jData = (jstring) jniEnv->CallObjectMethod(jout, jStringBuilderToStringID);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    result.responseText = GetString(jniEnv, jData);
+
+    // jin.close();
+    jniEnv->CallVoidMethod(jin, jBufferedReaderCloseID);
+    if (jniEnv->ExceptionCheck())
+      throw JavaException(jniEnv->ExceptionOccurred());
+
+    jint i = 0;
+    while (true)

[Felix Dahlke, 2013/09/13 14:40:39]

How about |for (jint i = 0; ; i++)| instead?

+    {
+      // String jHeaderName = connection.getHeaderFieldKey(i)
+      jstring jHeaderName = (jstring) jniEnv->CallObjectMethod(jConnection, jConnectionGetHeaderFieldKeyID, i);
+      if (jniEnv->ExceptionCheck())
+        throw JavaException(jniEnv->ExceptionOccurred());
+
+      // String jHeaderValue = connection.getHeaderField(i)
+      jstring jHeaderValue = (jstring) jniEnv->CallObjectMethod(jConnection, jConnectionGetHeaderFieldID, i);
+      if (jniEnv->ExceptionCheck())
+        throw JavaException(jniEnv->ExceptionOccurred());
+
+      if (jHeaderValue == NULL)

[Wladimir Palant, 2013/09/12 11:31:14]

From all I know, NULL is a Windows-specific constant. How about just using |if
(!jHeaderValue)| here? Also, I would actually check |if (!jHeaderKey)| above to
break out slightly earlier.

[Felix Dahlke, 2013/09/13 14:40:39]

NULL is not Windows-specific, it's in ANSI C. But agreed, no need to compare
with it here. And I personally prefer 0 to avoid macros.

+        break;
+
+      std::string headerName = GetString(jniEnv, jHeaderName);
+      std::string headerValue = GetString(jniEnv, jHeaderValue);
+
+      headerName = TrimString(headerName);
+      headerValue = TrimString(headerValue);
+
+      std::transform(headerName.begin(), headerName.end(), headerName.begin(), ::tolower);
+      std::transform(headerValue.begin(), headerValue.end(), headerValue.begin(), ::tolower);

[Wladimir Palant, 2013/09/12 11:31:14]

Please do not lower-case the value, only the header name is case-insensitive.

+
+      result.responseHeaders.push_back(std::pair<std::string, std::string>(headerName, headerValue));
+
+      i++;
+    }
+    D(D_WARN, "Finished downloading");
+
+    result.status = AdblockPlus::DefaultWebRequest::NS_OK;
+  }
+  catch(JavaException& e)
+  {
+    jniEnv->ExceptionClear();
+    jclass jThrowableClass = jniEnv->FindClass("java/lang/Throwable");
+    jmethodID jThrowableToStringID = jniEnv->GetMethodID(jThrowableClass, "toString", "()Ljava/lang/String;");
+    jstring jMsg = (jstring) jniEnv->CallObjectMethod(e.ex, jThrowableToStringID);
+    const char* msg = jniEnv->GetStringUTFChars(jMsg, 0);
+    D(D_ERROR, "Java Exception: %s", msg);
+    jniEnv->ReleaseStringUTFChars(jMsg, msg);
+    jniEnv->DeleteLocalRef(jMsg);
+    result.status = AdblockPlus::DefaultWebRequest::NS_ERROR_FAILURE;

[Wladimir Palant, 2013/09/12 11:31:14]

I think that we should try to do better than that. Some exception classes can be
mapped to a particular error code:

* ConnectException, NoRouteToHostException, PortUnreachableException =>
NS_ERROR_CONNECTION_REFUSED
* MalformedURLException, URISyntaxException => NS_ERROR_MALFORMED_URI
* SocketTimeoutException => NS_ERROR_NET_TIMEOUT
* UnknownHostException => NS_ERROR_UNKNOWN_HOST

Not sure which Java exceptions correspond to NS_ERROR_UNKNOWN_PROTOCOL,
NS_ERROR_NO_CONTENT or NS_ERROR_NET_INTERRUPT, this can be tested. You could
also add some constants to the WebRequest class to cover the various SSL
exceptions (complicated by the fact that Gecko's SSL exceptions are a lot more
detailed than Java's):

* SSLHandshakeException => SSL_ERROR_BAD_SERVER (0x805A2FF9)
* SSLKeyException => SSL_ERROR_WRONG_CERTIFICATE (0x805A2FF5)
* SSLPeerUnverifiedException => SSL_ERROR_BAD_CLIENT (0x805A2FFA)
* SSLProtocolException => SEC_ERROR_BAD_SIGNATURE (0x0x805A1FF6)

+  }
+  catch (const std::exception& e)
+  {
+    D(D_ERROR, "Exception: %s", e.what());
+    result.status = AdblockPlus::DefaultWebRequest::NS_ERROR_FAILURE;
+  }
+  catch (...)
+  {
+    D(D_ERROR, "Unknown exception");
+    result.status = AdblockPlus::DefaultWebRequest::NS_ERROR_FAILURE;
+  }
+
+  if (stat == JNI_EDETACHED)
+    globalJvm->DetachCurrentThread();
+
+  return result;
+}