Issue 5309 - Subscriptions update causes ANR

This code schedules updating of a particular intercepted subscription with zero timeout using the timer of JS engine.

- The timer is not blocked while JS code is running, so the caller won't be blocked.
- The timer is chosen because its lifespan is bound to the JS engine, what should eliminate any potential misuse of JS and FilterEngine and should avoid an addition manual work with a thread.

#depends on https://codereview.adblockplus.org/29465626/

COLLABORATOR=anton@adblockplus.org

[anton, 2017-06-15 05:51:54]

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:545: if (!filterEngine)
"{" required here

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:547: for (auto& subscription :
filterEngine->GetListedSubscriptions())
"}" required here

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
File libadblockplus-android/jni/JniJsEngine.h (right):

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/jni/JniJsEngine.h:24: struct JniJsEngine
Rietveld bug again, and the patch can't be applied ;(

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
(right):

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:204:
public void updateFiltersAsync(String subscriptionUrl)
i don't like the idea of having 'Subscription.updateFilters()' but
'FilterEngine.updateFiltersAsync()' as it looks inconsistent.
Since subscription is created by FilterEngine in JniFilterEngine.cpp we can hold
a reference to FilterEngine instance in Subscription instance and have
'Subscription.updateFiltersAsync()'. Also all subscriptions are released before
FilterEngine is released so it seems to be not significant change here. A other
benefit is we will not have to iterate over all subscriptions and check if URL
matches.

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
(right):

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:218:
Log.d(TAG, "Force subscription update");
previously we invoked updateSubscriptions 1 time for all subsriptions, now we do
it for every single subscription. Since it would be good to output subscription
url in log, smth like this:

`Log.e(TAG, "Force subscription update for " + url)`

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:219:
if (this.adblockEngine.filterEngine != null)
"{" required

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:220:
this.adblockEngine.filterEngine.updateFiltersAsync(url);
"}" required

[sergei, 2017-06-16 11:12:09]

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:545: if (!filterEngine)
On 2017/06/15 05:51:53, anton wrote:
> "{" required here

Done.

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:547: for (auto& subscription :
filterEngine->GetListedSubscriptions())
On 2017/06/15 05:51:53, anton wrote:
> "}" required here

Done.

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
(right):

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:204:
public void updateFiltersAsync(String subscriptionUrl)
On 2017/06/15 05:51:53, anton wrote:
> i don't like the idea of having 'Subscription.updateFilters()' but
> 'FilterEngine.updateFiltersAsync()' as it looks inconsistent.
> Since subscription is created by FilterEngine in JniFilterEngine.cpp we can
hold
> a reference to FilterEngine instance in Subscription instance and have
> 'Subscription.updateFiltersAsync()'. Also all subscriptions are released
before
> FilterEngine is released so it seems to be not significant change here. A
other
> benefit is we will not have to iterate over all subscriptions and check if URL
> matches.

When the request is intercepted there is no subscription object, there is only
an URL, so we will have to iterate through subscriptions anyway. Adding in
addition some JniSubscription which holds FilterEnginePtr does not look as a
good idea because I think subscription should not have a reference to
FilterEngine and that change will cause also some other changes (recall that
there is the weird cast Subscritpion* to JsValue* in the current code). I think
that it's fine to have such inconsistency, and since I also don't find
'Subscription.updateFilters()' good, maybe it makes sense to rather change the
rest API later.

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
(right):

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:218:
Log.d(TAG, "Force subscription update");
On 2017/06/15 05:51:53, anton wrote:
> previously we invoked updateSubscriptions 1 time for all subsriptions, now we
do
> it for every single subscription. Since it would be good to output
subscription
> url in log, smth like this:
> 
> `Log.e(TAG, "Force subscription update for " + url)`

OK, but why error (Log.e)?

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:219:
if (this.adblockEngine.filterEngine != null)
On 2017/06/15 05:51:53, anton wrote:
> "{" required

Done.

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:220:
this.adblockEngine.filterEngine.updateFiltersAsync(url);
On 2017/06/15 05:51:53, anton wrote:
> "}" required 

Done.

[anton, 2017-06-16 11:25:12]

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
(right):

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:204:
public void updateFiltersAsync(String subscriptionUrl)
On 2017/06/16 11:12:08, sergei wrote:
> On 2017/06/15 05:51:53, anton wrote:
> > i don't like the idea of having 'Subscription.updateFilters()' but
> > 'FilterEngine.updateFiltersAsync()' as it looks inconsistent.
> > Since subscription is created by FilterEngine in JniFilterEngine.cpp we can
> hold
> > a reference to FilterEngine instance in Subscription instance and have
> > 'Subscription.updateFiltersAsync()'. Also all subscriptions are released
> before
> > FilterEngine is released so it seems to be not significant change here. A
> other
> > benefit is we will not have to iterate over all subscriptions and check if
URL
> > matches.
> 
> When the request is intercepted there is no subscription object, there is only
> an URL, so we will have to iterate through subscriptions anyway. Adding in
> addition some JniSubscription which holds FilterEnginePtr does not look as a
> good idea because I think subscription should not have a reference to
> FilterEngine and that change will cause also some other changes (recall that
> there is the weird cast Subscritpion* to JsValue* in the current code). I
think
> that it's fine to have such inconsistency, and since I also don't find
> 'Subscription.updateFilters()' good, maybe it makes sense to rather change the
> rest API later.

Acknowledged.

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
(right):

https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:218:
Log.d(TAG, "Force subscription update");
On 2017/06/16 11:12:09, sergei wrote:
> On 2017/06/15 05:51:53, anton wrote:
> > previously we invoked updateSubscriptions 1 time for all subsriptions, now
we
> do
> > it for every single subscription. Since it would be good to output
> subscription
> > url in log, smth like this:
> > 
> > `Log.e(TAG, "Force subscription update for " + url)`
> 
> OK, but why error (Log.e)?

Yes, obviously not `.e`. My bad.

[anton, 2017-06-16 12:35:16]

On 2017/06/16 11:25:12, anton wrote:
>
https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
> File
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
> (right):
> 
>
https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:204:
> public void updateFiltersAsync(String subscriptionUrl)
> On 2017/06/16 11:12:08, sergei wrote:
> > On 2017/06/15 05:51:53, anton wrote:
> > > i don't like the idea of having 'Subscription.updateFilters()' but
> > > 'FilterEngine.updateFiltersAsync()' as it looks inconsistent.
> > > Since subscription is created by FilterEngine in JniFilterEngine.cpp we
can
> > hold
> > > a reference to FilterEngine instance in Subscription instance and have
> > > 'Subscription.updateFiltersAsync()'. Also all subscriptions are released
> > before
> > > FilterEngine is released so it seems to be not significant change here. A
> > other
> > > benefit is we will not have to iterate over all subscriptions and check if
> URL
> > > matches.
> > 
> > When the request is intercepted there is no subscription object, there is
only
> > an URL, so we will have to iterate through subscriptions anyway. Adding in
> > addition some JniSubscription which holds FilterEnginePtr does not look as a
> > good idea because I think subscription should not have a reference to
> > FilterEngine and that change will cause also some other changes (recall that
> > there is the weird cast Subscritpion* to JsValue* in the current code). I
> think
> > that it's fine to have such inconsistency, and since I also don't find
> > 'Subscription.updateFilters()' good, maybe it makes sense to rather change
the
> > rest API later.
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
> File
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
> (right):
> 
>
https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:218:
> Log.d(TAG, "Force subscription update");
> On 2017/06/16 11:12:09, sergei wrote:
> > On 2017/06/15 05:51:53, anton wrote:
> > > previously we invoked updateSubscriptions 1 time for all subsriptions, now
> we
> > do
> > > it for every single subscription. Since it would be good to output
> > subscription
> > > url in log, smth like this:
> > > 
> > > `Log.e(TAG, "Force subscription update for " + url)`
> > 
> > OK, but why error (Log.e)?
> 
> Yes, obviously not `.e`. My bad.

LGTM

[anton, 2017-06-16 13:27:21]

On 2017/06/16 12:35:16, anton wrote:
> On 2017/06/16 11:25:12, anton wrote:
> >
>
https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
> > File
> libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
> >
>
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:204:
> > public void updateFiltersAsync(String subscriptionUrl)
> > On 2017/06/16 11:12:08, sergei wrote:
> > > On 2017/06/15 05:51:53, anton wrote:
> > > > i don't like the idea of having 'Subscription.updateFilters()' but
> > > > 'FilterEngine.updateFiltersAsync()' as it looks inconsistent.
> > > > Since subscription is created by FilterEngine in JniFilterEngine.cpp we
> can
> > > hold
> > > > a reference to FilterEngine instance in Subscription instance and have
> > > > 'Subscription.updateFiltersAsync()'. Also all subscriptions are released
> > > before
> > > > FilterEngine is released so it seems to be not significant change here.
A
> > > other
> > > > benefit is we will not have to iterate over all subscriptions and check
if
> > URL
> > > > matches.
> > > 
> > > When the request is intercepted there is no subscription object, there is
> only
> > > an URL, so we will have to iterate through subscriptions anyway. Adding in
> > > addition some JniSubscription which holds FilterEnginePtr does not look as
a
> > > good idea because I think subscription should not have a reference to
> > > FilterEngine and that change will cause also some other changes (recall
that
> > > there is the weird cast Subscritpion* to JsValue* in the current code). I
> > think
> > > that it's fine to have such inconsistency, and since I also don't find
> > > 'Subscription.updateFilters()' good, maybe it makes sense to rather change
> the
> > > rest API later.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
> > File
> >
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29465639/diff/29465640/libadblockplus-andr...
> >
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:218:
> > Log.d(TAG, "Force subscription update");
> > On 2017/06/16 11:12:09, sergei wrote:
> > > On 2017/06/15 05:51:53, anton wrote:
> > > > previously we invoked updateSubscriptions 1 time for all subsriptions,
now
> > we
> > > do
> > > > it for every single subscription. Since it would be good to output
> > > subscription
> > > > url in log, smth like this:
> > > > 
> > > > `Log.e(TAG, "Force subscription update for " + url)`
> > > 
> > > OK, but why error (Log.e)?
> > 
> > Yes, obviously not `.e`. My bad.
> 
> LGTM

LGTM

[sergei, 2017-07-03 07:55:50]

Could you please review it and I think Anton could commit it after approving.

[diegocarloslima, 2017-07-06 18:46:35]

On 2017/07/03 07:55:50, sergei wrote:
> Could you please review it and I think Anton could commit it after approving.

LGTM