Issue 6265 - Create shared AdblockEngine instance in AdblockWebView in background

libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 218 matching lines @@
 
   private void runScript(String script)
   {
     d("runScript started");
     evaluateJavascript(script, null);
     d("runScript finished");
   }
 
   public void setProvider(final AdblockEngineProvider provider)
   {
     if (this.provider != null && provider != null && this.provider == provider)

[sergei, 2018/01/19 13:56:41]

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.

[diegocarloslima, 2018/01/19 15:56:11]

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

[anton, 2018/01/22 06:17:36]

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

     {
       return;
     }
 
     final Runnable setRunnable = new Runnable()
     {
       @Override
       public void run()
       {
         AdblockWebView.this.provider = provider;
-        AdblockWebView.this.provider.retain(true); // asynchronously
+        if (AdblockWebView.this.provider != null)
+        {
+          AdblockWebView.this.provider.retain(true); // asynchronously
+        }
       }
     };
 
     if (this.provider != null)
     {
       // as adblockEngine can be busy with elemhide thread we need to use callback
       this.dispose(setRunnable);
     }
     else
     {
@@ skipping 697 matching lines @@
     {
       this.finishedLatch = finishedLatch;
       isCancelled = new AtomicBoolean(false);
     }
 
     @Override
     public void run()
     {
       try
       {
         if (provider.getCounter() == 0)

[sergei, 2018/01/19 13:56:41]

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).

[anton, 2018/01/22 06:17:36]

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.

         {
           w("FilterEngine already disposed");
           selectorsString = EMPTY_ELEMHIDE_ARRAY_STRING;
         }
         else
         {
           String[] referrers = new String[]
             {
               url
             };
@@ skipping 95 matching lines @@
         w("elemHideThread set to null");
         elemHideThread = null;
       }
     }
   };
 
   private void initAbpLoading()
   {
     getSettings().setJavaScriptEnabled(true);
     buildInjectJs();
-    initProvider();
-  }
-
-  private void initProvider()

[sergei, 2018/01/19 13:56:41]

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.

[anton, 2018/01/22 06:17:36]

Acknowledged.

+    ensureProvider();
+  }
+
+  private void ensureProvider()
   {
     // if AdblockWebView works as drop-in replacement for WebView 'provider' is not set.
     // Thus AdblockWebView is using SingleInstanceEngineProvider instance
     if (provider == null)
     {
       setProvider(new SingleInstanceEngineProvider(
         getContext(), AdblockEngine.BASE_PATH_DIRECTORY, debugMode));
     }
   }
 
@@ skipping 327 matching lines @@
         w("Busy with elemhide selectors, delayed disposing scheduled");
         elemHideThread.setFinishedRunnable(disposeRunnable);
       }
       else
       {
         disposeRunnable.run();
       }
     }
   }
 }