Issue 4188 - Update libadblockplus-android to use the latest libadblockplus

Issue 4188 - Update libadblockplus-android to use the latest libadblockplus

depends on https://codereview.adblockplus.org/29364548 (binaries)

[anton, 2016-06-27 06:52:45]

Compiled libadblockplus on linux with ndk r10c, replaced .a files in
libadblockplus-binaries manually, edited libadblockplus-android sources to be
compilable. Checked tests working (compilable and runnable on android emulator).

The main concerns are: is it safe to pass rvalue reference to JsValue to filter
and subscription ctor since the data can be moved out of JsValue due to 'move':
{{{
static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz, jlong jsValue)
{
  try
  {
    AdblockPlus::JsValue *jsValuePtr = JniGetJsValue(jsValue);
    return JniPtrToLong(new AdblockPlus::FilterPtr(
      new AdblockPlus::Filter(std::move(*jsValuePtr))));
  }
  CATCH_THROW_AND_RETURN(env, 0)
}
}}}

ps. since the files to be changed are binary not all of them were uploaded for
review, but actually all files were replaced (libadblockplus.a, libv8_base.a,
libv8_snapshot.a)

[sergei, 2016-06-29 11:14:10]

Actually, Filter and Subscription should be constructed only by FilterEngine, so
to avoid potential bugs I would propose to remove these constructors.

[René Jeschke, 2016-06-29 11:16:37]

On 2016/06/29 11:14:10, sergei wrote:
> Actually, Filter and Subscription should be constructed only by FilterEngine,
so
> to avoid potential bugs I would propose to remove these constructors.

You have to take into account how the bindings work. These constructors need to
be there because we
need to be able to wrap an returned object-pointer into a Java representation.

[anton, 2016-06-29 11:23:18]

On 2016/06/29 11:16:37, René Jeschke wrote:
> On 2016/06/29 11:14:10, sergei wrote:
> > Actually, Filter and Subscription should be constructed only by
FilterEngine,
> so
> > to avoid potential bugs I would propose to remove these constructors.
> 
> You have to take into account how the bindings work. These constructors need
to
> be there because we
> need to be able to wrap an returned object-pointer into a Java representation.

The constructors are not used from java source code (new Filter()), but from cpp
jni code:
https://hg.adblockplus.org/adblockplusandroid/file/tip/jni/Utils.cpp#l109 like
this:
https://hg.adblockplus.org/adblockplusandroid/file/tip/jni/JniFilterEngine.cp...

[sergei, 2016-06-29 11:36:06]

On 2016/06/29 11:16:37, René Jeschke wrote:
> On 2016/06/29 11:14:10, sergei wrote:
> > Actually, Filter and Subscription should be constructed only by
FilterEngine,
> so
> > to avoid potential bugs I would propose to remove these constructors.
> 
> You have to take into account how the bindings work. These constructors need
to
> be there because we
> need to be able to wrap an returned object-pointer into a Java representation.

That's clear but where does jsValue come from? Imagine that Filter has a private
constructor and some additional members which are initialized in member function
of FilterEngine.

I can guess that jsValue is already a pointer to Filter or Subscription, so it
should be better to copy only smart pointer and don't touch the internals of
derived classes, is it possible?

[René Jeschke, 2016-06-29 12:08:00]

On 2016/06/29 11:36:06, sergei wrote:
> On 2016/06/29 11:16:37, René Jeschke wrote:
> > On 2016/06/29 11:14:10, sergei wrote:
> > > Actually, Filter and Subscription should be constructed only by
> FilterEngine,
> > so
> > > to avoid potential bugs I would propose to remove these constructors.
> > 
> > You have to take into account how the bindings work. These constructors need
> to
> > be there because we
> > need to be able to wrap an returned object-pointer into a Java
representation.
> 
> That's clear but where does jsValue come from? Imagine that Filter has a
private
> constructor and some additional members which are initialized in member
function
> of FilterEngine.
> 
> I can guess that jsValue is already a pointer to Filter or Subscription, so it
> should be better to copy only smart pointer and don't touch the internals of
> derived classes, is it possible?

As Filter is derived from JsValue having a purely private ctor will break this
all anyways. There's a copy ctor in JsValue. If you do not want inheritance,
avoid it. And if you really need private methods to initialize something, then
make other ways of instancing impossible. As long as you do have a public ctor,
any form of initialization beside of that need to be documented or better 
prohibited ... otherwise the API should just be considered broken.

[sergei, 2016-06-29 13:18:28]

On 2016/06/29 12:08:00, René Jeschke wrote:
> On 2016/06/29 11:36:06, sergei wrote:
> > On 2016/06/29 11:16:37, René Jeschke wrote:
> > > On 2016/06/29 11:14:10, sergei wrote:
> > > > Actually, Filter and Subscription should be constructed only by
> > FilterEngine,
> > > so
> > > > to avoid potential bugs I would propose to remove these constructors.
> > > 
> > > You have to take into account how the bindings work. These constructors
need
> > to
> > > be there because we
> > > need to be able to wrap an returned object-pointer into a Java
> representation.
> > 
> > That's clear but where does jsValue come from? Imagine that Filter has a
> private
> > constructor and some additional members which are initialized in member
> function
> > of FilterEngine.
> > 
> > I can guess that jsValue is already a pointer to Filter or Subscription, so
it
> > should be better to copy only smart pointer and don't touch the internals of
> > derived classes, is it possible?
> 
> As Filter is derived from JsValue having a purely private ctor will break this
> all anyways.
I guess you mean that it's still possible to "copy Filter to JsValue" and lose
some data or even get inconsistency, e.g. if we use a pointer to derived class
(Filter) from base class (JsValue). Right?

> There's a copy ctor in JsValue.
JIC, there is no copy ctor in JsValue, there is only move-ctr, however the
troubles mentioned above are still possible.

> If you do not want inheritance, avoid it.
Actually I would like to avoid inheritance from JsValue here but it will require
a lot of changes, additional long codereview(s) and may be some additional work
for dependent projects. I would like to wait when it really hurts.

> And if you really need private methods to initialize something, then
> make other ways of instancing impossible. As long as you do have a public
ctor,
> any form of initialization beside of that need to be documented or better 
> prohibited ... otherwise the API should just be considered broken.
Actually it is already documented that normally this constructor should not be
used and it's not required to have it private right now. Currently it works but
I as I proposed above it would be better to avoid such way of constructing of
Filter. It can require not only to make changes later again but can be also
dangerous because one has to to track all changes in libadblockplus to keep it
relevant. I see that JniCtor is actually needed but it seems the implementation
could be simpler and safer.

[sergei, 2016-08-30 09:45:57]

https://codereview.adblockplus.org/29347012/diff/29347014/libadblockplus-andr...
File libadblockplus-android/jni/JniFilter.cpp (right):

https://codereview.adblockplus.org/29347012/diff/29347014/libadblockplus-andr...
libadblockplus-android/jni/JniFilter.cpp:33: new
AdblockPlus::Filter(std::move(*jsValuePtr))));
What about using of
return JniPtrToLong(new
AdblockPlus::FilterPtr(dynamic_cast<AdblockPlus::FilterPtr&>(JniGetJsValuePtr(jsValue)));

and similar for Subscription?

We construct C++ Filter and Subscriptions in C++ FilterEngine and can do
whatever we need there, it's better to keep it that way and avoid constructing
of them manually.
- Subscription can have some members and the things get complicated.
- moving of *jsValuePtr can steal the actual value from FilterEngine, although
it's not true now.

However, in that case the internal state of Java Filter or Subscription is also
shared with others but on practice it's not shared now.

PS:
If there are some issues with address arithmetic in dynamic_cast then we should
rather address
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and....

[sergei, 2016-08-30 10:07:32]

https://codereview.adblockplus.org/29347012/diff/29347014/libadblockplus-andr...
File libadblockplus-android/jni/JniFilter.cpp (right):

https://codereview.adblockplus.org/29347012/diff/29347014/libadblockplus-andr...
libadblockplus-android/jni/JniFilter.cpp:33: new
AdblockPlus::Filter(std::move(*jsValuePtr))));
On 2016/08/30 09:45:57, sergei wrote:
> What about using of
> return JniPtrToLong(new
>
AdblockPlus::FilterPtr(dynamic_cast<AdblockPlus::FilterPtr&>(JniGetJsValuePtr(jsValue)));
> 
> and similar for Subscription?
> 
> We construct C++ Filter and Subscriptions in C++ FilterEngine and can do
> whatever we need there, it's better to keep it that way and avoid constructing
> of them manually.
> - Subscription can have some members and the things get complicated.
> - moving of *jsValuePtr can steal the actual value from FilterEngine, although
> it's not true now.
> 
> However, in that case the internal state of Java Filter or Subscription is
also
> shared with others but on practice it's not shared now.
> 
> PS:
> If there are some issues with address arithmetic in dynamic_cast then we
should
> rather address
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and....

Even better without affecting address arithmetic
return JniPtrToLong(new
AdblockPlus::FilterPtr(*JniLongToTypePtr<AdblockPlus::FilterPtr>(jsValue)));
or even
return jsValue;

[anton, 2016-08-30 10:57:29]

https://codereview.adblockplus.org/29347012/diff/29347014/libadblockplus-andr...
File libadblockplus-android/jni/JniFilter.cpp (right):

https://codereview.adblockplus.org/29347012/diff/29347014/libadblockplus-andr...
libadblockplus-android/jni/JniFilter.cpp:33: new
AdblockPlus::Filter(std::move(*jsValuePtr))));
On 2016/08/30 09:45:57, sergei wrote:
> What about using of
> return JniPtrToLong(new
>
AdblockPlus::FilterPtr(dynamic_cast<AdblockPlus::FilterPtr&>(JniGetJsValuePtr(jsValue)));
> 
> and similar for Subscription?
> 
> We construct C++ Filter and Subscriptions in C++ FilterEngine and can do
> whatever we need there, it's better to keep it that way and avoid constructing
> of them manually.
> - Subscription can have some members and the things get complicated.
> - moving of *jsValuePtr can steal the actual value from FilterEngine, although
> it's not true now.
> 
> However, in that case the internal state of Java Filter or Subscription is
also
> shared with others but on practice it's not shared now.
> 
> PS:
> If there are some issues with address arithmetic in dynamic_cast then we
should
> rather address
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and....

Done.

[sergei, 2016-08-30 12:52:54]

https://codereview.adblockplus.org/29347012/diff/29350301/libadblockplus-andr...
File libadblockplus-android/jni/JniFilter.cpp (right):

https://codereview.adblockplus.org/29347012/diff/29350301/libadblockplus-andr...
libadblockplus-android/jni/JniFilter.cpp:117: { (char*)"ctor", (char*)"(J)J",
(void*)JniCtor },
Who deletes created in JniCtor "new AdblockPlus::FilterPtr"? Should we add
"dtor"?
I think we should ignore leaking of "jsValue" arg of "JniCtor" for presence
because I guess "new T" (https://issues.adblockplus.org/ticket/4153#comment:1)
is incorrect way.

[sergei, 2016-08-31 13:21:05]

We have discussed it in oral, here is my conclusion: For me it seems the best
option is to remove Java "public Filter" constructor and this JniCtor because
Filter should be instantiated only via factory FilterEngine.

On 2016/06/27 06:52:45, anton wrote:
> ...
> The main concerns are: is it safe to pass rvalue reference to JsValue to
filter
> and subscription ctor since the data can be moved out of JsValue due to
'move':
> ...
It's not safe because it leads to incorrect state of Java "JsValue jsValue"
argument after the call of constructor "public Filter(final JsValue jsValue)".
It's wrong semantically.

https://codereview.adblockplus.org/29347012/diff/29350301/libadblockplus-andr...
File libadblockplus-android/jni/JniFilter.cpp (right):

https://codereview.adblockplus.org/29347012/diff/29350301/libadblockplus-andr...
libadblockplus-android/jni/JniFilter.cpp:117: { (char*)"ctor", (char*)"(J)J",
(void*)JniCtor },
On 2016/08/30 12:52:54, sergei wrote:
> Who deletes created in JniCtor "new AdblockPlus::FilterPtr"? Should we add
> "dtor"?
> I think we should ignore leaking of "jsValue" arg of "JniCtor" for presence
> because I guess "new T" (https://issues.adblockplus.org/ticket/4153#comment:1)
> is incorrect way.

I see that constructor of Filter passes argument to the base class JsValue which
cares about deleting of C++ class. So technically we don't need dtor here.
However there are several issues:
- in JniCtor we create "shared_ptr<Filter>*" but in JniDtor (see JniJsValue.cpp)
we cast it to "shared_ptr<JsValue>*" and call delete operator. It seems it works
so far in that particular case but in general it's like "delete ((B*)new A())"
where A and B are just arbitrary classes, thus we are calling destructor of
class B for the pointer to object of class A.
- It's actually unclear what is behind "jlong jsValue" and if it's a pointer who
is responsible for that pointer?

[anton, 2016-12-07 09:05:20]

On 2016/08/31 13:21:05, sergei wrote:
> We have discussed it in oral, here is my conclusion: For me it seems the best
> option is to remove Java "public Filter" constructor and this JniCtor because
> Filter should be instantiated only via factory FilterEngine.
> 
> On 2016/06/27 06:52:45, anton wrote:
> > ...
> > The main concerns are: is it safe to pass rvalue reference to JsValue to
> filter
> > and subscription ctor since the data can be moved out of JsValue due to
> 'move':
> > ...
> It's not safe because it leads to incorrect state of Java "JsValue jsValue"
> argument after the call of constructor "public Filter(final JsValue jsValue)".
> It's wrong semantically.
> 
>
https://codereview.adblockplus.org/29347012/diff/29350301/libadblockplus-andr...
> File libadblockplus-android/jni/JniFilter.cpp (right):
> 
>
https://codereview.adblockplus.org/29347012/diff/29350301/libadblockplus-andr...
> libadblockplus-android/jni/JniFilter.cpp:117: { (char*)"ctor", (char*)"(J)J",
> (void*)JniCtor },
> On 2016/08/30 12:52:54, sergei wrote:
> > Who deletes created in JniCtor "new AdblockPlus::FilterPtr"? Should we add
> > "dtor"?
> > I think we should ignore leaking of "jsValue" arg of "JniCtor" for presence
> > because I guess "new T"
(https://issues.adblockplus.org/ticket/4153#comment:1)
> > is incorrect way.
> 
> I see that constructor of Filter passes argument to the base class JsValue
which
> cares about deleting of C++ class. So technically we don't need dtor here.
> However there are several issues:
> - in JniCtor we create "shared_ptr<Filter>*" but in JniDtor (see
JniJsValue.cpp)
> we cast it to "shared_ptr<JsValue>*" and call delete operator. It seems it
works
> so far in that particular case but in general it's like "delete ((B*)new A())"
> where A and B are just arbitrary classes, thus we are calling destructor of
> class B for the pointer to object of class A.
> - It's actually unclear what is behind "jlong jsValue" and if it's a pointer
who
> is responsible for that pointer?

Yes, discussed it orally with Sergei. 

In brief:

java filter instance is created using filterEngine.getFilter(String) 
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...

forwarded to native method
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
native method impl is here
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
c++ filter engine creates c++ filter instance
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
creating java filter instance with native instance ptr
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
forwarding to helper method
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
forwarding to helper method which creates instance using reflection
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...

(BTW ctor is found here
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...,
"(J)V" means java signature "void (long)" which corresponds to this ctor
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...,
more about this java signatures definitions
http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/types.html).

ctor itself
(https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...)
is used only in
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
and this ctor
(https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...)
is not used directly anywhere.

Probably it's not used at all or used using reflection (but it's hard to find it
in the sources).

So i guess we can remove ctor from both java file Filter.java and
JniFilter.cpp/h. Rene it is correct or it's still required anywhere?

[anton, 2016-12-07 09:14:43]

On 2016/12/07 09:05:20, anton wrote:
> On 2016/08/31 13:21:05, sergei wrote:
> > We have discussed it in oral, here is my conclusion: For me it seems the
best
> > option is to remove Java "public Filter" constructor and this JniCtor
because
> > Filter should be instantiated only via factory FilterEngine.
> > 
> > On 2016/06/27 06:52:45, anton wrote:
> > > ...
> > > The main concerns are: is it safe to pass rvalue reference to JsValue to
> > filter
> > > and subscription ctor since the data can be moved out of JsValue due to
> > 'move':
> > > ...
> > It's not safe because it leads to incorrect state of Java "JsValue jsValue"
> > argument after the call of constructor "public Filter(final JsValue
jsValue)".
> > It's wrong semantically.
> > 
> >
>
https://codereview.adblockplus.org/29347012/diff/29350301/libadblockplus-andr...
> > File libadblockplus-android/jni/JniFilter.cpp (right):
> > 
> >
>
https://codereview.adblockplus.org/29347012/diff/29350301/libadblockplus-andr...
> > libadblockplus-android/jni/JniFilter.cpp:117: { (char*)"ctor",
(char*)"(J)J",
> > (void*)JniCtor },
> > On 2016/08/30 12:52:54, sergei wrote:
> > > Who deletes created in JniCtor "new AdblockPlus::FilterPtr"? Should we add
> > > "dtor"?
> > > I think we should ignore leaking of "jsValue" arg of "JniCtor" for
presence
> > > because I guess "new T"
> (https://issues.adblockplus.org/ticket/4153#comment:1)
> > > is incorrect way.
> > 
> > I see that constructor of Filter passes argument to the base class JsValue
> which
> > cares about deleting of C++ class. So technically we don't need dtor here.
> > However there are several issues:
> > - in JniCtor we create "shared_ptr<Filter>*" but in JniDtor (see
> JniJsValue.cpp)
> > we cast it to "shared_ptr<JsValue>*" and call delete operator. It seems it
> works
> > so far in that particular case but in general it's like "delete ((B*)new
A())"
> > where A and B are just arbitrary classes, thus we are calling destructor of
> > class B for the pointer to object of class A.
> > - It's actually unclear what is behind "jlong jsValue" and if it's a pointer
> who
> > is responsible for that pointer?
> 
> Yes, discussed it orally with Sergei. 
> 
> In brief:
> 
> java filter instance is created using filterEngine.getFilter(String) 
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
> 
> forwarded to native method
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
> native method impl is here
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
> c++ filter engine creates c++ filter instance
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
> creating java filter instance with native instance ptr
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
> forwarding to helper method
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
> forwarding to helper method which creates instance using reflection
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
> 
> (BTW ctor is found here
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...,
> "(J)V" means java signature "void (long)" which corresponds to this ctor
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...,
> more about this java signatures definitions
> http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/types.html).
> 
> ctor itself
>
(https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...)
> is used only in
>
https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...
> and this ctor
>
(https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...)
> is not used directly anywhere.
> 
> Probably it's not used at all or used using reflection (but it's hard to find
it
> in the sources).
> 
> So i guess we can remove ctor from both java file Filter.java and
> JniFilter.cpp/h. Rene it is correct or it's still required anywhere?

Same about `ctor` for Subscription

[anton, 2016-12-12 14:35:58]

https://codereview.adblockplus.org/29347012/diff/29367256/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Filter.java
(left):

https://codereview.adblockplus.org/29347012/diff/29367256/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Filter.java:28: public
Filter(final JsValue jsValue)
Not used directly

https://codereview.adblockplus.org/29347012/diff/29367256/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Subscription.java
(left):

https://codereview.adblockplus.org/29347012/diff/29367256/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Subscription.java:28:
public Subscription(final JsValue jsValue)
Not used directly too

[sergei, 2016-12-16 10:56:55]

I have not tried it, the change LGTM.

[anton, 2016-12-16 11:00:20]

On 2016/12/16 10:56:55, sergei wrote:
> I have not tried it, the change LGTM.

I've used this for Lightning + ABP dev build #98, tested and confirmed by Scott

[diegocarloslima, 2016-12-16 11:23:06]

On 2016/12/16 10:56:55, sergei wrote:
> I have not tried it, the change LGTM.

LGTM

[Felix Dahlke, 2016-12-20 09:23:25]

LGTM, you can address my comment later.

https://codereview.adblockplus.org/29347012/diff/29367256/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29347012/diff/29367256/dependencies#newcode4
dependencies:4: libadblockplus-android/jni/libadblockplus-binaries =
libadblockplus-binaries hg:8b65075ece0c
git:177af804de51a98c5151e8784b0aa93047e1dc5b
We normally only add the first seven digits for Git hashes, but we can fix that
later.

[anton, 2016-12-20 10:06:44]

https://codereview.adblockplus.org/29347012/diff/29367256/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29347012/diff/29367256/dependencies#newcode4
dependencies:4: libadblockplus-android/jni/libadblockplus-binaries =
libadblockplus-binaries hg:8b65075ece0c
git:177af804de51a98c5151e8784b0aa93047e1dc5b
On 2016/12/20 09:23:23, Felix Dahlke wrote:
> We normally only add the first seven digits for Git hashes, but we can fix
that
> later.

I've used full hash as previously it was that way:
https://hg.adblockplus.org/adblockplusandroid/file/tip/dependencies#l4