Issue 4948 - add possibility to not send data depending on connection properties

libadblockplus-android/jni/JniFilterEngine.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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
@@ skipping 31 matching lines @@
       (jstring) env->CallObjectMethod(jContentType, nameMethod));
   const std::string value = JniJavaToStdString(env, *jValue);
   return AdblockPlus::FilterEngine::StringToContentType(value);
 }
 
 static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz, jlong jsEnginePtr, jlong isAllowedConnectionCallbackPtr)
 {
   try
   {
     AdblockPlus::JsEnginePtr& jsEngine = *JniLongToTypePtr<AdblockPlus::JsEnginePtr>(jsEnginePtr);
-    AdblockPlus::FilterEnginePtr filterEnginePtr = NULL;

[sergei, 2017/03/15 10:01:38]

smart pointer is already initialized to nullptr, so no need for " = NULL" here.

[sergei, 2017/03/15 10:01:38]

I would just call the variable as filterEngine, without Ptr suffix, especially
there is no need for it, like collision with another variable.

[anton, 2017/03/15 10:13:52]

Acknowledged.

+    AdblockPlus::FilterEnginePtr* filterEngine = NULL;
 
     if (isAllowedConnectionCallbackPtr != 0)
     {
-      AdblockPlus::FilterEngine::CreateParameters createParameters;
+      AdblockPlus::FilterEngine::CreationParameters creationParameters;
       JniIsAllowedConnectionTypeCallback* callback =
         JniLongToTypePtr<JniIsAllowedConnectionTypeCallback>(isAllowedConnectionCallbackPtr);
 
       AdblockPlus::FilterEngine::IsConnectionAllowedCallback cppCallback =
         std::bind(&JniIsAllowedConnectionTypeCallback::Callback, callback, std::placeholders::_1);
-      createParameters.isConnectionAllowed = cppCallback;
-
-      filterEnginePtr = AdblockPlus::FilterEngine::Create(jsEngine, createParameters); // here (1) - create engine sync
+      creationParameters.isConnectionAllowedCallback = cppCallback;
+
+      filterEngine = new AdblockPlus::FilterEnginePtr(
+        AdblockPlus::FilterEngine::Create(jsEngine, creationParameters));
     }
     else
     {
-      filterEnginePtr = AdblockPlus::FilterEngine::Create(jsEngine);
+      filterEngine = new AdblockPlus::FilterEnginePtr(
+        AdblockPlus::FilterEngine::Create(jsEngine));
     }
 
-    return JniPtrToLong(filterEnginePtr.get());

[anton, 2017/03/10 06:50:40]

for some (unknown) reason new static methods (instead of ctor previously)
returns incorrect pointer. iw as unable to test because of this

[sergei, 2017/03/15 10:01:38]

TL;DR: Actually here is a bug, but my vote is to accept it as is for present.

Long story:
The bug is that instance of FilterEngine is destroyed after finishing of the
method but we continue to use the dangling pointer. It is destroyed because we
exit from the scope, no one keeps the reference to FilterEngine and
FilterEnginePtr destroys it.

There are several ways to figure it out but I think it should be firstly
addressed in libadblockplus and we should not disturb the code of
libadblockplus-android because of bugs in libadblockplus. On the interface level
I plan to not force the user of libadblockplus to use shared_ptr<FilterEngine>
and change it to std::unique_ptr<FilterEngine> (or even to FilterEngine, though
it's unlikely), it will require to change only this particular part.

I'm ready to accept it because currently we have a circular reference and
FilterEngine is not destroyed, so technically it does work now, though it's
incorrect. Secondly, I think that fixing it now in libadblockplus-android will
introduce only numerous changes most of which will be reverted afterwards, and
this actions are unnecessary and error-prone. So for the sake of simplicity I
think we can accept it for present, then fix in libadblockplus and then change
probably only one line here.

[anton, 2017/03/15 10:13:52]

Acknowledged.

+    return JniPtrToLong(filterEngine);
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 static void JNICALL JniDtor(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  delete JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  delete JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 }
 
 static jboolean JNICALL JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
 {
   try
   {
-    AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+    AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
     return engine->IsFirstRun() ? JNI_TRUE : JNI_FALSE;
   }
   CATCH_THROW_AND_RETURN(env, JNI_FALSE);
 }
 
 static jobject JNICALL JniGetFilter(JNIEnv* env, jclass clazz, jlong ptr, jstring jText)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
   std::string text = JniJavaToStdString(env, jText);
 
   try
   {
     AdblockPlus::FilterPtr filter = engine->GetFilter(text);
 
     return NewJniFilter(env, filter);
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 static jobject JNICALL JniGetListedFilters(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   try
   {
     std::vector<AdblockPlus::FilterPtr> filters = engine->GetListedFilters();
 
     jobject list = NewJniArrayList(env);
 
     for (std::vector<AdblockPlus::FilterPtr>::iterator it = filters.begin(), end = filters.end(); it != end; it++)
     {
       JniAddObjectToList(env, list, *JniLocalReference<jobject>(env, NewJniFilter(env, *it)));
     }
 
     return list;
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 static jobject JNICALL JniGetSubscription(JNIEnv* env, jclass clazz, jlong ptr, jstring jUrl)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
   std::string url = JniJavaToStdString(env, jUrl);
 
   try
   {
     AdblockPlus::SubscriptionPtr subscription = engine->GetSubscription(url);
 
     return NewJniSubscription(env, subscription);
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 static void JNICALL JniShowNextNotification(JNIEnv* env, jclass clazz, jlong ptr, jstring jUrl)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
   std::string url = JniJavaToStdString(env, jUrl);
 
   try
   {
     engine->ShowNextNotification(url);
   }
   CATCH_AND_THROW(env);
 }
 
 static void JNICALL JniSetShowNotificationCallback(JNIEnv* env, jclass clazz,
                                                   jlong ptr, jlong callbackPtr)
 {
-  AdblockPlus::FilterEngine* const engine =
-      JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine =
+      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
   JniShowNotificationCallback* const callback =
       JniLongToTypePtr<JniShowNotificationCallback>(callbackPtr);
   AdblockPlus::FilterEngine::ShowNotificationCallback showNotificationCallback =
       std::bind(&JniShowNotificationCallback::Callback, callback,
                      std::placeholders::_1);
   try
   {
     engine->SetShowNotificationCallback(showNotificationCallback);
   }
   CATCH_AND_THROW(env)
 }
 
 static void JNICALL JniRemoveShowNotificationCallback(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   try
   {
     engine->RemoveShowNotificationCallback();
   }
   CATCH_AND_THROW(env);
 }
 
 static jobject JNICALL JniGetListedSubscriptions(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   try
   {
     std::vector<AdblockPlus::SubscriptionPtr> subscriptions = engine->GetListedSubscriptions();
 
     return SubscriptionsToArrayList(env, subscriptions);
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 static jobject JNICALL JniFetchAvailableSubscriptions(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   try
   {
     std::vector<AdblockPlus::SubscriptionPtr> subscriptions = engine->FetchAvailableSubscriptions();
 
     return SubscriptionsToArrayList(env, subscriptions);
   }
   CATCH_THROW_AND_RETURN(env, 0);
 }
 
 static void JNICALL JniRemoveUpdateAvailableCallback(JNIEnv* env, jclass clazz,
                                                      jlong ptr)
 {
-  AdblockPlus::FilterEngine* const engine =
-      JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine =
+      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
   try
   {
     engine->RemoveUpdateAvailableCallback();
   }
   CATCH_AND_THROW(env)
 }
 
 static void JNICALL JniSetUpdateAvailableCallback(JNIEnv* env, jclass clazz,
                                                   jlong ptr, jlong callbackPtr)
 {
-  AdblockPlus::FilterEngine* const engine =
-      JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine =
+      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
   JniUpdateAvailableCallback* const callback =
       JniLongToTypePtr<JniUpdateAvailableCallback>(callbackPtr);
   AdblockPlus::FilterEngine::UpdateAvailableCallback updateAvailableCallback =
       std::bind(&JniUpdateAvailableCallback::Callback, callback,
                      std::placeholders::_1);
   try
   {
     engine->SetUpdateAvailableCallback(updateAvailableCallback);
   }
   CATCH_AND_THROW(env)
 }
 
 static void JNICALL JniRemoveFilterChangeCallback(JNIEnv* env, jclass clazz, jlong ptr)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   try
   {
     engine->RemoveFilterChangeCallback();
   }
   CATCH_AND_THROW(env)
 }
 
 static void JNICALL JniSetFilterChangeCallback(JNIEnv* env, jclass clazz,
     jlong ptr, jlong filterPtr)
 {
-  AdblockPlus::FilterEngine* engine =
-      JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine =
+      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
   JniFilterChangeCallback* callback = JniLongToTypePtr<JniFilterChangeCallback>(
       filterPtr);
 
   AdblockPlus::FilterEngine::FilterChangeCallback filterCallback =
       std::bind(&JniFilterChangeCallback::Callback, callback,
           std::placeholders::_1, std::placeholders::_2);
 
   try
   {
     engine->SetFilterChangeCallback(filterCallback);
   }
   CATCH_AND_THROW(env)
 }
 
 static void JNICALL JniForceUpdateCheck(JNIEnv* env, jclass clazz, jlong ptr, jlong updaterPtr)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
   JniUpdateCheckDoneCallback* callback =
       JniLongToTypePtr<JniUpdateCheckDoneCallback>(updaterPtr);
 
   AdblockPlus::FilterEngine::UpdateCheckDoneCallback
       updateCheckDoneCallback = 0;
 
   if (updaterPtr)
   {
     updateCheckDoneCallback =
         std::bind(&JniUpdateCheckDoneCallback::Callback, callback,
                        std::placeholders::_1);
   }
 
   try
   {
     engine->ForceUpdateCheck(updateCheckDoneCallback);
   }
   CATCH_AND_THROW(env)
 }
 
 static jobject JNICALL JniGetElementHidingSelectors(JNIEnv* env, jclass clazz,
     jlong ptr, jstring jDomain)
 {
-  AdblockPlus::FilterEngine* engine =
-      JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine =
+      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   std::string domain = JniJavaToStdString(env, jDomain);
 
   try
   {
     std::vector<std::string> selectors = engine->GetElementHidingSelectors(
         domain);
 
     jobject list = NewJniArrayList(env);
 
     for (std::vector<std::string>::iterator it = selectors.begin(), end =
         selectors.end(); it != end; it++)
     {
       JniAddObjectToList(env, list,
           *JniLocalReference<jstring>(env, env->NewStringUTF(it->c_str())));
     }
 
     return list;
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 static jobject JNICALL JniMatches(JNIEnv* env, jclass clazz, jlong ptr, jstring jUrl, jobject jContentType, jstring jDocumentUrl)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   std::string url = JniJavaToStdString(env, jUrl);
   AdblockPlus::FilterEngine::ContentType contentType =
       ConvertContentType(env, jContentType);
   std::string documentUrl = JniJavaToStdString(env, jDocumentUrl);
 
   try
   {
     AdblockPlus::FilterPtr filter = engine->Matches(url, contentType, documentUrl);
 
@@ skipping 16 matching lines @@
               *JniLocalReference<jstring>(env,
                   static_cast<jstring>(
                       env->GetObjectArrayElement(jArray, i)))));
     }
   }
 }
 
 static jobject JNICALL JniMatchesMany(JNIEnv* env, jclass clazz, jlong ptr,
     jstring jUrl, jobject jContentType, jobjectArray jDocumentUrls)
 {
-  AdblockPlus::FilterEngine* engine =
-      JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine =
+      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   std::string url = JniJavaToStdString(env, jUrl);
   AdblockPlus::FilterEngine::ContentType contentType =
       ConvertContentType(env, jContentType);
 
   std::vector<std::string> documentUrls;
   JavaStringArrayToStringVector(env, jDocumentUrls, documentUrls);
 
   try
   {
     AdblockPlus::FilterPtr filter = engine->Matches(url, contentType,
         documentUrls);
 
     return NewJniFilter(env, filter);
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 static jboolean JNICALL JniIsDocumentWhitelisted(JNIEnv* env, jclass clazz, jlong ptr,
     jstring jUrl, jobjectArray jDocumentUrls)
 {
-  AdblockPlus::FilterEngine* engine =
-      JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine =
+      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   std::string url = JniJavaToStdString(env, jUrl);
   std::vector<std::string> documentUrls;
   JavaStringArrayToStringVector(env, jDocumentUrls, documentUrls);
   try
   {
     return engine->IsDocumentWhitelisted(url, documentUrls) ?
         JNI_TRUE : JNI_FALSE;
   }
   CATCH_THROW_AND_RETURN(env, JNI_FALSE)
 }
 
 static jboolean JNICALL JniIsElemhideWhitelisted(JNIEnv* env, jclass clazz, jlong ptr,
     jstring jUrl, jobjectArray jDocumentUrls)
 {
-  AdblockPlus::FilterEngine* engine =
-      JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine =
+      *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   std::string url = JniJavaToStdString(env, jUrl);
   std::vector<std::string> documentUrls;
   JavaStringArrayToStringVector(env, jDocumentUrls, documentUrls);
   try
   {
     return engine->IsElemhideWhitelisted(url, documentUrls) ?
         JNI_TRUE : JNI_FALSE;
   }
   CATCH_THROW_AND_RETURN(env, JNI_FALSE)
 }
 
 static jobject JNICALL JniGetPref(JNIEnv* env, jclass clazz, jlong ptr, jstring jPref)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr < AdblockPlus::FilterEngine > (ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   std::string pref = JniJavaToStdString(env, jPref);
 
   try
   {
     AdblockPlus::JsValuePtr value = engine->GetPref(pref);
 
     return NewJniJsValue(env, value);
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 static void JNICALL JniSetPref(JNIEnv* env, jclass clazz, jlong ptr, jstring jPref, jlong jsValue)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   std::string pref = JniJavaToStdString(env, jPref);
   AdblockPlus::JsValuePtr value = JniGetJsValuePtr(jsValue);
 
   try
   {
     engine->SetPref(pref, value);
   }
   CATCH_AND_THROW(env)
 }
 
 static jstring JNICALL JniGetHostFromURL(JNIEnv* env, jclass clazz, jlong ptr, jstring jurl)
 {
   if (jurl == NULL)
   {
     return NULL;
   }
 
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
 
   std::string url = JniJavaToStdString(env, jurl);
   try
   {
     std::string host = engine->GetHostFromURL(url);
 
     return JniStdStringToJava(env, host);
   }
   CATCH_THROW_AND_RETURN(env, 0)
 }
 
 static void JNICALL JniSetAllowedConnectionType(JNIEnv* env, jclass clazz, jlong ptr, jstring jvalue)
 {
-  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
-
-  std::string* value = NULL;
-  if (jvalue != NULL)
-  {
-    std::string stdValue = JniJavaToStdString(env, jvalue);
-    value = &stdValue;
-  }
+  AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
+
+  std::string stdValue;
+  const std::string* value = (jvalue != NULL
+    ? &(stdValue = JniJavaToStdString(env, jvalue))
+    : NULL);
 
   try
   {
     engine->SetAllowedConnectionType(value);
   }
   CATCH_AND_THROW(env)
 }
 
-
 static jstring JNICALL JniGetAllowedConnectionType(JNIEnv* env, jclass clazz, jlong ptr)
 {
   try
   {
-    AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
+    AdblockPlus::FilterEnginePtr& engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
     std::unique_ptr<std::string> value = engine->GetAllowedConnectionType();
 
     if (value == NULL)
     {
       return NULL;
     }
 
     return JniStdStringToJava(env, *value.get());
   }
   CATCH_THROW_AND_RETURN(env, 0)
@@ skipping 26 matching lines @@
   { (char*)"getHostFromURL", (char*)"(JLjava/lang/String;)Ljava/lang/String;", (void*)JniGetHostFromURL },
   { (char*)"setAllowedConnectionType", (char*)"(JLjava/lang/String;)V", (void*)JniSetAllowedConnectionType },
   { (char*)"getAllowedConnectionType", (char*)"(J)Ljava/lang/String;", (void*)JniGetAllowedConnectionType },
   { (char*)"dtor", (char*)"(J)V", (void*)JniDtor }
 };
 
 extern "C" JNIEXPORT void JNICALL Java_org_adblockplus_libadblockplus_FilterEngine_registerNatives(JNIEnv *env, jclass clazz)
 {
   env->RegisterNatives(clazz, methods, sizeof(methods) / sizeof(methods[0]));
 }