Issue 6265 - Create shared AdblockEngine instance in AdblockWebView in background

Issue 6265 - Create shared AdblockEngine instance in AdblockWebView in background

[anton, 2018-01-17 11:57:13]

Previously AdblockWebView accepted AdblockEngine as argument but creating
AdblockEngine is long-running operation. Now it has to be done in AdblockWebView
and in background. So we have to use AdblockHelper functionality. For this i
have to move most of AdblockHelper code to SingleInstanceEngineProvider. Since
it uses isAllowedConnection.. from settings this is moved to
libadblockplus-android too leading to minSdkVersion of libadblockplus is now 16
instead of 11.

Since we can compile with maven/ant i have to update pom.xml and AndroidManifest
(Gradle updates AndroidManifest.xml automatically but maven/ant does not). Since
i had to update AndroidManifest.xml i've fixed indentation too (though it's not
related to this ticket directly).

[jens, 2018-01-19 09:54:37]

On 2018/01/17 11:57:13, anton wrote:
> Previously AdblockWebView accepted AdblockEngine as argument but creating
> AdblockEngine is long-running operation. Now it has to be done in
AdblockWebView
> and in background. So we have to use AdblockHelper functionality. For this i
> have to move most of AdblockHelper code to SingleInstanceEngineProvider. Since
> it uses isAllowedConnection.. from settings this is moved to
> libadblockplus-android too leading to minSdkVersion of libadblockplus is now
16
> instead of 11.
> 
> Since we can compile with maven/ant i have to update pom.xml and
AndroidManifest
> (Gradle updates AndroidManifest.xml automatically but maven/ant does not).
Since
> i had to update AndroidManifest.xml i've fixed indentation too (though it's
not
> related to this ticket directly).

LGTM besides the little remark.
But a general question: Why don't we use the final modifier more often (for
classes, members and function parameters)?

[jens, 2018-01-19 09:54:44]

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:61:

Do wen need that empty line?

[anton, 2018-01-19 10:15:03]

On 2018/01/19 09:54:37, jens wrote:
> On 2018/01/17 11:57:13, anton wrote:
> > Previously AdblockWebView accepted AdblockEngine as argument but creating
> > AdblockEngine is long-running operation. Now it has to be done in
> AdblockWebView
> > and in background. So we have to use AdblockHelper functionality. For this i
> > have to move most of AdblockHelper code to SingleInstanceEngineProvider.
Since
> > it uses isAllowedConnection.. from settings this is moved to
> > libadblockplus-android too leading to minSdkVersion of libadblockplus is now
> 16
> > instead of 11.
> > 
> > Since we can compile with maven/ant i have to update pom.xml and
> AndroidManifest
> > (Gradle updates AndroidManifest.xml automatically but maven/ant does not).
> Since
> > i had to update AndroidManifest.xml i've fixed indentation too (though it's
> not
> > related to this ticket directly).
> 
> LGTM besides the little remark.
> But a general question: Why don't we use the final modifier more often (for
> classes, members and function parameters)?

I believe the answer is "code style was not set to be too strict from the very
beginning".
I think i should merge https://codereview.adblockplus.org/29567648/.

Diego, how do you think about setting "final" more often?

[anton, 2018-01-19 10:15:10]

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:61:

On 2018/01/19 09:54:44, jens wrote:
> Do wen need that empty line?

it's just [optional] separation between comments (i can remove it and put "." at
the end of 1st comment)

[jens, 2018-01-19 10:22:44]

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:61:

On 2018/01/19 10:15:10, anton wrote:
> On 2018/01/19 09:54:44, jens wrote:
> > Do wen need that empty line?
> 
> it's just [optional] separation between comments (i can remove it and put "."
at
> the end of 1st comment)

I would say that's up to you. I was just not sure if it was intentional.

[diegocarloslima, 2018-01-19 12:17:46]

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:51:
private Runnable engineCreatedCallback = new Runnable()
maybe declare it final?

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:61:

On 2018/01/19 10:22:43, jens wrote:
> On 2018/01/19 10:15:10, anton wrote:
> > On 2018/01/19 09:54:44, jens wrote:
> > > Do wen need that empty line?
> > 
> > it's just [optional] separation between comments (i can remove it and put
"."
> at
> > the end of 1st comment)
> 
> I would say that's up to you. I was just not sure if it was intentional.

for me it looks a bit weird this new line between 2 comments. I would prefer the
'.' option

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:81:
private Runnable engineDisposedCallback = new Runnable()
maybe declare it final?

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:166:
// The methods below are deprecated: use .getProvider().method() instead
This comment would be better inside the javadoc part of each method

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File libadblockplus-android-webview/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webview/AndroidManifest.xml:4: android:versionCode="3"
Is this shifting from 1 to 3 because it was inconsistent with build.gradle?

[anton, 2018-01-19 12:27:01]

uploaded new patch set

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:51:
private Runnable engineCreatedCallback = new Runnable()
On 2018/01/19 12:17:45, diegocarloslima wrote:
> maybe declare it final?

Acknowledged.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:61:

On 2018/01/19 12:17:46, diegocarloslima wrote:
> On 2018/01/19 10:22:43, jens wrote:
> > On 2018/01/19 10:15:10, anton wrote:
> > > On 2018/01/19 09:54:44, jens wrote:
> > > > Do wen need that empty line?
> > > 
> > > it's just [optional] separation between comments (i can remove it and put
> "."
> > at
> > > the end of 1st comment)
> > 
> > I would say that's up to you. I was just not sure if it was intentional.
> 
> for me it looks a bit weird this new line between 2 comments. I would prefer
the
> '.' option

Acknowledged.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:81:
private Runnable engineDisposedCallback = new Runnable()
On 2018/01/19 12:17:46, diegocarloslima wrote:
> maybe declare it final?

Acknowledged.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:166:
// The methods below are deprecated: use .getProvider().method() instead
On 2018/01/19 12:17:45, diegocarloslima wrote:
> This comment would be better inside the javadoc part of each method

Acknowledged.

[anton, 2018-01-19 12:36:47]

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File libadblockplus-android-webview/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webview/AndroidManifest.xml:4: android:versionCode="3"
On 2018/01/19 12:17:46, diegocarloslima wrote:
> Is this shifting from 1 to 3 because it was inconsistent with build.gradle?

Right

[diegocarloslima, 2018-01-19 12:59:19]

https://codereview.adblockplus.org/29671734/diff/29674608/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29671734/diff/29674608/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:166:
* The method is deprecated: use .getProvider().retain() instead
It would be also good to add @deprecated in the javadoc:
* @deprecated The method is deprecated: use .getProvider().retain() instead

https://codereview.adblockplus.org/29671734/diff/29674608/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:168:
@Deprecated public boolean retain(boolean asynchronous)
I would prefer this annotation as it was before, above the method declaration (I
don't think we have a specific code style for that, so we should stick with the
one used by Google):
@Deprecated
public boolean retain(boolean asynchronous)

[anton, 2018-01-19 13:05:35]

On 2018/01/19 12:59:19, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29671734/diff/29674608/libadblockplus-andr...
> File
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
> (right):
> 
>
https://codereview.adblockplus.org/29671734/diff/29674608/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:166:
> * The method is deprecated: use .getProvider().retain() instead
> It would be also good to add @deprecated in the javadoc:
> * @deprecated The method is deprecated: use .getProvider().retain() instead
> 
>
https://codereview.adblockplus.org/29671734/diff/29674608/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:168:
> @Deprecated public boolean retain(boolean asynchronous)
> I would prefer this annotation as it was before, above the method declaration
(I
> don't think we have a specific code style for that, so we should stick with
the
> one used by Google):
> @Deprecated
> public boolean retain(boolean asynchronous)

Done. See uploaded patch set

[sergei, 2018-01-19 13:56:42]

Few thoughts and BTW the changes in last patches are missing previous changes.

I have not checked the everything just wanted to share my opinion regarding some
places.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:239:
if (this.provider != null && provider != null && this.provider == provider)
So, if both this.provider and provider are null then there will NPE later. What
about just checking whether `this.provider == provider` and checking that it's
not null when it's accessed. It might be better to do it in another codereview.

Alternatively it could make sense to even prohibit to pass null here.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:968:
if (provider.getCounter() == 0)
Can it actually happen and if so, why it cannot happen few lines below?

I have not check the whole threading here but IMO it would be better to
implement it differently, of course in another issue.
As I understood ElemHideThread can be created several times for the same
instance of AdblockWebView (WebView) class, so instead of just overriding a
member (elemHideThread) what can be a cause of race conditions there should be
something a bit more advanced and the code of the ElemHide thread should not
care about existence of all requirements. How to do it?
When the thread is being started it should be counted somehow, depending what
you want later. Let's say it's added to a linked list, but perhaps in Java world
it can be just an incrementing of a counter. When the thread finishes its useful
work it should somehow (either by obtaining a lock or sending an asynchronous
task to the the "main" thread) remove itself from that linked list (linked list
is useful for performance because a thread can hold an iterator and use it as ID
to remove itself while other iterators are not invalided). In the dispose method
of our WebView instead of assigning a "post run" task to some thread we should:
switch to the state when no more ElemHide threads can be created, cancel running
threads (by setting of a WebView AtomicBoolean member to true, see below), then
wait for the finishing of a useful work of all currently running threads
(basically, wait until that linked list is empty or the counter value is zero)
and then dispose AdblockEngine, since there are no any users of it in some
thread anymore.
When the list of selectors is ready then we should schedule a task to the "main
thread" and perform the injection there if the ElemHideThread (or some other ID)
corresponds to the currently being viewed web site.
Canceling could be implemented as a callback function which is synchronously
checking whether the currently being viewed web site is still the same and that
a global cancelling is not triggered (the value of WebView AtomicBoolean
member).

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1087:
private void initProvider()
Would it be better to call it ensureAbpProvider()?

SingleInstanceEngineProvider has member `private AdblockEngine engine;`, so when
there are several instances of the WebView then each is using its own instance
of AdblockEngine with its own FilterEngine, I thought that the idea is that they
shared the same instance of AdblockEngine.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/SettingsActivity.java
(left):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/SettingsActivity.java:46:
AdblockHelper.get().retain(MainActivity.ADBLOCKENGINE_RETAIN_ASYNC);
I'm not sure that changing from ADBLOCKENGINE_RETAIN_ASYNC to Boolean true is
better.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java:27:
* @return if a new instance is allocated
Is it actually required to know that a new instance is allocated?

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java:39:
* @return AdblockEngine instance. Can be `null` is not yet retained or already
released
Typo in "Can be `null` is not yet retained"

[diegocarloslima, 2018-01-19 15:56:12]

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:239:
if (this.provider != null && provider != null && this.provider == provider)
On 2018/01/19 13:56:41, sergei wrote:
> So, if both this.provider and provider are null then there will NPE later.
What
> about just checking whether `this.provider == provider` and checking that it's
> not null when it's accessed. It might be better to do it in another
codereview.
> 
> Alternatively it could make sense to even prohibit to pass null here.

We can add the @NonNull annotation for AdblockEngineProvide param in order to
have a null check

[anton, 2018-01-22 06:17:38]

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:239:
if (this.provider != null && provider != null && this.provider == provider)
On 2018/01/19 13:56:41, sergei wrote:
> So, if both this.provider and provider are null then there will NPE later.
What
> about just checking whether `this.provider == provider` and checking that it's
> not null when it's accessed. It might be better to do it in another
codereview.
> 
> Alternatively it could make sense to even prohibit to pass null here.

I'd prefer to fix it by adding `null` check.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:968:
if (provider.getCounter() == 0)
On 2018/01/19 13:56:41, sergei wrote:
> Can it actually happen and if so, why it cannot happen few lines below?
> 
> I have not check the whole threading here but IMO it would be better to
> implement it differently, of course in another issue.
> As I understood ElemHideThread can be created several times for the same
> instance of AdblockWebView (WebView) class, so instead of just overriding a
> member (elemHideThread) what can be a cause of race conditions there should be
> something a bit more advanced and the code of the ElemHide thread should not
> care about existence of all requirements. How to do it?
> When the thread is being started it should be counted somehow, depending what
> you want later. Let's say it's added to a linked list, but perhaps in Java
world
> it can be just an incrementing of a counter. When the thread finishes its
useful
> work it should somehow (either by obtaining a lock or sending an asynchronous
> task to the the "main" thread) remove itself from that linked list (linked
list
> is useful for performance because a thread can hold an iterator and use it as
ID
> to remove itself while other iterators are not invalided). In the dispose
method
> of our WebView instead of assigning a "post run" task to some thread we
should:
> switch to the state when no more ElemHide threads can be created, cancel
running
> threads (by setting of a WebView AtomicBoolean member to true, see below),
then
> wait for the finishing of a useful work of all currently running threads
> (basically, wait until that linked list is empty or the counter value is zero)
> and then dispose AdblockEngine, since there are no any users of it in some
> thread anymore.
> When the list of selectors is ready then we should schedule a task to the
"main
> thread" and perform the injection there if the ElemHideThread (or some other
ID)
> corresponds to the currently being viewed web site.
> Canceling could be implemented as a callback function which is synchronously
> checking whether the currently being viewed web site is still the same and
that
> a global cancelling is not triggered (the value of WebView AtomicBoolean
> member).

I've just reviewed the code and it seems that it can happen in *very* rare case:
1) elemhide thread #1 is created, but not started
2) URL #2 set, started loading. Elemhide thread #2 is created, but not started
3) dispose called
4) thread #2 started, finishRunnable set, filter engine actually disposed, ref
counter = 0
5) thread #1 started (check is required)

Note thread starting takes time and the order is not guaranteed.
We can refactor multithreading here is separate thread (please create separate
ticket)
but i'm absolutely sure existing check does not hurt.

In fact it happened few times when website forwards to "mobile" or
"per-language" version of webpage
so changing of URL happens quickly and elemhiding for URL #1 is not finished.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1087:
private void initProvider()
On 2018/01/19 13:56:41, sergei wrote:
> Would it be better to call it ensureAbpProvider()?
> 
> SingleInstanceEngineProvider has member `private AdblockEngine engine;`, so
when
> there are several instances of the WebView then each is using its own instance
> of AdblockEngine with its own FilterEngine, I thought that the idea is that
they
> shared the same instance of AdblockEngine.

Acknowledged.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/SettingsActivity.java
(left):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/SettingsActivity.java:46:
AdblockHelper.get().retain(MainActivity.ADBLOCKENGINE_RETAIN_ASYNC);
On 2018/01/19 13:56:41, sergei wrote:
> I'm not sure that changing from ADBLOCKENGINE_RETAIN_ASYNC to Boolean true is
> better.

What do you mean "better"?
Previously we had this flag to adjust is it should be done async or synch. Since
now it's up to  AdblockWebView and it does it in async way (no choice) we don't
have this setting.

We could invoke it sync here or async + waitForReady - does not make sense (it
just affects the code line when it's locked).
In practice since we create AdblockEngine in MainActivity we don't create new
instance of FilterEngine but just inc/dec the counter.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java
(right):

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java:27:
* @return if a new instance is allocated
On 2018/01/19 13:56:41, sergei wrote:
> Is it actually required to know that a new instance is allocated?

Yes, in AdblockChromium we need to know it to send new raw instance address.

https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java:39:
* @return AdblockEngine instance. Can be `null` is not yet retained or already
released
On 2018/01/19 13:56:41, sergei wrote:
> Typo in "Can be `null` is not yet retained"

Acknowledged.

[anton, 2018-01-22 07:00:20]

On 2018/01/22 06:17:38, anton wrote:
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> File
>
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
> (right):
> 
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
>
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:239:
> if (this.provider != null && provider != null && this.provider == provider)
> On 2018/01/19 13:56:41, sergei wrote:
> > So, if both this.provider and provider are null then there will NPE later.
> What
> > about just checking whether `this.provider == provider` and checking that
it's
> > not null when it's accessed. It might be better to do it in another
> codereview.
> > 
> > Alternatively it could make sense to even prohibit to pass null here.
> 
> I'd prefer to fix it by adding `null` check.
> 
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
>
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:968:
> if (provider.getCounter() == 0)
> On 2018/01/19 13:56:41, sergei wrote:
> > Can it actually happen and if so, why it cannot happen few lines below?
> > 
> > I have not check the whole threading here but IMO it would be better to
> > implement it differently, of course in another issue.
> > As I understood ElemHideThread can be created several times for the same
> > instance of AdblockWebView (WebView) class, so instead of just overriding a
> > member (elemHideThread) what can be a cause of race conditions there should
be
> > something a bit more advanced and the code of the ElemHide thread should not
> > care about existence of all requirements. How to do it?
> > When the thread is being started it should be counted somehow, depending
what
> > you want later. Let's say it's added to a linked list, but perhaps in Java
> world
> > it can be just an incrementing of a counter. When the thread finishes its
> useful
> > work it should somehow (either by obtaining a lock or sending an
asynchronous
> > task to the the "main" thread) remove itself from that linked list (linked
> list
> > is useful for performance because a thread can hold an iterator and use it
as
> ID
> > to remove itself while other iterators are not invalided). In the dispose
> method
> > of our WebView instead of assigning a "post run" task to some thread we
> should:
> > switch to the state when no more ElemHide threads can be created, cancel
> running
> > threads (by setting of a WebView AtomicBoolean member to true, see below),
> then
> > wait for the finishing of a useful work of all currently running threads
> > (basically, wait until that linked list is empty or the counter value is
zero)
> > and then dispose AdblockEngine, since there are no any users of it in some
> > thread anymore.
> > When the list of selectors is ready then we should schedule a task to the
> "main
> > thread" and perform the injection there if the ElemHideThread (or some other
> ID)
> > corresponds to the currently being viewed web site.
> > Canceling could be implemented as a callback function which is synchronously
> > checking whether the currently being viewed web site is still the same and
> that
> > a global cancelling is not triggered (the value of WebView AtomicBoolean
> > member).
> 
> I've just reviewed the code and it seems that it can happen in *very* rare
case:
> 1) elemhide thread #1 is created, but not started
> 2) URL #2 set, started loading. Elemhide thread #2 is created, but not started
> 3) dispose called
> 4) thread #2 started, finishRunnable set, filter engine actually disposed, ref
> counter = 0
> 5) thread #1 started (check is required)
> 
> Note thread starting takes time and the order is not guaranteed.
> We can refactor multithreading here is separate thread (please create separate
> ticket)
> but i'm absolutely sure existing check does not hurt.
> 
> In fact it happened few times when website forwards to "mobile" or
> "per-language" version of webpage
> so changing of URL happens quickly and elemhiding for URL #1 is not finished.
> 
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
>
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1087:
> private void initProvider()
> On 2018/01/19 13:56:41, sergei wrote:
> > Would it be better to call it ensureAbpProvider()?
> > 
> > SingleInstanceEngineProvider has member `private AdblockEngine engine;`, so
> when
> > there are several instances of the WebView then each is using its own
instance
> > of AdblockEngine with its own FilterEngine, I thought that the idea is that
> they
> > shared the same instance of AdblockEngine.
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> File
>
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/SettingsActivity.java
> (left):
> 
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
>
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/SettingsActivity.java:46:
> AdblockHelper.get().retain(MainActivity.ADBLOCKENGINE_RETAIN_ASYNC);
> On 2018/01/19 13:56:41, sergei wrote:
> > I'm not sure that changing from ADBLOCKENGINE_RETAIN_ASYNC to Boolean true
is
> > better.
> 
> What do you mean "better"?
> Previously we had this flag to adjust is it should be done async or synch.
Since
> now it's up to  AdblockWebView and it does it in async way (no choice) we
don't
> have this setting.
> 
> We could invoke it sync here or async + waitForReady - does not make sense (it
> just affects the code line when it's locked).
> In practice since we create AdblockEngine in MainActivity we don't create new
> instance of FilterEngine but just inc/dec the counter.
> 
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> File
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java
> (right):
> 
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java:27:
> * @return if a new instance is allocated
> On 2018/01/19 13:56:41, sergei wrote:
> > Is it actually required to know that a new instance is allocated?
> 
> Yes, in AdblockChromium we need to know it to send new raw instance address.
> 
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java:39:
> * @return AdblockEngine instance. Can be `null` is not yet retained or already
> released
> On 2018/01/19 13:56:41, sergei wrote:
> > Typo in "Can be `null` is not yet retained"
> 
> Acknowledged.

Uploaded new patch set

[diegocarloslima, 2018-01-22 11:35:52]

On 2018/01/22 07:00:20, anton wrote:
> On 2018/01/22 06:17:38, anton wrote:
> >
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> > File
> >
>
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> >
>
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:239:
> > if (this.provider != null && provider != null && this.provider == provider)
> > On 2018/01/19 13:56:41, sergei wrote:
> > > So, if both this.provider and provider are null then there will NPE later.
> > What
> > > about just checking whether `this.provider == provider` and checking that
> it's
> > > not null when it's accessed. It might be better to do it in another
> > codereview.
> > > 
> > > Alternatively it could make sense to even prohibit to pass null here.
> > 
> > I'd prefer to fix it by adding `null` check.
> > 
> >
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> >
>
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:968:
> > if (provider.getCounter() == 0)
> > On 2018/01/19 13:56:41, sergei wrote:
> > > Can it actually happen and if so, why it cannot happen few lines below?
> > > 
> > > I have not check the whole threading here but IMO it would be better to
> > > implement it differently, of course in another issue.
> > > As I understood ElemHideThread can be created several times for the same
> > > instance of AdblockWebView (WebView) class, so instead of just overriding
a
> > > member (elemHideThread) what can be a cause of race conditions there
should
> be
> > > something a bit more advanced and the code of the ElemHide thread should
not
> > > care about existence of all requirements. How to do it?
> > > When the thread is being started it should be counted somehow, depending
> what
> > > you want later. Let's say it's added to a linked list, but perhaps in Java
> > world
> > > it can be just an incrementing of a counter. When the thread finishes its
> > useful
> > > work it should somehow (either by obtaining a lock or sending an
> asynchronous
> > > task to the the "main" thread) remove itself from that linked list (linked
> > list
> > > is useful for performance because a thread can hold an iterator and use it
> as
> > ID
> > > to remove itself while other iterators are not invalided). In the dispose
> > method
> > > of our WebView instead of assigning a "post run" task to some thread we
> > should:
> > > switch to the state when no more ElemHide threads can be created, cancel
> > running
> > > threads (by setting of a WebView AtomicBoolean member to true, see below),
> > then
> > > wait for the finishing of a useful work of all currently running threads
> > > (basically, wait until that linked list is empty or the counter value is
> zero)
> > > and then dispose AdblockEngine, since there are no any users of it in some
> > > thread anymore.
> > > When the list of selectors is ready then we should schedule a task to the
> > "main
> > > thread" and perform the injection there if the ElemHideThread (or some
other
> > ID)
> > > corresponds to the currently being viewed web site.
> > > Canceling could be implemented as a callback function which is
synchronously
> > > checking whether the currently being viewed web site is still the same and
> > that
> > > a global cancelling is not triggered (the value of WebView AtomicBoolean
> > > member).
> > 
> > I've just reviewed the code and it seems that it can happen in *very* rare
> case:
> > 1) elemhide thread #1 is created, but not started
> > 2) URL #2 set, started loading. Elemhide thread #2 is created, but not
started
> > 3) dispose called
> > 4) thread #2 started, finishRunnable set, filter engine actually disposed,
ref
> > counter = 0
> > 5) thread #1 started (check is required)
> > 
> > Note thread starting takes time and the order is not guaranteed.
> > We can refactor multithreading here is separate thread (please create
separate
> > ticket)
> > but i'm absolutely sure existing check does not hurt.
> > 
> > In fact it happened few times when website forwards to "mobile" or
> > "per-language" version of webpage
> > so changing of URL happens quickly and elemhiding for URL #1 is not
finished.
> > 
> >
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> >
>
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1087:
> > private void initProvider()
> > On 2018/01/19 13:56:41, sergei wrote:
> > > Would it be better to call it ensureAbpProvider()?
> > > 
> > > SingleInstanceEngineProvider has member `private AdblockEngine engine;`,
so
> > when
> > > there are several instances of the WebView then each is using its own
> instance
> > > of AdblockEngine with its own FilterEngine, I thought that the idea is
that
> > they
> > > shared the same instance of AdblockEngine.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> > File
> >
>
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/SettingsActivity.java
> > (left):
> > 
> >
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> >
>
libadblockplus-android-webviewapp/src/org/adblockplus/libadblockplus/android/webviewapp/SettingsActivity.java:46:
> > AdblockHelper.get().retain(MainActivity.ADBLOCKENGINE_RETAIN_ASYNC);
> > On 2018/01/19 13:56:41, sergei wrote:
> > > I'm not sure that changing from ADBLOCKENGINE_RETAIN_ASYNC to Boolean true
> is
> > > better.
> > 
> > What do you mean "better"?
> > Previously we had this flag to adjust is it should be done async or synch.
> Since
> > now it's up to  AdblockWebView and it does it in async way (no choice) we
> don't
> > have this setting.
> > 
> > We could invoke it sync here or async + waitForReady - does not make sense
(it
> > just affects the code line when it's locked).
> > In practice since we create AdblockEngine in MainActivity we don't create
new
> > instance of FilterEngine but just inc/dec the counter.
> > 
> >
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> > File
> >
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> >
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java:27:
> > * @return if a new instance is allocated
> > On 2018/01/19 13:56:41, sergei wrote:
> > > Is it actually required to know that a new instance is allocated?
> > 
> > Yes, in AdblockChromium we need to know it to send new raw instance address.
> > 
> >
>
https://codereview.adblockplus.org/29671734/diff/29671735/libadblockplus-andr...
> >
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngineProvider.java:39:
> > * @return AdblockEngine instance. Can be `null` is not yet retained or
already
> > released
> > On 2018/01/19 13:56:41, sergei wrote:
> > > Typo in "Can be `null` is not yet retained"
> > 
> > Acknowledged.
> 
> Uploaded new patch set

LGTM