Issue 6366 - Elemhide thread is accessing released engine

libadblockplus-android/src/org/adblockplus/libadblockplus/android/SingleInstanceEngineProvider.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 40 matching lines @@
   private boolean developmentBuild;
   private String preloadedPreferenceName;
   private Map<String, Integer> urlToResourceIdMap;
   private AdblockEngine engine;
   private CountDownLatch engineCreated;
   private Long v8IsolateProviderPtr;
   private List<EngineCreatedListener> engineCreatedListeners =
     new LinkedList<EngineCreatedListener>();
   private List<EngineDisposedListener> engineDisposedListeners =
     new LinkedList<EngineDisposedListener>();
+  private Object engineLock = new Object();

[diegocarloslima, 2018/02/07 14:00:06]

This should be final

[jens, 2018/02/07 14:01:23]

Could be final

[anton, 2018/02/07 14:03:22]

Acknowledged.

See patch set #3

 
   /*
     Simple ARC management for AdblockEngine
     Use `retain` and `release`
    */
 
   private AtomicInteger referenceCounter = new AtomicInteger(0);
 
   /**
    * Init with context
@@ skipping 59 matching lines @@
     this.engineDisposedListeners.remove(listener);
   }
 
   public void clearEngineDisposedListeners()
   {
     this.engineDisposedListeners.clear();
   }
 
   private void createAdblock()
   {
-    ConnectivityManager connectivityManager =
-      (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
-    IsAllowedConnectionCallback isAllowedConnectionCallback =
-      new IsAllowedConnectionCallbackImpl(connectivityManager);
+    Log.w(TAG, "Waiting for lock");
+    synchronized (engineLock)
+    {
+      Log.d(TAG, "Creating adblock engine ...");
+      ConnectivityManager connectivityManager =
+        (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
+      IsAllowedConnectionCallback isAllowedConnectionCallback =
+        new IsAllowedConnectionCallbackImpl(connectivityManager);
 
-    Log.d(TAG, "Creating adblock engine ...");
+      AdblockEngine.Builder builder = AdblockEngine
+        .builder(
+          AdblockEngine.generateAppInfo(context, developmentBuild),
+          basePath)
+        .setIsAllowedConnectionCallback(isAllowedConnectionCallback)
+        .enableElementHiding(true);
 
-    AdblockEngine.Builder builder = AdblockEngine
-      .builder(
-        AdblockEngine.generateAppInfo(context, developmentBuild),
-        basePath)
-      .setIsAllowedConnectionCallback(isAllowedConnectionCallback)
-      .enableElementHiding(true);
+      if (v8IsolateProviderPtr != null)
+      {
+        builder.useV8IsolateProvider(v8IsolateProviderPtr);
+      }
 
-    if (v8IsolateProviderPtr != null)
-    {
-      builder.useV8IsolateProvider(v8IsolateProviderPtr);
-    }
+      // if preloaded subscriptions provided
+      if (preloadedPreferenceName != null)
+      {
+        SharedPreferences preloadedSubscriptionsPrefs = context.getSharedPreferences(
+          preloadedPreferenceName,
+          Context.MODE_PRIVATE);
+        builder.preloadSubscriptions(
+          context,
+          urlToResourceIdMap,
+          new AndroidWebRequestResourceWrapper.SharedPrefsStorage(preloadedSubscriptionsPrefs));
+      }
 
-    // if preloaded subscriptions provided
-    if (preloadedPreferenceName != null)
-    {
-      SharedPreferences preloadedSubscriptionsPrefs = context.getSharedPreferences(
-        preloadedPreferenceName,
-        Context.MODE_PRIVATE);
-      builder.preloadSubscriptions(
-        context,
-        urlToResourceIdMap,
-        new AndroidWebRequestResourceWrapper.SharedPrefsStorage(preloadedSubscriptionsPrefs));
-    }
+      engine = builder.build();
 
-    engine = builder.build();
+      Log.d(TAG, "AdblockHelper engine created");
 
-    Log.d(TAG, "AdblockHelper engine created");
-
-    // sometimes we need to init AdblockEngine instance, eg. set user settings
-    for (EngineCreatedListener listener : engineCreatedListeners)
-    {
-      listener.onAdblockEngineCreated(engine);
+      // sometimes we need to init AdblockEngine instance, eg. set user settings
+      for (EngineCreatedListener listener : engineCreatedListeners)
+      {
+        listener.onAdblockEngineCreated(engine);
+      }
     }
   }
 
   @Override
   public synchronized boolean retain(boolean asynchronous)
   {
     boolean firstInstance = false;
 
     if (referenceCounter.getAndIncrement() == 0)
     {
       firstInstance = true;
 
       if (!asynchronous)
       {
         createAdblock();
       }
       else
       {
         // latch is required for async (see `waitForReady()`)
         engineCreated = new CountDownLatch(1);
 
         new Thread(new Runnable()
         {
           @Override
           public void run()
           {
-            createAdblock();
+            synchronized (engineLock)
+            {
+              createAdblock();
 
-            // unlock waiting client thread
-            engineCreated.countDown();
+              // unlock waiting client thread
+              engineCreated.countDown();
+            }
           }
         }).start();
       }
     }
     return firstInstance;
   }
 
   @Override
   public void waitForReady()
   {
@@ skipping 42 matching lines @@
       else
       {
         disposeAdblock();
       }
     }
     return lastInstance;
   }
 
   private void disposeAdblock()
   {
-    Log.w(TAG, "Disposing adblock engine");
+    Log.w(TAG, "Waiting for lock");
+    synchronized (engineLock)
+    {
+      Log.w(TAG, "Disposing adblock engine");
 
-    engine.dispose();
-    engine = null;
+      engine.dispose();
+      engine = null;
 
-    // sometimes we need to deinit something after AdblockEngine instance disposed
-    // eg. release user settings
-    for (EngineDisposedListener listener : engineDisposedListeners)
-    {
-      listener.onAdblockEngineDisposed();
+      // sometimes we need to deinit something after AdblockEngine instance disposed
+      // eg. release user settings
+      for (EngineDisposedListener listener : engineDisposedListeners)
+      {
+        listener.onAdblockEngineDisposed();
+      }
     }
   }
 
   @Override
   public int getCounter()
   {
     return referenceCounter.get();
   }
+
+  @Override
+  public Object getEngineLock()
+  {
+    return engineLock;
+  }
 }