ABP/Android libadblockplus integration

jni/abpEngine.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 <jni.h>
+#include <AdblockPlus.h>
+#include "AndroidLogSystem.h"
+#include "AndroidWebRequest.h"
+#include "Utils.h"
+#include "debug.h"
+
+JavaVM* globalJvm;
+AdblockPlus::FilterEngine* filterEngine;

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

Please use a smart pointer here, raw pointers should be avoided.
std::tr1::unique_ptr<AdblockPlus::FilterEngine> would be the obvious candidate.
|filterEngine = null| will be enough to both release the object and null out the
pointer.

+jobject jniObject;
+
+extern "C"
+{
+  JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_initialize(JNIEnv *pEnv, jobject, jstring basepath);
+  JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_release(JNIEnv *pEnv, jobject);
+  JNIEXPORT jboolean JNICALL Java_org_adblockplus_android_ABPEngine_isFirstRun(JNIEnv *pEnv, jobject);
+  JNIEXPORT jobjectArray JNICALL Java_org_adblockplus_android_ABPEngine_getListedSubscriptions(JNIEnv *pEnv, jobject);
+  JNIEXPORT jobjectArray JNICALL Java_org_adblockplus_android_ABPEngine_getRecommendedSubscriptions(JNIEnv *pEnv, jobject);
+  JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_addSubscription(JNIEnv *pEnv, jobject, jstring url);
+  JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_removeSubscription(JNIEnv *pEnv, jobject, jstring url);
+  JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_refreshSubscription(JNIEnv *pEnv, jobject, jstring url);
+  JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_actualizeSubscriptionStatus(JNIEnv *pEnv, jobject, jstring url);
+  JNIEXPORT jboolean JNICALL Java_org_adblockplus_android_ABPEngine_matches(JNIEnv *pEnv, jobject, jstring url, jstring contentType, jstring documentUrl);
+  JNIEXPORT jobjectArray JNICALL Java_org_adblockplus_android_ABPEngine_getSelectorsForDomain(JNIEnv *pEnv, jobject, jstring domain);
+};
+
+jobjectArray subscriptionsAsJavaArray(JNIEnv *pEnv, std::vector<AdblockPlus::SubscriptionPtr> subscriptions)
+{
+  D(D_WARN, "subscriptionsAsJavaArray()");
+  static jclass cls = reinterpret_cast<jclass>(pEnv->NewGlobalRef(pEnv->FindClass("org/adblockplus/android/Subscription")));
+  static jmethodID cid = pEnv->GetMethodID(cls, "<init>", "()V");
+  static jfieldID ftitle = pEnv->GetFieldID(cls, "title", "Ljava/lang/String;");
+  static jfieldID furl = pEnv->GetFieldID(cls, "url", "Ljava/lang/String;");
+
+  D(D_WARN, "Subscriptions: %d", subscriptions.size());
+  const jobjectArray ret = (jobjectArray) pEnv->NewObjectArray(subscriptions.size(), cls, NULL);

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

The const modifier is somewhat misleading here and should be removed IMHO. While
you don't change the variable value you are modifying the array.

+
+  int i = 0;
+  for (std::vector<AdblockPlus::SubscriptionPtr>::const_iterator it = subscriptions.begin();
+       it != subscriptions.end(); it++)
+  {
+    jobject subscription = pEnv->NewObject(cls, cid);
+    pEnv->SetObjectField(subscription, ftitle, pEnv->NewStringUTF((*it)->GetProperty("title")->AsString().c_str()));
+    pEnv->SetObjectField(subscription, furl, pEnv->NewStringUTF((*it)->GetProperty("url")->AsString().c_str()));
+    pEnv->SetObjectArrayElement(ret, i, subscription);
+    i++;
+  }
+
+  return ret;
+}
+
+void UpdateSubscriptionStatus(const std::string& url)
+{
+  D(D_WARN, "UpdateSubscriptionStatus()");
+
+  AdblockPlus::SubscriptionPtr subscription = filterEngine->GetSubscription(url);
+  std::string downloadStatus = subscription->GetProperty("downloadStatus")->IsNull() ? "" : subscription->GetProperty("downloadStatus")->AsString();
+  int64_t lastDownload = subscription->GetProperty("lastDownload")->AsInt();
+
+  std::string status = "synchronize_never";
+  int64_t time = 0;
+
+  if (subscription->IsUpdating())
+  {
+    status = "synchronize_in_progress";
+  }
+  else if (!downloadStatus.empty() && downloadStatus != "synchronize_ok")
+  {
+    status = downloadStatus;
+  }
+  else if (lastDownload > 0)
+  {
+    time = lastDownload;
+    status = "synchronize_last_at";
+  }
+
+  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");
+  }
+
+  jstring jUrl = jniEnv->NewStringUTF(url.c_str());
+  jstring jStatus = jniEnv->NewStringUTF(status.c_str());
+  jlong jTime = time * 1000;
+
+  static jclass cls = jniEnv->GetObjectClass(jniObject);
+  static jmethodID mid = jniEnv->GetMethodID(cls, "onFilterChanged", "(Ljava/lang/String;Ljava/lang/String;J)V");
+  if (mid)
+    jniEnv->CallVoidMethod(jniObject, mid, jUrl, jStatus, jTime);
+  jniEnv->DeleteLocalRef(jUrl);
+  jniEnv->DeleteLocalRef(jStatus);
+
+  if (stat == JNI_EDETACHED)
+    globalJvm->DetachCurrentThread();
+}
+
+void FilterChangedCallback(const std::string& action, const std::string& url)

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

I think that in the final revision of your libadblockplus change this function
will get JsValuePtr as the second parameter.

+{
+  D(D_WARN, "FilterChangedCallback()");
+
+  if (action == "subscription.lastDownload" || action == "subscription.downloadStatus")
+  {
+          UpdateSubscriptionStatus(url);

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

Nit: Indentation is off here, need to convert a tab to spaces?

+  }
+}
+
+void UpdateAvailableCallback(AdblockPlus::JsValueList& params)
+{
+  //TODO Notify Java UI about available update
+}
+
+jint JNICALL JNI_OnLoad(JavaVM *vm, void *reserved)
+{
+  return JNI_VERSION_1_6;

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

I don't think that we should hardcode the JNI version all over the place. If the
JNIEnvHolder class I mentioned above is implemented then it would make sense to
have a static jniVersion constant there which would be used everywhere instead
of JNI_VERSION_1_6.

+}
+
+void JNICALL JNI_OnUnload(JavaVM *vm, void *reserved)
+{
+}
+
+JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_initialize(JNIEnv *pEnv, jobject pObject, jstring basepath)
+{
+  D(D_WARN, "nativeInitialize()");
+  int status = pEnv->GetJavaVM(&globalJvm);
+
+  jniObject = pEnv->NewGlobalRef(pObject);
+
+  const std::string path = GetString(pEnv, basepath);
+
+  AdblockPlus::AppInfo appInfo;
+  // FIXME Should be taken from Manifest

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

Please use "TODO" instead of "FIXME", this is what we are normally searching
for.

+  appInfo.version = "1.1.2";
+  appInfo.name = "adblockplusandroid";
+  appInfo.platform = "android";

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

Please note that this class changed in the current libadblockplus revision. The
"platform" field has been renamed into "application". Also, there is an
applicationVersion field now.

locale and developmentBuild fields aren't being set here, please add at least a
TODO comment on that.

+
+  AdblockPlus::JsEnginePtr jsEngine(AdblockPlus::JsEngine::New(appInfo));
+
+  AdblockPlus::DefaultFileSystem* defaultFileSystem = new AdblockPlus::DefaultFileSystem();
+  AndroidLogSystem* androidLogSystem = new AndroidLogSystem();
+  AndroidWebRequest* androidWebRequest = new AndroidWebRequest(globalJvm);

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

I think your should assign to a shared pointer immediately instead of leaving
the pointers dangling:

AdblockPlus::FileSystemPtr fileSystem(new DefaultFileSystem());
AdblockPlus::LogSystemPtr logSystem(new AndroidLogSystem());
AdblockPlus::WebRequestPtr webRequest(new AndroidWebRequest(globalJvm));

You can cast to the right type later if necessary:

dynamic_cast<AdblockPlus::DefaultFileSystem*>(jsEngine->GetFileSystem().get())->SetBasePath(path);

+
+  defaultFileSystem->SetBasePath(path);

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

You initialize the path variable 15 lines before it is used. I suggest moving
the initialization down here, then it will be obvious what you need the variable
for.

+  jsEngine->SetLogSystem(AdblockPlus::LogSystemPtr(androidLogSystem));
+  jsEngine->SetFileSystem(AdblockPlus::FileSystemPtr(defaultFileSystem));
+  jsEngine->SetWebRequest(AdblockPlus::WebRequestPtr(androidWebRequest));
+  jsEngine->SetEventCallback("updateAvailable", std::tr1::bind(&UpdateAvailableCallback, std::tr1::placeholders::_1));

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

Why use std::tr1::bind() here instead of simply passing in
&UpdateAvailableCallback?

+
+  filterEngine = new AdblockPlus::FilterEngine(jsEngine);
+  filterEngine->SetFilterChangeCallback(&FilterChangedCallback);
+}
+
+JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_release(JNIEnv *pEnv, jobject)
+{
+  D(D_WARN, "nativeRelease()");
+  AdblockPlus::JsEnginePtr jsEngine = filterEngine->GetJsEngine();
+  jsEngine->RemoveEventCallback("updateAvailable");
+  filterEngine->RemoveFilterChangeCallback();

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

I think that you can rely on FilterEngine and JsEngine to do the cleanup,
including releasing your callbacks.

+  delete filterEngine;
+  pEnv->DeleteGlobalRef(jniObject);
+  jniObject = NULL;
+  globalJvm = NULL;
+}
+
+JNIEXPORT jboolean JNICALL Java_org_adblockplus_android_ABPEngine_isFirstRun(JNIEnv *pEnv, jobject)
+{
+  return filterEngine->IsFirstRun() ? JNI_TRUE : JNI_FALSE;
+}
+
+JNIEXPORT jobjectArray JNICALL Java_org_adblockplus_android_ABPEngine_getListedSubscriptions(JNIEnv *pEnv, jobject)
+{
+  D(D_WARN, "getListedSubscriptions()");
+  const std::vector<AdblockPlus::SubscriptionPtr> subscriptions = filterEngine->GetListedSubscriptions();

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

Why are you catching exceptions in the GetRecommendedSubscriptions() call but
not here? Any API call can throw a JS exception, it makes sense to catch
exceptions everywhere (IsFirstRun() call is an exception, no JS code involved
there).

+  return subscriptionsAsJavaArray(pEnv, subscriptions);
+}
+
+JNIEXPORT jobjectArray JNICALL Java_org_adblockplus_android_ABPEngine_getRecommendedSubscriptions(JNIEnv *pEnv, jobject)
+{
+  D(D_WARN, "getRecommendedSubscriptions()");
+  try
+  {
+    const std::vector<AdblockPlus::SubscriptionPtr> subscriptions = filterEngine->FetchAvailableSubscriptions();
+    return subscriptionsAsJavaArray(pEnv, subscriptions);
+  }
+  catch (const std::exception& e)
+  {
+    D(D_ERROR, "Exception: %s", e.what());
+  }
+  catch (...)
+  {
+    D(D_ERROR, "Unknown exception");
+  }
+  return NULL;

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

I don't think that returning a null pointer makes sense, the application won't
be happy about it. You should rather convert the C++ exception into a Java
exception here:

catch (const std::exception& e)
{
  ThrowJavaException(pEnv, e);
}
catch (...)
{
  ThrowJavaException(pEnv);
}
return NULL;

ThrowJavaException() helper should get the exception parameter and call
pEnv->ThrowNew().

+}
+
+JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_addSubscription(JNIEnv *pEnv, jobject, jstring url)
+{
+  D(D_WARN, "addSubscription()");
+  const std::string surl = GetString(pEnv, url);
+  AdblockPlus::SubscriptionPtr subscription = filterEngine->GetSubscription(surl);
+  subscription->AddToList();
+}
+
+JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_removeSubscription(JNIEnv *pEnv, jobject, jstring url)
+{
+  D(D_WARN, "removeSubscription()");
+  const std::string surl = GetString(pEnv, url);
+  AdblockPlus::SubscriptionPtr subscription = filterEngine->GetSubscription(surl);
+  if (subscription->IsListed())

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

Calling IsListed() is unnecessary, that duplicates a check that is performed
inside libadblockplus anyway.

+  {
+    subscription->RemoveFromList();
+  }
+}
+
+JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_refreshSubscription(JNIEnv *pEnv, jobject, jstring url)
+{
+  D(D_WARN, "refreshSubscription()");
+  const std::string surl = GetString(pEnv, url);
+  AdblockPlus::SubscriptionPtr subscription = filterEngine->GetSubscription(surl);
+  subscription->UpdateFilters();
+}
+
+JNIEXPORT void JNICALL Java_org_adblockplus_android_ABPEngine_actualizeSubscriptionStatus(JNIEnv *pEnv, jobject, jstring url)

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

Nit: I don't think that "actualize" is really what you mean here, call this
method updateSubscriptionStatus() maybe?

+{
+  D(D_WARN, "actualizeSubscriptionStatus()");
+  const std::string surl = GetString(pEnv, url);
+  UpdateSubscriptionStatus(surl);
+}
+
+JNIEXPORT jboolean JNICALL Java_org_adblockplus_android_ABPEngine_matches(JNIEnv *pEnv, jobject, jstring url, jstring contentType, jstring documentUrl)
+{
+  const std::string surl = GetString(pEnv, url);
+  const std::string stype = GetString(pEnv, contentType);
+  const std::string sdoc = GetString(pEnv, documentUrl);
+
+  AdblockPlus::FilterPtr filter = filterEngine->Matches(surl, stype, sdoc);
+
+  if (! filter)
+    return JNI_FALSE;
+
+  // hack: if there is no referrer block only if filter is domain-specific
+  // (to re-enable in-app ads blocking, proposed on 12.11.2012 Monday meeting)
+  // documentUrl contains the referrer (Android application special case)
+  if (sdoc.empty() && (filter->GetProperty("text")->AsString()).find("||") != std::string::npos)

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

Nit: the parentheses around filter->GetProperty("text")->AsString() are
unnecessary.

Also, I think that the logic is reverted here, shouldn't it be |.find("||") ==
std::string::npos| instead?

[Felix Dahlke, 2013/11/07 18:27:23]

I don't really get what this is supposed to do - why are we looking for filters
containing (nor not containing) "||"? Seems arbitrary.

[Wladimir Palant, 2013/11/08 06:16:30]

As the comment says, it looks for domain-specific filters. From what I remember,
the issue that made us implement this hack were false positives breaking some
apps. Our work-around was to apply only specific filters to requests seemingly
originating from apps, not the generic ones.

+    return JNI_FALSE;
+
+  return filter->GetType() == AdblockPlus::Filter::TYPE_EXCEPTION ? JNI_FALSE : JNI_TRUE;
+}
+
+JNIEXPORT jobjectArray JNICALL Java_org_adblockplus_android_ABPEngine_getSelectorsForDomain(JNIEnv *pEnv, jobject, jstring domain)
+{
+  const std::string sdomain = GetString(pEnv, domain);
+  const std::vector<std::string> selectors = filterEngine->GetElementHidingSelectors(sdomain);
+
+  static jclass cls = reinterpret_cast<jclass>(pEnv->NewGlobalRef(pEnv->FindClass("java/lang/String")));
+
+  D(D_WARN, "Selectors: %d", selectors.size());
+  const jobjectArray ret = (jobjectArray) pEnv->NewObjectArray(selectors.size(), cls, NULL);
+
+  int i = 0;
+  for (std::vector<std::string>::const_iterator it = selectors.begin();
+       it != selectors.end(); it++)
+  {
+    jstring selector = pEnv->NewStringUTF((*it).c_str());
+    pEnv->SetObjectArrayElement(ret, i, selector);
+    pEnv->DeleteLocalRef(selector);
+    i++;
+  }
+
+  return ret;
+}
+
+