Issue 5010 - Allow to preload subscription files

libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 package org.adblockplus.libadblockplus.android;
 
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Set;
 
 import org.adblockplus.libadblockplus.AppInfo;
 import org.adblockplus.libadblockplus.Filter;
 import org.adblockplus.libadblockplus.FilterChangeCallback;
 import org.adblockplus.libadblockplus.FilterEngine;
 import org.adblockplus.libadblockplus.FilterEngine.ContentType;
 import org.adblockplus.libadblockplus.JsEngine;
 import org.adblockplus.libadblockplus.LogSystem;
 import org.adblockplus.libadblockplus.ShowNotificationCallback;
 import org.adblockplus.libadblockplus.Subscription;
 import org.adblockplus.libadblockplus.UpdateAvailableCallback;
 import org.adblockplus.libadblockplus.UpdateCheckDoneCallback;
+import org.adblockplus.libadblockplus.WebRequest;
 
 import android.content.Context;
 import android.content.pm.PackageInfo;
 import android.content.pm.PackageManager.NameNotFoundException;
 import android.os.Build.VERSION;
+import android.os.Handler;
+import android.os.Looper;
 import android.util.Log;
 
 public final class AdblockEngine
 {
   private static final String TAG = Utils.getTag(AdblockEngine.class);
 
   /*
    * The fields below are volatile because:
    *
    * I encountered JNI related bugs/crashes caused by JNI backed Java objects. It seemed that under
    * certain conditions the objects were optimized away which resulted in crashes when trying to
    * release the object, sometimes even on access.
    *
    * The only solution that really worked was to declare the variables holding the references
    * volatile, this seems to prevent the JNI from 'optimizing away' those objects (as a volatile
    * variable might be changed at any time from any thread).
    */
   private volatile JsEngine jsEngine;
   private volatile FilterEngine filterEngine;
   private volatile LogSystem logSystem;
-  private volatile AndroidWebRequest webRequest;
+  private volatile WebRequest webRequest;
   private volatile UpdateAvailableCallback updateAvailableCallback;
   private volatile UpdateCheckDoneCallback updateCheckDoneCallback;
   private volatile FilterChangeCallback filterChangeCallback;
   private volatile ShowNotificationCallback showNotificationCallback;
-  private final boolean elemhideEnabled;
+  private volatile boolean elemhideEnabled;
   private volatile boolean enabled = true;
-  private List<String> whitelistedDomains;
-
-  private AdblockEngine(final boolean enableElemhide)
-  {
-    this.elemhideEnabled = enableElemhide;
-  }
+  private volatile List<String> whitelistedDomains;
 
   public static AppInfo generateAppInfo(final Context context, boolean developmentBuild)
   {
     String version = "0";
     try
     {
       final PackageInfo info = context.getPackageManager().getPackageInfo(context.getPackageName(), 0);
       version = info.versionName;
       if (developmentBuild)
         version += "." + info.versionCode;
     }
     catch (final NameNotFoundException e)
     {
       Log.e(TAG, "Failed to get the application version number", e);
     }
     final String sdkVersion = String.valueOf(VERSION.SDK_INT);
     final String locale = Locale.getDefault().toString().replace('_', '-');
 
     return AppInfo.builder()
         .setVersion(version)
         .setApplicationVersion(sdkVersion)
         .setLocale(locale)
         .setDevelopmentBuild(developmentBuild)
         .build();
   }
 
-  public static AdblockEngine create(final AppInfo appInfo,
-                                     final String basePath, boolean enableElemhide,
-                                     UpdateAvailableCallback updateAvailableCallback,
-                                     UpdateCheckDoneCallback updateCheckDoneCallback,
-                                     ShowNotificationCallback showNotificationCallback,
-                                     FilterChangeCallback filterChangeCallback)
+  /**
+   * Builds Adblock engine
+   */
+  public static class Builder

[anton, 2017/03/20 08:28:45]

I had to refactor this monster-ctor to builder as arguments list is growing and
the arguments can be used independently.
For example, now i had to add preloaded subscriptions arguments and it meant i
had to add new ctor with other areguments passed as nulls. However adding new
arguments would add this ctor even worse.

I decided not to use struct-like approach with one new class like
'AdlockEngineParams' with all these arguments as class fields. To be discussed

[diegocarloslima, 2017/03/20 15:13:12]

I liked the Builder approach, but as I pointed out on Issue 4948, we need to
either make this issue blocking the Issue 4948 and include here the
isAllowedConnectionCallback param or perform this refactoring in a follow up
issue

[anton, 2017/03/21 05:54:47]

the issues are related by the code (the same file changed) but not the meaning.
i'm going to merge the changes when merging to main branch. i can prepare new
code review after that code review approved but i'm having feeling that this
code review does not depend on that one directly. is that okay for you?

[diegocarloslima, 2017/03/28 13:47:15]

You're right, the two issues don't depend on each other by their
meaning/feature-wise. But they do have conflicts on each other, which require a
further merge step. I just feel like that is important to always have a second
pair of eyes looking on sensitive changes to the code. We can ask Felix about
what he thinks is best to do on these cases.

   {
-    Log.w(TAG, "Create");
+    private Context context;
+    private Map<String, Integer> URLtoResourceIdMap;
+    private AndroidWebRequestResourceWrapper.Storage resourceStorage;
+    private AndroidWebRequest androidWebRequest;
+    private AppInfo appInfo;
+    private String basePath;
 
-    final AdblockEngine engine = new AdblockEngine(enableElemhide);
+    private AdblockEngine engine;
 
-    engine.jsEngine = new JsEngine(appInfo);
-    engine.jsEngine.setDefaultFileSystem(basePath);
+    protected Builder(final AppInfo appInfo, final String basePath)
+    {
+      engine = new AdblockEngine();
+      engine.elemhideEnabled = true;
 
-    engine.logSystem = new AndroidLogSystem();
-    engine.jsEngine.setLogSystem(engine.logSystem);
-
-    engine.webRequest = new AndroidWebRequest(enableElemhide);
-    engine.jsEngine.setWebRequest(engine.webRequest);
-
-    engine.filterEngine = new FilterEngine(engine.jsEngine);
-
-    engine.updateAvailableCallback = updateAvailableCallback;
-    if (engine.updateAvailableCallback != null)
-    {
-      engine.filterEngine.setUpdateAvailableCallback(updateAvailableCallback);
+      // we can't create JsEngine and FilterEngine right now as it starts to download subscriptions
+      // and requests (AndroidWebRequest and probbaly wrappers) are not specified yet
+      this.appInfo = appInfo;
+      this.basePath = basePath;
     }
 
-    engine.updateCheckDoneCallback = updateCheckDoneCallback;
-
-    engine.showNotificationCallback = showNotificationCallback;
-    if (engine.showNotificationCallback != null)
+    public Builder enableElementHiding(boolean enable)
     {
-      engine.filterEngine.setShowNotificationCallback(showNotificationCallback);
+      engine.elemhideEnabled = enable;
+      return this;
     }
 
-    engine.filterChangeCallback = filterChangeCallback;
-    if (engine.filterChangeCallback != null)
+    public Builder preloadSubscriptions(Context context,
+                                        Map<String, Integer> URLtoResourceIdMap,
+                                        AndroidWebRequestResourceWrapper.Storage storage)
     {
-      engine.filterEngine.setFilterChangeCallback(filterChangeCallback);
+      this.context = context;
+      this.URLtoResourceIdMap = URLtoResourceIdMap;
+      this.resourceStorage = storage;
+      return this;
     }
 
-    engine.webRequest.updateSubscriptionURLs(engine.filterEngine);
+    public Builder setUpdateAvailableCallback(UpdateAvailableCallback callback)
+    {
+      engine.updateAvailableCallback = callback;
+      return this;
+    }
 
-    return engine;
+    public Builder setUpdateCheckDoneCallback(UpdateCheckDoneCallback callback)
+    {
+      engine.updateCheckDoneCallback = callback;
+      return this;
+    }
+
+    public Builder setShowNotificationCallback(ShowNotificationCallback callback)
+    {
+      engine.showNotificationCallback = callback;
+      return this;
+    }
+
+    public Builder setFilterChangeCallback(FilterChangeCallback callback)
+    {
+      engine.filterChangeCallback = callback;
+      return this;
+    }
+
+    private void initRequests()
+    {
+      androidWebRequest = new AndroidWebRequest(engine.elemhideEnabled);
+      engine.webRequest = androidWebRequest;
+
+      if (URLtoResourceIdMap != null)
+      {
+        AndroidWebRequestResourceWrapper wrapper = new AndroidWebRequestResourceWrapper(
+          context, engine.webRequest, URLtoResourceIdMap, resourceStorage);
+        wrapper.setListener(engine.resourceWrapperListener);
+
+        engine.webRequest = wrapper;
+      }
+    }
+
+    private void initCallbacks()
+    {
+      if (engine.updateAvailableCallback != null)
+      {
+        engine.filterEngine.setUpdateAvailableCallback(engine.updateAvailableCallback);
+      }
+
+      if (engine.showNotificationCallback != null)
+      {
+        engine.filterEngine.setShowNotificationCallback(engine.showNotificationCallback);
+      }
+
+      if (engine.filterChangeCallback != null)
+      {
+        engine.filterEngine.setFilterChangeCallback(engine.filterChangeCallback);
+      }
+    }
+
+    public AdblockEngine build()
+    {
+      initRequests();
+
+      // webRequest should be ready to be used passed right after JsEngine is created
+      createEngines();

[anton, 2017/03/20 08:28:45]

this is completely strange that webRequest should be ready.
The problem is that JsEngine starts using `webRequest` for update immediately
after it's created (ctor) and if it's not yet assigned it fails to update. So we
had to set webrequest property of just created jsengine instance ASAP! It could
be easily fixed by adding webrequest argument to ctor or (better but more
difficult) changing core logics

+
+      initCallbacks();
+
+      androidWebRequest.updateSubscriptionURLs(engine.filterEngine);
+
+      return engine;
+    }
+
+    private void createEngines()
+    {
+      engine.jsEngine = new JsEngine(appInfo);
+      engine.jsEngine.setDefaultFileSystem(basePath);
+
+      engine.jsEngine.setWebRequest(engine.webRequest);
+
+      engine.logSystem = new AndroidLogSystem();
+      engine.jsEngine.setLogSystem(engine.logSystem);
+
+      engine.filterEngine = new FilterEngine(engine.jsEngine);
+    }
   }
 
-  public static AdblockEngine create(final AppInfo appInfo,
-                                     final String basePath, boolean elemhideEnabled)
+  public static Builder builder(AppInfo appInfo, String basePath)

[anton, 2017/03/20 08:28:45]

i used the same approach (`builder()`) instead of `new
AdblockEngine.Builder..()` to be consistent with `AppInfo.builder()` though `new
.. .Builder()` is more frequently used in Android. e.g.
https://developer.android.com/reference/android/app/AlertDialog.Builder.html

   {
-    return create(appInfo, basePath, elemhideEnabled, null, null, null, null);
+    return new Builder(appInfo, basePath);
   }
 
+  private AndroidWebRequestResourceWrapper.Listener resourceWrapperListener =
+    new AndroidWebRequestResourceWrapper.Listener()
+  {
+    private static final int UPDATE_DELAY_MS = 1 * 1000;
+
+    private Handler handler = new Handler(Looper.getMainLooper());
+
+    private Runnable forceUpdateRunnable = new Runnable()
+    {
+      public void run() {
+        // Filter Engine can be already disposed
+        if (filterEngine != null)
+        {
+          Log.d(TAG, "Force update subscriptions");
+          filterEngine.forceUpdateCheck(updateCheckDoneCallback);
+        }
+      }
+    };
+
+    @Override
+    public void onIntercepted(String url, int resourceId)
+    {
+      // we need to force update subscriptions ASAP after preloaded one is returned

[anton, 2017/03/20 08:28:45]

Please think about it while reviewing!

[diegocarloslima, 2017/03/28 13:47:15]

Is this onIntercepted() callback always called by the same thread? If so, this
looks okay for me. Otherwise, some thread sync is needed here.

[anton, 2017/03/28 13:59:06]

It's invoked in background thread (most likely) and it's up to
core/libadblcokplus to create it. We do sync (using android handler), see
AdblockEngine and it's processed in main thread after it.

+      // but we should note that multiple interceptions (for main easylist and AA) and force update once only
+
+      // adding into main thread queue to avoid concurrency issues (start update while updating)
+      // as usually onIntercepted() is invoked in background thread
+      handler.removeCallbacks(forceUpdateRunnable);
+      handler.postDelayed(forceUpdateRunnable, UPDATE_DELAY_MS);
+
+      Log.d(TAG, "Scheduled force update in " + UPDATE_DELAY_MS);
+    }
+  };
+
   public void dispose()
   {
     Log.w(TAG, "Dispose");
 
     if (this.logSystem != null)
     {
       this.logSystem.dispose();
       this.logSystem = null;
     }
 
@@ skipping 298 matching lines @@
   public void setWhitelistedDomains(List<String> domains)
   {
     this.whitelistedDomains = domains;
   }
 
   public List<String> getWhitelistedDomains()
   {
     return whitelistedDomains;
   }
 }