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 30 matching lines @@
 import android.webkit.WebChromeClient;
 import android.webkit.WebResourceRequest;  // makes android min version to be 21
 import android.webkit.WebResourceResponse;
 import android.webkit.WebStorage;
 import android.webkit.WebView;
 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;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.regex.Pattern;
@@ skipping 27 matching lines @@
 
   private static final Pattern RE_JS = Pattern.compile("\\.js$", Pattern.CASE_INSENSITIVE);
   private static final Pattern RE_CSS = Pattern.compile("\\.css$", Pattern.CASE_INSENSITIVE);
   private static final Pattern RE_IMAGE = Pattern.compile("\\.(?:gif|png|jpe?g|bmp|ico)$", Pattern.CASE_INSENSITIVE);
   private static final Pattern RE_FONT = Pattern.compile("\\.(?:ttf|woff)$", Pattern.CASE_INSENSITIVE);
   private static final Pattern RE_HTML = Pattern.compile("\\.html?$", Pattern.CASE_INSENSITIVE);
 
   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;
   private WebViewClient extWebViewClient;
   private WebViewClient intWebViewClient;
   private Map<String, String> url2Referrer = Collections.synchronizedMap(new HashMap<String, String>());
   private String url;
   private String domain;
   private String injectJs;
   private CountDownLatch elemHideLatch;
@@ skipping 67 matching lines @@
     applyAdblockEnabled();
   }
 
   public boolean isDebugMode()
   {
     return debugMode;
   }
 
   /**
    * 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)
   {
     this.debugMode = debugMode;
   }
 
   private void d(String message)
   {
     if (debugMode)
@@ skipping 28 matching lines @@
       .replace(DEBUG_TOKEN, (debugMode ? "" : "//"));
   }
 
   private void runScript(String script)
   {
     d("runScript started");
     evaluateJavascript(script, null);
     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;
     }
 
     final Runnable setRunnable = new Runnable()
     {
       @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);
     }
     else
     {
       setRunnable.run();
     }
   }
 
@@ skipping 545 matching lines @@
       {
         super.onReceivedLoginRequest(view, realm, account, args);
       }
     }
 
     protected WebResourceResponse shouldInterceptRequest(
       WebView webview, String url, boolean isMainFrame,
       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");
 
         // allow loading by returning null
         return null;
       }
 
       if (isMainFrame)
       {
         // never blocking main frame requests, just subrequests
         w(url + " is main frame, allow loading");
 
         // allow loading by returning null
         return null;
       }
 
       // whitelisted
-      if (adblockEngine.isDomainWhitelisted(url, referrerChainArray))
+      if (provider.getEngine().isDomainWhitelisted(url, referrerChainArray))
       {
         w(url + " domain is whitelisted, allow loading");
 
         // allow loading by returning null
         return null;
       }
 
-      if (adblockEngine.isDocumentWhitelisted(url, referrerChainArray))
+      if (provider.getEngine().isDocumentWhitelisted(url, referrerChainArray))
       {
         w(url + " document is whitelisted, allow loading");
 
         // allow loading by returning null
         return null;
       }
 
       // determine the content
       FilterEngine.ContentType contentType;
       if (isXmlHttpRequest)
@@ skipping 22 matching lines @@
         {
           contentType = FilterEngine.ContentType.SUBDOCUMENT;
         }
         else
         {
           contentType = FilterEngine.ContentType.OTHER;
         }
       }
 
       // check if we should block
-      if (adblockEngine.matches(url, contentType, referrerChainArray))
+      if (provider.getEngine().matches(url, contentType, referrerChainArray))
       {
         w("Blocked loading " + url);
 
         // if we should block, return empty response which results in 'errorLoading' callback
         return new WebResourceResponse("text/plain", "UTF-8", null);
       }
 
       d("Allowed loading " + url);
 
       // continue by returning null
@@ skipping 41 matching lines @@
     addJavascriptInterface(this, BRIDGE);
     initClients();
   }
 
   private void initClients()
   {
     intWebViewClient = new AdblockWebViewClient();
     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;
     private CountDownLatch finishedLatch;
     private AtomicBoolean isCancelled;
 
     public ElemHideThread(CountDownLatch finishedLatch)
     {
       this.finishedLatch = finishedLatch;
       isCancelled = new AtomicBoolean(false);
     }
 
     @Override
     public void run()
     {
       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;
         }
         else
         {
           String[] referrers = new String[]
             {
               url
             };
 
-          List<Subscription> subscriptions = adblockEngine.getFilterEngine().getListedSubscriptions();
+          List<Subscription> subscriptions = provider
+            .getEngine()
+            .getFilterEngine()
+            .getListedSubscriptions();
+
           try
           {
             d("Listed subscriptions: " + subscriptions.size());
             if (debugMode)
             {
               for (Subscription eachSubscription : subscriptions)
               {
                 d("Subscribed to "
                   + (eachSubscription.isDisabled() ? "disabled" : "enabled")
                   + " " + eachSubscription);
               }
             }
           }
           finally
           {
             for (Subscription eachSubscription : subscriptions)
             {
               eachSubscription.dispose();
             }
           }
 
           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);
         }
       }
       finally
       {
         if (!isCancelled.get())
         {
           finish(selectorsString);
         }
@@ skipping 53 matching lines @@
         w("elemHideThread set to null");
         elemHideThread = null;
       }
     }
   };
 
   private void initAbpLoading()
   {
     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));
     }
   }
 
   private void startAbpLoading(String newUrl)
   {
     d("Start loading " + newUrl);
 
     loading = true;
     addDomListener = true;
     elementsHidden = false;
     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);
         }
 
         d("Extracted domain " + domain + " from " + url);
       }
       catch (Throwable t)
       {
         e("Failed to extract domain from " + url, t);
@@ skipping 242 matching lines @@
       {
         w("Interrupted, returning empty selectors list");
         return EMPTY_ELEMHIDE_ARRAY_STRING;
       }
     }
   }
 
   private void doDispose()
   {
     w("Disposing AdblockEngine");
-    adblockEngine.dispose();
-    adblockEngine = null;
-
-    disposeEngine = false;
+    provider.release();
   }
 
   private class DisposeRunnable implements Runnable
   {
     private Runnable disposeFinished;
 
     private DisposeRunnable(Runnable disposeFinished)
     {
       this.disposeFinished = disposeFinished;
     }
 
     @Override
     public void run()
     {
-      if (disposeEngine)
-      {
-        doDispose();
-      }
+      doDispose();
 
       if (disposeFinished != null)
       {
         disposeFinished.run();
       }
     }
   }
 
   /**
    * Dispose AdblockWebView and internal adblockEngine if it was created
    * If external AdblockEngine was passed using `setAdblockEngine()` it should be disposed explicitly
    * Warning: runnable can be invoked from background thread
    * @param disposeFinished runnable to run when AdblockWebView is disposed
    */
   public void dispose(final Runnable disposeFinished)
   {
     d("Dispose invoked");
 
+    if (provider == null)
+    {
+      d("No internal AdblockEngineProvider created");
+      return;
+    }
+
     stopLoading();
 
-    removeJavascriptInterface(BRIDGE);
-    if (!disposeEngine)
-    {
-      adblockEngine = null;
-    }
-
     DisposeRunnable disposeRunnable = new DisposeRunnable(disposeFinished);
     synchronized (elemHideThreadLockObject)
     {
       if (elemHideThread != null)
       {
         w("Busy with elemhide selectors, delayed disposing scheduled");
         elemHideThread.setFinishedRunnable(disposeRunnable);
       }
       else
       {
         disposeRunnable.run();
       }
     }
   }
 }