ABP/Android libadblockplus integration

ABP/Android libadblockplus integration

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

I think that lots of the complexity here can be removed with the right set of
helper classes, similarly to how we wrapped the JS engine. For example:

class JString
{
  JString(JNIEnv *pEnv, const std::string& value);
  JString(JNIEnv *pEnv, jstring str);
  ~JString();
  operator jstring();
  std::string GetString() const;
}

class JObject
{
  JObject(JNIEnv *pEnv, const std::string& className, ...);
  JObject(JNIEnv *pEnv, jobject obj);
  ~JObject();
  operator jobject();
  void CallVoidMethod(const std::string& methodName, const std::string&
signature, ...);
  jobject CallObjectMethod(const std::string& methodName, const std::string&
signature, ...);
  jstring CallStringMethod(const std::string& methodName, const std::string&
signature, ...);
  jboolean CallBooleanMethod(const std::string& methodName, const std::string&
signature, ...);
  jint CallIntMethod(const std::string& methodName, const std::string&
signature, ...);
  void SetField(const std::string& fieldName, const std::string& value);
  void SetField(const std::string& fieldName, int value);
}

class JObjectArray
{
  JObjectArray(JNIEnv *pEnv, const std::string& className, size);
  JObject(JNIEnv *pEnv, jobjectArray arr);
  ~JObjectArray();
  operator jobjectArray();
  jobject Get(int index);
  void Set(int index, jobject value);
}

What's important here:

* Cleanup happens automatically in the destructor, you shouldn't keep track of
created objects, strings and global references all over the code. There are some
global references in your code that haven't been cleaned up, also lots of
objects and strings that will leak if an exception is thrown - this doesn't have
to be.
* Looking up classes and methods can happen behind the scenes when JObject
constructor or some Call<type>Method() is called. I doubt that caching the
results is worth the effort, this JNI code doesn't run often enough.
* Exception checking can also happen behind the scenes - the helper classes can
convert Java exceptions into C++ exceptions. This will both reduce the
boilerplate and ensure you don't forget to check for exceptions.
* The assumption is that no objects need to survive the local scope and outlive
pEnv (seems to be correct if you don't cache classes and methods). For return
values I would add a Release() method that will return the Java pointer while
also noting that it doesn't need to be cleaned up.

I didn't comment on any issues that can be avoided by using this approach.

http://codereview.adblockplus.org/11172036/diff/1/jni/Android.mk
File jni/Android.mk (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/Android.mk#newcode39
jni/Android.mk:39: -DENABLE_LOGGING_AND_PROFILING
This definition and ENABLE_DEBUGGER_SUPPORT are also V8-speific, any reason to
keep them here?

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp
File jni/AndroidWebRequest.cpp (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:33: return "Java Exception";
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:45: }
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:117: throw JavaException(jniEnv->ExceptionOccurred());
Seeing this block of code repeated all over the place, it is probably better to
move it into a helper function like CheckExceptions().

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:150: jint lenghtOfFile =
jniEnv->CallIntMethod(jConnection, jConnectionGetContentLengthID);
This variable's name is misspelled, and it isn't actually used anyway - remove?

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:245: if (jHeaderValue == NULL)
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:255: std::transform(headerValue.begin(),
headerValue.end(), headerValue.begin(), ::tolower);
Please do not lower-case the value, only the header name is case-insensitive.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:275: result.status =
AdblockPlus::DefaultWebRequest::NS_ERROR_FAILURE;
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)

http://codereview.adblockplus.org/11172036/diff/1/jni/Application.mk
File jni/Application.mk (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/Application.mk#newcode3
jni/Application.mk:3: APP_ABI	:= armeabi
Replace the tab by a space here?

http://codereview.adblockplus.org/11172036/diff/1/jni/Utils.cpp
File jni/Utils.cpp (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/Utils.cpp#newcode21
jni/Utils.cpp:21: const std::string GetString(JNIEnv *pEnv, jstring str)
This can be a method of the JString helper class.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp
File jni/abpEngine.cpp (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode26
jni/abpEngine.cpp:26: AdblockPlus::FilterEngine* filterEngine;
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode53
jni/abpEngine.cpp:53: const jobjectArray ret = (jobjectArray)
pEnv->NewObjectArray(subscriptions.size(), cls, NULL);
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode117
jni/abpEngine.cpp:117: void FilterChangedCallback(const std::string& action,
const std::string& url)
I think that in the final revision of your libadblockplus change this function
will get JsValuePtr as the second parameter.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode123
jni/abpEngine.cpp:123: UpdateSubscriptionStatus(url);
Nit: Indentation is off here, need to convert a tab to spaces?

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode134
jni/abpEngine.cpp:134: return JNI_VERSION_1_6;
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode151
jni/abpEngine.cpp:151: // FIXME Should be taken from Manifest
Please use "TODO" instead of "FIXME", this is what we are normally searching
for.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode154
jni/abpEngine.cpp:154: appInfo.platform = "android";
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode160
jni/abpEngine.cpp:160: AndroidWebRequest* androidWebRequest = new
AndroidWebRequest(globalJvm);
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);

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode162
jni/abpEngine.cpp:162: defaultFileSystem->SetBasePath(path);
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode166
jni/abpEngine.cpp:166: jsEngine->SetEventCallback("updateAvailable",
std::tr1::bind(&UpdateAvailableCallback, std::tr1::placeholders::_1));
Why use std::tr1::bind() here instead of simply passing in
&UpdateAvailableCallback?

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode177
jni/abpEngine.cpp:177: filterEngine->RemoveFilterChangeCallback();
I think that you can rely on FilterEngine and JsEngine to do the cleanup,
including releasing your callbacks.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode192
jni/abpEngine.cpp:192: const std::vector<AdblockPlus::SubscriptionPtr>
subscriptions = filterEngine->GetListedSubscriptions();
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).

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode212
jni/abpEngine.cpp:212: return NULL;
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().

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode228
jni/abpEngine.cpp:228: if (subscription->IsListed())
Calling IsListed() is unnecessary, that duplicates a check that is performed
inside libadblockplus anyway.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode242
jni/abpEngine.cpp:242: JNIEXPORT void JNICALL
Java_org_adblockplus_android_ABPEngine_actualizeSubscriptionStatus(JNIEnv *pEnv,
jobject, jstring url)
Nit: I don't think that "actualize" is really what you mean here, call this
method updateSubscriptionStatus() maybe?

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode263
jni/abpEngine.cpp:263: if (sdoc.empty() &&
(filter->GetProperty("text")->AsString()).find("||") != std::string::npos)
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?

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
File src/org/adblockplus/android/AdblockPlus.java (right):

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
src/org/adblockplus/android/AdblockPlus.java:56: private final static Pattern
RE_FONT = Pattern.compile(".*\\.(?:ttf|woff)$", Pattern.CASE_INSENSITIVE);
Wouldn't it make sense to drop .* at the beginning of these patterns and to use
Matcher.find() instead of Matcher.matches()?

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
src/org/adblockplus/android/AdblockPlus.java:71: private Subscription[]
subscriptions;
Add a comment explaining that this is the cached list of recommended
subscriptions?

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
src/org/adblockplus/android/AdblockPlus.java:278: * @return ready to use HTML
element with CSS selectors
This comment doesn't seem to match the actual return value.

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
src/org/adblockplus/android/AdblockPlus.java:366: //     
jsEngine.put("_version", getVersion());
I guess you intend to remove that code or replace it by some TODO comment?

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
src/org/adblockplus/android/AdblockPlus.java:385: */
I guess you intend to remove that code?

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
src/org/adblockplus/android/Preferences.java:142: boolean firstRun = false;
I don't think you need the firstRun variable, just use application.isFirstRun()
when necessary. E.g. below you should have |if (application.isFirstRun() &&
current != null)|.

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/brazil/...
File src/org/adblockplus/brazil/RequestHandler.java (right):

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/brazil/...
src/org/adblockplus/brazil/RequestHandler.java:308: // TODO Do we need to set
encoding here?
Yes, pretty certainly. We need to get the encoding from the Content-Type header
at least and encode the contents of this tag accordingly.

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

Reviewed, sorry for not getting around to this earlier. It looks like a lot of
work, so I'd be happy to help with this, Andrey.

http://codereview.adblockplus.org/11172036/diff/1/jni/Android.mk
File jni/Android.mk (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/Android.mk#newcode29
jni/Android.mk:29: LOCAL_CPP_FEATURES 	:= rtti #exceptions
Why is exceptions commented out? We're using exceptions.

http://codereview.adblockplus.org/11172036/diff/1/jni/Android.mk#newcode30
jni/Android.mk:30: LOCAL_CFLAGS += \
Do we really need all these flags? I'm thinking -std=gnu++0x should suffice.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp
File jni/AndroidWebRequest.cpp (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:37: AndroidWebRequest::AndroidWebRequest(JavaVM*&
gJvm) : globalJvm(gJvm)
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:112: // URL jUrl = new URL(url)
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.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:117: throw JavaException(jniEnv->ExceptionOccurred());
On 2013/09/12 11:31:14, Wladimir Palant wrote:
> Seeing this block of code repeated all over the place, it is probably better
to
> move it into a helper function like CheckExceptions().

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.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:118: jniEnv->DeleteLocalRef(jUrlStr);
DeleteLocalRef shouldn't be necessary here. It'll be garbage collected once we
detach the thread. Same goes for the other DeleteLocalRef calls below.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:233: while (true)
How about |for (jint i = 0; ; i++)| instead?

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.cpp#n...
jni/AndroidWebRequest.cpp:245: if (jHeaderValue == NULL)
On 2013/09/12 11:31:14, Wladimir Palant wrote:
> 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.

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.

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.h
File jni/AndroidWebRequest.h (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/AndroidWebRequest.h#new...
jni/AndroidWebRequest.h:18: #ifndef ADBLOCKPLUS_WEB_REQUEST_ANDROID_H
Nit: We typically write ADBLOCK_PLUS_*

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/11172036/diff/1/src/org/adblockplus/android...
src/org/adblockplus/android/Preferences.java:124:
subscriptionList.setEntryValues(entryValues);
Nit: Trailing whitespace.

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

Andrey fixed some issues already, but I guess I'll take this over since it's
blocking other work. So let's close this review (please do that Andrey, I
can't).

I fixed all whitespace/comment issues in the following commits:
https://hg.adblockplus.org/adblockplusandroid/rev/3ef7be5bb2dd
https://hg.adblockplus.org/adblockplusandroid/rev/909c3564a0f1
https://hg.adblockplus.org/adblockplusandroid/rev/a5836e8db9e1
https://hg.adblockplus.org/adblockplusandroid/rev/d9c76f5bfbdb
https://hg.adblockplus.org/adblockplusandroid/rev/2ce664f0f66b
https://hg.adblockplus.org/adblockplusandroid/rev/806b2111b654
https://hg.adblockplus.org/adblockplusandroid/rev/e62f5070b0ed

I moved all the other stuff that needs testing and some more work to a Trello
card:
https://trello.com/c/gywOLz7n/331-clean-up-libadblockplus-integration

From all I can tell, the integration works pretty well now, I had to fix one
bug:
http://codereview.adblockplus.org/28457002/

I'm in favour of merging the libadblockplus branch now, addressing issues as we
go. Objections?

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp
File jni/abpEngine.cpp (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode263
jni/abpEngine.cpp:263: if (sdoc.empty() &&
(filter->GetProperty("text")->AsString()).find("||") != std::string::npos)
On 2013/09/12 11:31:14, Wladimir Palant wrote:
> 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?

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]

On 2013/11/07 18:27:23, Felix H. Dahlke wrote:
> I fixed all whitespace/comment issues in the following commits:
> https://hg.adblockplus.org/adblockplusandroid/rev/3ef7be5bb2dd
> https://hg.adblockplus.org/adblockplusandroid/rev/909c3564a0f1
> https://hg.adblockplus.org/adblockplusandroid/rev/a5836e8db9e1
> https://hg.adblockplus.org/adblockplusandroid/rev/d9c76f5bfbdb
> https://hg.adblockplus.org/adblockplusandroid/rev/2ce664f0f66b
> https://hg.adblockplus.org/adblockplusandroid/rev/806b2111b654
> https://hg.adblockplus.org/adblockplusandroid/rev/e62f5070b0ed

LGTM for these changes.

> I'm in favour of merging the libadblockplus branch now, addressing issues as
we
> go. Objections?

Not sure whether this is really a good idea. We have 10k development build users
and some of the things I noted look like they could be stability issues (e.g.
the one below). It's probably better to fix the most important issues first.

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp
File jni/abpEngine.cpp (right):

http://codereview.adblockplus.org/11172036/diff/1/jni/abpEngine.cpp#newcode263
jni/abpEngine.cpp:263: if (sdoc.empty() &&
(filter->GetProperty("text")->AsString()).find("||") != std::string::npos)
On 2013/11/07 18:27:23, Felix H. Dahlke wrote:
> I don't really get what this is supposed to do - why are we looking for
filters
> containing (nor not containing) "||"? Seems arbitrary.

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.

[Felix Dahlke, 2013-12-02 14:52:54]

Can you close this one, Andrey?