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

# based on
# - https://codereview.adblockplus.org/29377064/

[Eric, 2017-02-28 17:35:37]

There's no reason for this change to touch C++ code. Everything necessary can be
done by changing the algorithm for scheduling downloads in "updater.js". The
ticket even says it should be a change in core, so if that's to happen, its
implementation best not contain C++.

What's really necessary is to minimize the use of the network, not to eliminate
it. The ticket seems confused on this point. If the library is never allowed to
use the network, it will never be able to download filter lists in the first
place and might as well not be installed at all. At the very minimum, it means
that the application will have to allow connections initially until initial
filters are downloaded, and I don't believe we have an API that allows verifying
or waiting on such a condition.

A better way of dealing with this would be to (1) create a flag for manual or
automatic filter list updates, and (2) provide an API for a manual filter list
update. That takes care of any undesired automatic behavior in the engine and
leaves all the application logic with the application. The engine probably ought
to automatically download filter lists initially, but if not that, we'd need an
API predicate to determine whether filter lists exist or not.

https://codereview.adblockplus.org/29377570/diff/29377571/src/WebRequestJsObj...
File src/WebRequestJsObject.cpp (right):

https://codereview.adblockplus.org/29377570/diff/29377571/src/WebRequestJsObj...
src/WebRequestJsObject.cpp:70: jsEngine->GetWebRequest()->GET(url, headers) :
NotAllowedResponse();
This is an absolutely awful place to intervene. It means, among other things,
that it will be impossible to distinguish between an actual failure and a
synthetic one unless you've got access to global configuration preference, and
that's just a bad design choice.

[sergei, 2017-02-28 22:27:23]

On 2017/02/28 17:35:37, Eric wrote:
> There's no reason for this change to touch C++ code. Everything necessary can
be
> done by changing the algorithm for scheduling downloads in "updater.js". The
> ticket even says it should be a change in core, so if that's to happen, its
> implementation best not contain C++.
updater.js is merely to check the presence of a new version of an application
built on libadblockplus, like addon for IE, but it's not related to cases when
libadblockplus is used by our partners on mobile platforms where this issue
origins from. The aim is to prevent downloading of subscriptions on a limited
connection.

All concerns below are known but it had been decided to have a quick hack in
libadblockplus for present to have something working. Please keep it mind while
reading the comments and the code.

> 
> What's really necessary is to minimize the use of the network, not to
eliminate
> it.
That's what would be good to have finally. I'm pretty sure that we could still
send HEAD requests for subscriptions because of various reasons and requests
with relatively small responses, like request for notification.json. One of the
reasons, for example, is that, I think, that it is good for user to know that
there are new filters and that they should be automatically downloaded when
there is a good connection, not in days when the chance to fail again is very
high.
> The ticket seems confused on this point. If the library is never allowed to
> use the network, it will never be able to download filter lists in the first
> place
That's correct. First of all there are discussions about shipping of some
subscriptions with the application (which is also not supported right now and
has many flaws), secondly it should be very good reflected in UX.
> and might as well not be installed at all.
It's not clear, what cannot be installed.
> At the very minimum, it means
> that the application will have to allow connections initially until initial
> filters are downloaded, and I don't believe we have an API that allows
verifying
> or waiting on such a condition.
We don't have it now and I think it's not decided yet what exact functionality
should be there and how exactly it should work.
> 
> A better way of dealing with this would be to (1) create a flag for manual or
> automatic filter list updates, and (2) provide an API for a manual filter list
> update. That takes care of any undesired automatic behavior in the engine and
> leaves all the application logic with the application. The engine probably
ought
> to automatically download filter lists initially, but if not that, we'd need
an
> API predicate to determine whether filter lists exist or not.
It had been also discussed, it's one of the reasons, I think, it should be in
adblockpluscore.

https://codereview.adblockplus.org/29377570/diff/29377571/src/WebRequestJsObj...
File src/WebRequestJsObject.cpp (right):

https://codereview.adblockplus.org/29377570/diff/29377571/src/WebRequestJsObj...
src/WebRequestJsObject.cpp:70: jsEngine->GetWebRequest()->GET(url, headers) :
NotAllowedResponse();
On 2017/02/28 17:35:37, Eric wrote:
> This is an absolutely awful place to intervene. It means, among other things,
> that it will be impossible to distinguish between an actual failure and a
> synthetic one unless you've got access to global configuration preference, and
> that's just a bad design choice.

It is also one of known concerns, yes, UX will suffer from it until it's in
adblockpluscore.

[Eric, 2017-02-28 22:57:14]

On 2017/02/28 22:27:23, sergei wrote:
> The aim is to prevent downloading of subscriptions on a limited
> connection.

I don't know the subscription code well, I must admit. If none of it's in this
library, then you will need some kind of brute force hack.

Honestly, though, if mobile is so important and this is best done in core, why
not do it there in the first place with adequate priority? It seems needing to
do anything like a hack here is compensating for some organizational hurdle that
might not even be present.

> All concerns below are known but it had been decided to have a quick hack in
> libadblockplus for present to have something working.

You can do both of these things, you can do them in the JS code of
libadblockplus, and you don't need to touch C++ to do it.

If you want a quick hack, overwrite "<global>._webRequest.GET" in JS code with a
stub implementation. This can be done at any point after the engine has
initialized. If you want to be able to go back and forth, keep a copy of the
function object and reassign it. You don't need to go mucking inside the web
request implementation in C++ if all you're trying to do is change the behavior
of GET() to return an error. This can all be done with JS code.

This has the virtue of being very easy to revert once it's no longer needed,
since the changes are localized to some initialization and API code in JS.

[sergei, 2017-02-28 23:40:11]

On 2017/02/28 22:57:14, Eric wrote:
> On 2017/02/28 22:27:23, sergei wrote:
> > The aim is to prevent downloading of subscriptions on a limited
> > connection.
> 
> I don't know the subscription code well, I must admit. If none of it's in this
> library, then you will need some kind of brute force hack.
> 
> Honestly, though, if mobile is so important and this is best done in core, why
> not do it there in the first place with adequate priority? It seems needing to
> do anything like a hack here is compensating for some organizational hurdle
that
> might not even be present.
> 
> > All concerns below are known but it had been decided to have a quick hack in
> > libadblockplus for present to have something working.
> 
> You can do both of these things, you can do them in the JS code of
> libadblockplus, and you don't need to touch C++ to do it.
> 
> If you want a quick hack, overwrite "<global>._webRequest.GET" in JS code with
a
> stub implementation.
> This can be done at any point after the engine has
> initialized. If you want to be able to go back and forth, keep a copy of the
> function object and reassign it. You don't need to go mucking inside the web
> request implementation in C++ if all you're trying to do is change the
behavior
> of GET() to return an error. This can all be done with JS code.
It sounds like you propose to frequently monitor current connection or subscribe
to its changes if it's possible and update some flag depending on it. The flag
is a bit abstract term, it can be also "<global>._webRequest.GET" where the
value of this flag corresponds to some function. However I think that we should
query the system whether we may make a heavy request right now before making of
the request. The latter approach seems to be more reliable, working in more
cases and easier for both us and users of libadblockplus.
> 
> This has the virtue of being very easy to revert once it's no longer needed,
> since the changes are localized to some initialization and API code in JS.
Is it difficult now to remove changes in JsEngine and in WebRequestJsObject.cpp?

Another moment. Let's say we have defined this callback in JS and call it from
_webRequest.GET in JS and let's assume there is no other possible issues with
it, but the querying of the system can be slow and it's a good way to not block
JS code. It seems more complicated to add asynchrony there than to reuse the
thread from web requests.

[anton, 2017-03-07 20:45:51]

On 2017/02/28 14:36:23, sergei wrote:

something is broken here:

03-08 01:22:51.510 5960-5998/? D/libadblockplus: Getting pref for
subscriptions_exceptionsurl
                                                 
                                                 --------- beginning of crash
03-08 01:22:51.510 5960-5998/? A/libc: Fatal signal 11 (SIGSEGV), code 2, fault
addr 0xa1f3e244 in tid 5998 (Thread-318)
03-08 01:22:51.613 1170-1170/? I/DEBUG: *** *** *** *** *** *** *** *** *** ***
*** *** *** *** *** ***
03-08 01:22:51.613 1170-1170/? I/DEBUG: Build fingerprint:
'Android/sdk_google_phone_x86/generic_x86:5.1.1/LMY48X/3728910:userdebug/test-keys'
03-08 01:22:51.613 1170-1170/? I/DEBUG: Revision: '0'
03-08 01:22:51.613 1170-1170/? I/DEBUG: ABI: 'x86'
03-08 01:22:51.613 1170-1170/? I/DEBUG: pid: 5960, tid: 5998, name: Thread-318 
>>> org.adblockplus.libadblockplus.android.webviewapp <<<
03-08 01:22:51.613 1170-1170/? I/DEBUG: signal 11 (SIGSEGV), code 2
(SEGV_ACCERR), fault addr 0xa1f3e244
03-08 01:22:51.620 1170-1170/? I/DEBUG:     eax f0e48156  ebx a24f2d30  ecx
f0e48157  edx a1f3e240
03-08 01:22:51.620 1170-1170/? I/DEBUG:     esi a1f3e244  edi a24d88b8
03-08 01:22:51.620 1170-1170/? I/DEBUG:     xcs 00000073  xds 0000007b  xes
0000007b  xfs 000000af  xss 0000007b
03-08 01:22:51.620 1170-1170/? I/DEBUG:     eip a229e4fc  ebp a26fb738  esp
a26fb734  flags 00210293
03-08 01:22:51.620 1170-1170/? I/DEBUG: backtrace:
03-08 01:22:51.621 1170-1170/? I/DEBUG:     #00 pc 005204fc 
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
(std::__1::__shared_weak_count::lock()+28)
03-08 01:22:51.621 1170-1170/? I/DEBUG:     #01 pc 001bd609 
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
(AdblockPlus::JsEngine::Evaluate(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> > const&)+57)
03-08 01:22:51.621 1170-1170/? I/DEBUG:     #02 pc 001b8094 
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
(AdblockPlus::FilterEngine::GetPref(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&) const+132)
03-08 01:22:51.621 1170-1170/? I/DEBUG:     #03 pc 001a6bfa 
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
(JniGetPref(_JNIEnv*, _jclass*, long long, _jstring*)+138)
03-08 01:22:51.621 1170-1170/? I/DEBUG:     #04 pc 001739d2 
/data/dalvik-cache/x86/data@app@org.adblockplus.libadblockplus.android.webvie...

[sergei, 2017-03-16 08:48:08]

On 2017/03/07 20:45:51, anton wrote:
> On 2017/02/28 14:36:23, sergei wrote:
> 
> something is broken here:
> 
> 03-08 01:22:51.510 5960-5998/? D/libadblockplus: Getting pref for
> subscriptions_exceptionsurl
>                                                  
>                                                  --------- beginning of crash
> 03-08 01:22:51.510 5960-5998/? A/libc: Fatal signal 11 (SIGSEGV), code 2,
fault
> addr 0xa1f3e244 in tid 5998 (Thread-318)
> 03-08 01:22:51.613 1170-1170/? I/DEBUG: *** *** *** *** *** *** *** *** ***
***
> *** *** *** *** *** ***
> 03-08 01:22:51.613 1170-1170/? I/DEBUG: Build fingerprint:
>
'Android/sdk_google_phone_x86/generic_x86:5.1.1/LMY48X/3728910:userdebug/test-keys'
> 03-08 01:22:51.613 1170-1170/? I/DEBUG: Revision: '0'
> 03-08 01:22:51.613 1170-1170/? I/DEBUG: ABI: 'x86'
> 03-08 01:22:51.613 1170-1170/? I/DEBUG: pid: 5960, tid: 5998, name: Thread-318

> >>> org.adblockplus.libadblockplus.android.webviewapp <<<
> 03-08 01:22:51.613 1170-1170/? I/DEBUG: signal 11 (SIGSEGV), code 2
> (SEGV_ACCERR), fault addr 0xa1f3e244
> 03-08 01:22:51.620 1170-1170/? I/DEBUG:     eax f0e48156  ebx a24f2d30  ecx
> f0e48157  edx a1f3e240
> 03-08 01:22:51.620 1170-1170/? I/DEBUG:     esi a1f3e244  edi a24d88b8
> 03-08 01:22:51.620 1170-1170/? I/DEBUG:     xcs 00000073  xds 0000007b  xes
> 0000007b  xfs 000000af  xss 0000007b
> 03-08 01:22:51.620 1170-1170/? I/DEBUG:     eip a229e4fc  ebp a26fb738  esp
> a26fb734  flags 00210293
> 03-08 01:22:51.620 1170-1170/? I/DEBUG: backtrace:
> 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #00 pc 005204fc 
>
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
> (std::__1::__shared_weak_count::lock()+28)
> 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #01 pc 001bd609 
>
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
> (AdblockPlus::JsEngine::Evaluate(std::__1::basic_string<char,
> std::__1::char_traits<char>, std::__1::allocator<char> > const&,
> std::__1::basic_string<char, std::__1::char_traits<char>,
> std::__1::allocator<char> > const&)+57)
> 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #02 pc 001b8094 
>
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
> (AdblockPlus::FilterEngine::GetPref(std::__1::basic_string<char,
> std::__1::char_traits<char>, std::__1::allocator<char> > const&) const+132)
> 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #03 pc 001a6bfa 
>
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
> (JniGetPref(_JNIEnv*, _jclass*, long long, _jstring*)+138)
> 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #04 pc 001739d2 
>
mailto:/data/dalvik-cache/x86/data@app@org.adblockplus.libadblockplus.android.webviewapp-1@base.apk@classes.dex

It was a bug in android part.

[sergei, 2017-03-16 08:48:49]

Is there any chance to get it reviewed?

[anton, 2017-03-16 08:49:54]

On 2017/03/16 08:48:08, sergei wrote:
> On 2017/03/07 20:45:51, anton wrote:
> > On 2017/02/28 14:36:23, sergei wrote:
> > 
> > something is broken here:
> > 
> > 03-08 01:22:51.510 5960-5998/? D/libadblockplus: Getting pref for
> > subscriptions_exceptionsurl
> >                                                  
> >                                                  --------- beginning of
crash
> > 03-08 01:22:51.510 5960-5998/? A/libc: Fatal signal 11 (SIGSEGV), code 2,
> fault
> > addr 0xa1f3e244 in tid 5998 (Thread-318)
> > 03-08 01:22:51.613 1170-1170/? I/DEBUG: *** *** *** *** *** *** *** *** ***
> ***
> > *** *** *** *** *** ***
> > 03-08 01:22:51.613 1170-1170/? I/DEBUG: Build fingerprint:
> >
>
'Android/sdk_google_phone_x86/generic_x86:5.1.1/LMY48X/3728910:userdebug/test-keys'
> > 03-08 01:22:51.613 1170-1170/? I/DEBUG: Revision: '0'
> > 03-08 01:22:51.613 1170-1170/? I/DEBUG: ABI: 'x86'
> > 03-08 01:22:51.613 1170-1170/? I/DEBUG: pid: 5960, tid: 5998, name:
Thread-318
> 
> > >>> org.adblockplus.libadblockplus.android.webviewapp <<<
> > 03-08 01:22:51.613 1170-1170/? I/DEBUG: signal 11 (SIGSEGV), code 2
> > (SEGV_ACCERR), fault addr 0xa1f3e244
> > 03-08 01:22:51.620 1170-1170/? I/DEBUG:     eax f0e48156  ebx a24f2d30  ecx
> > f0e48157  edx a1f3e240
> > 03-08 01:22:51.620 1170-1170/? I/DEBUG:     esi a1f3e244  edi a24d88b8
> > 03-08 01:22:51.620 1170-1170/? I/DEBUG:     xcs 00000073  xds 0000007b  xes
> > 0000007b  xfs 000000af  xss 0000007b
> > 03-08 01:22:51.620 1170-1170/? I/DEBUG:     eip a229e4fc  ebp a26fb738  esp
> > a26fb734  flags 00210293
> > 03-08 01:22:51.620 1170-1170/? I/DEBUG: backtrace:
> > 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #00 pc 005204fc 
> >
>
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
> > (std::__1::__shared_weak_count::lock()+28)
> > 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #01 pc 001bd609 
> >
>
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
> > (AdblockPlus::JsEngine::Evaluate(std::__1::basic_string<char,
> > std::__1::char_traits<char>, std::__1::allocator<char> > const&,
> > std::__1::basic_string<char, std::__1::char_traits<char>,
> > std::__1::allocator<char> > const&)+57)
> > 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #02 pc 001b8094 
> >
>
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
> > (AdblockPlus::FilterEngine::GetPref(std::__1::basic_string<char,
> > std::__1::char_traits<char>, std::__1::allocator<char> > const&) const+132)
> > 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #03 pc 001a6bfa 
> >
>
/data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/x86/libadblockplus-jni.so
> > (JniGetPref(_JNIEnv*, _jclass*, long long, _jstring*)+138)
> > 03-08 01:22:51.621 1170-1170/? I/DEBUG:     #04 pc 001739d2 
> >
>
mailto:/data/dalvik-cache/x86/data@app@org.adblockplus.libadblockplus.android.webviewapp-1@base.apk@classes.dex
> 
> It was a bug in android part.

What bug do you mean?

[sergei, 2017-03-16 09:00:14]

On 2017/03/16 08:49:54, anton wrote:
....
> > 
> > It was a bug in android part.
> 
> What bug do you mean?

Usage of dangling pointer to already destroyed FilterEngine.

[anton, 2017-03-16 10:36:49]

On 2017/03/16 09:00:14, sergei wrote:
> On 2017/03/16 08:49:54, anton wrote:
> ....
> > > 
> > > It was a bug in android part.
> > 
> > What bug do you mean?
> 
> Usage of dangling pointer to already destroyed FilterEngine.

I've been thinking that #4964 was another reason and i've just checked it's not.
Everything is ok now.

[sergei, 2017-03-16 10:39:56]

On 2017/03/16 10:36:49, anton wrote:
> On 2017/03/16 09:00:14, sergei wrote:
> > On 2017/03/16 08:49:54, anton wrote:
> > ....
> > > > 
> > > > It was a bug in android part.
> > > 
> > > What bug do you mean?
> > 
> > Usage of dangling pointer to already destroyed FilterEngine.
> 
> I've been thinking that #4964 was another reason and i've just checked it's
not.
> Everything is ok now.

You mean #4694, it's also the issue, however the stack trace above is for
dangling pointer.

[anton, 2017-03-16 11:26:22]

On 2017/03/16 10:39:56, sergei wrote:
> On 2017/03/16 10:36:49, anton wrote:
> > On 2017/03/16 09:00:14, sergei wrote:
> > > On 2017/03/16 08:49:54, anton wrote:
> > > ....
> > > > > 
> > > > > It was a bug in android part.
> > > > 
> > > > What bug do you mean?
> > > 
> > > Usage of dangling pointer to already destroyed FilterEngine.
> > 
> > I've been thinking that #4964 was another reason and i've just checked it's
> not.
> > Everything is ok now.
> 
> You mean #4694, it's also the issue, however the stack trace above is for
> dangling pointer.

Yes, i've meant #4694. It's the issue but not related to this one so i was able
to test my part.

[Oleksandr, 2017-03-16 14:19:53]

Overall this approach is fine by me. Especially considering the aggressive
timeline.

https://codereview.adblockplus.org/29377570/diff/29377674/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29377570/diff/29377674/src/FilterEngine.cp...
src/FilterEngine.cpp:181: auto isConnectionAllowed = params.isConnectionAllowed;
The naming here is highly confusing, IMO. How about isConnectionAllowedCallback?

https://codereview.adblockplus.org/29377570/diff/29377674/src/FilterEngine.cp...
src/FilterEngine.cpp:448: if (value)
Nit: this doesn't look related, is it?

[sergei, 2017-03-16 16:25:22]

https://codereview.adblockplus.org/29377570/diff/29377674/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29377570/diff/29377674/src/FilterEngine.cp...
src/FilterEngine.cpp:181: auto isConnectionAllowed = params.isConnectionAllowed;
On 2017/03/16 14:19:53, Oleksandr wrote:
> The naming here is highly confusing, IMO. How about
isConnectionAllowedCallback?
> 

No objections, fixed.

https://codereview.adblockplus.org/29377570/diff/29377674/src/FilterEngine.cp...
src/FilterEngine.cpp:448: if (value)
On 2017/03/16 14:19:53, Oleksandr wrote:
> Nit: this doesn't look related, is it?

It's related, now we can call SetPref(nullptr) and if value is nullptr then it
crashes afterwards. nullptr here means there is no value, what is undefined in
terms of JavaScript, so we either should pass undefined or not pass the
argument. I have chosen the latter.

[Oleksandr, 2017-03-16 19:42:36]

LGTM

[sergei, 2017-03-17 14:40:11]

I'm going to push it today.