Issue 6265 - Create shared AdblockEngine instance in AdblockWebView in background

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

Index: libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
diff --git a/libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java b/libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
index 5536696065bf2e3a6310dd9fa0ecfebede4a857a..b3fc893057fc13e48376b0a7a78101156ff4e586 100644
--- a/libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
+++ b/libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
@@ -48,6 +48,8 @@ import android.webkit.WebViewClient;
 import org.adblockplus.libadblockplus.FilterEngine;
 import org.adblockplus.libadblockplus.Subscription;
 import org.adblockplus.libadblockplus.android.AdblockEngine;
+import org.adblockplus.libadblockplus.android.AdblockEngineProvider;
+import org.adblockplus.libadblockplus.android.SingleInstanceEngineProvider;
 import org.adblockplus.libadblockplus.android.Utils;
 
 import java.io.IOException;
@@ -95,8 +97,7 @@ public class AdblockWebView extends WebView
   private volatile boolean addDomListener = true;
   private boolean adblockEnabled = true;
   private boolean debugMode;
-  private AdblockEngine adblockEngine;
-  private boolean disposeEngine;
+  private AdblockEngineProvider provider;
   private Integer loadError;
   private int allowDrawDelay = ALLOW_DRAW_DELAY;
   private WebChromeClient extWebChromeClient;
@@ -184,6 +185,7 @@ public class AdblockWebView extends WebView
 
   /**
    * Set to true to see debug log output int AdblockWebView and JS console
+   * Should be set before first URL loading if using internal AdblockEngineProvider
    * @param debugMode is debug mode
    */
   public void setDebugMode(boolean debugMode)
@@ -232,19 +234,9 @@ public class AdblockWebView extends WebView
     d("runScript finished");
   }
 
-  public AdblockEngine getAdblockEngine()
+  public void setProvider(final AdblockEngineProvider provider)
   {
-    return adblockEngine;
-  }
-
-  /**
-   * Set external adblockEngine. A new (internal) is created automatically if not set
-   * Don't forget to invoke {@link #dispose(Runnable)} later and dispose external adblockEngine
-   * @param adblockEngine external adblockEngine
-   */
-  public void setAdblockEngine(final AdblockEngine adblockEngine)
-  {
-    if (this.adblockEngine != null && adblockEngine != null && this.adblockEngine == adblockEngine)
+    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;
     }
@@ -254,12 +246,12 @@ public class AdblockWebView extends WebView
       @Override
       public void run()
       {
-        AdblockWebView.this.adblockEngine = adblockEngine;
-        AdblockWebView.this.disposeEngine = false;
+        AdblockWebView.this.provider = provider;
+        AdblockWebView.this.provider.retain(true); // asynchronously
       }
     };
 
-    if (this.adblockEngine != null && disposeEngine)
+    if (this.provider != null)
     {
       // as adblockEngine can be busy with elemhide thread we need to use callback
       this.dispose(setRunnable);
@@ -825,7 +817,7 @@ public class AdblockWebView extends WebView
       boolean isXmlHttpRequest, String[] referrerChainArray)
     {
       // if dispose() was invoke, but the page is still loading then just let it go
-      if (adblockEngine == null)
+      if (provider.getCounter() == 0)
       {
         e("FilterEngine already disposed, allow loading");
 
@@ -843,7 +835,7 @@ public class AdblockWebView extends WebView
       }
 
       // whitelisted
-      if (adblockEngine.isDomainWhitelisted(url, referrerChainArray))
+      if (provider.getEngine().isDomainWhitelisted(url, referrerChainArray))
       {
         w(url + " domain is whitelisted, allow loading");
 
@@ -851,7 +843,7 @@ public class AdblockWebView extends WebView
         return null;
       }
 
-      if (adblockEngine.isDocumentWhitelisted(url, referrerChainArray))
+      if (provider.getEngine().isDocumentWhitelisted(url, referrerChainArray))
       {
         w(url + " document is whitelisted, allow loading");
 
@@ -894,7 +886,7 @@ public class AdblockWebView extends WebView
       }
 
       // check if we should block
-      if (adblockEngine.matches(url, contentType, referrerChainArray))
+      if (provider.getEngine().matches(url, contentType, referrerChainArray))
       {
         w("Blocked loading " + url);
 
@@ -956,19 +948,6 @@ public class AdblockWebView extends WebView
     applyAdblockEnabled();
   }
 
-  private void createAdblockEngine()
-  {
-    w("Creating AdblockEngine");
-
-    // assuming `this.debugMode` can be used as `developmentBuild` value
-    adblockEngine = AdblockEngine
-      .builder(
-        AdblockEngine.generateAppInfo(this.getContext(), debugMode),
-        this.getContext().getDir(AdblockEngine.BASE_PATH_DIRECTORY, Context.MODE_PRIVATE).getAbsolutePath())
-      .enableElementHiding(true)
-      .build();
-  }
-
   private class ElemHideThread extends Thread
   {
     private String selectorsString;
@@ -986,7 +965,7 @@ public class AdblockWebView extends WebView
     {
       try
       {
-        if (adblockEngine == null)
+        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;
@@ -998,7 +977,11 @@ public class AdblockWebView extends WebView
               url
             };
 
-          List<Subscription> subscriptions = adblockEngine.getFilterEngine().getListedSubscriptions();
+          List<Subscription> subscriptions = provider
+            .getEngine()
+            .getFilterEngine()
+            .getListedSubscriptions();
+
           try
           {
             d("Listed subscriptions: " + subscriptions.size());
@@ -1021,7 +1004,10 @@ public class AdblockWebView extends WebView
           }
 
           d("Requesting elemhide selectors from AdblockEngine for " + url + " in " + this);
-          List<String> selectors = adblockEngine.getElementHidingSelectors(url, domain, referrers);
+          List<String> selectors = provider
+            .getEngine()
+            .getElementHidingSelectors(url, domain, referrers);
+
           d("Finished requesting elemhide selectors, got " + selectors.size() + " in " + this);
           selectorsString = Utils.stringListToJsonArray(selectors);
         }
@@ -1095,11 +1081,17 @@ public class AdblockWebView extends WebView
   {
     getSettings().setJavaScriptEnabled(true);
     buildInjectJs();
+    initProvider();
+  }
 
-    if (adblockEngine == null)
+  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.

+  {
+    // if AdblockWebView works as drop-in replacement for WebView 'provider' is not set.
+    // Thus AdblockWebView is using SingleInstanceEngineProvider instance
+    if (provider == null)
     {
-      createAdblockEngine();
-      disposeEngine = true;
+      setProvider(new SingleInstanceEngineProvider(
+        getContext(), AdblockEngine.BASE_PATH_DIRECTORY, debugMode));
     }
   }
 
@@ -1113,11 +1105,14 @@ public class AdblockWebView extends WebView
     loadError = null;
     url = newUrl;
 
-    if (url != null && adblockEngine != null)
+    if (url != null)
     {
       try
       {
-        domain = adblockEngine.getFilterEngine().getHostFromURL(url);
+        d("Waiting for adblock engine");
+        provider.waitForReady();
+
+        domain = provider.getEngine().getFilterEngine().getHostFromURL(url);
         if (domain == null)
         {
           throw new RuntimeException("Failed to extract domain from " + url);
@@ -1380,10 +1375,7 @@ public class AdblockWebView extends WebView
   private void doDispose()
   {
     w("Disposing AdblockEngine");
-    adblockEngine.dispose();
-    adblockEngine = null;
-
-    disposeEngine = false;
+    provider.release();
   }
 
   private class DisposeRunnable implements Runnable
@@ -1398,10 +1390,7 @@ public class AdblockWebView extends WebView
     @Override
     public void run()
     {
-      if (disposeEngine)
-      {
-        doDispose();
-      }
+      doDispose();
 
       if (disposeFinished != null)
       {
@@ -1420,14 +1409,14 @@ public class AdblockWebView extends WebView
   {
     d("Dispose invoked");
 
-    stopLoading();
-
-    removeJavascriptInterface(BRIDGE);
-    if (!disposeEngine)
+    if (provider == null)
     {
-      adblockEngine = null;
+      d("No internal AdblockEngineProvider created");
+      return;
     }
 
+    stopLoading();
+
     DisposeRunnable disposeRunnable = new DisposeRunnable(disposeFinished);
     synchronized (elemHideThreadLockObject)
     {