Issue 4591 - Crash after dispose

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
@@ skipping 85 matching lines @@
     final String locale = Locale.getDefault().toString().replace('_', '-');
 
     return AppInfo.builder()
         .setVersion(version)
         .setApplicationVersion(sdkVersion)
         .setLocale(locale)
         .setDevelopmentBuild(developmentBuild)
         .build();
   }
 
-  public static final UpdateAvailableCallback UPDATE_AVAILABLE_CALLBACK =
-    new UpdateAvailableCallback()
-  {
-    @Override
-    public void updateAvailableCallback(String url)
-    {
-      Log.d(TAG, "Update available for " + url);
-    }
-  };
-
-  public static final UpdateCheckDoneCallback UPDATE_CHECK_DONE_CALLBACK =
-    new UpdateCheckDoneCallback()
-  {
-    @Override
-    public void updateCheckDoneCallback(String error)
-    {
-      Log.d(TAG, "Update check done, error: " + error);
-    }
-  };
-
-  public static final ShowNotificationCallback SHOW_NOTIFICATION_CALLBACK =
-    new ShowNotificationCallback()
-  {
-    @Override
-    public void showNotificationCallback(Notification jsValue)
-    {
-      Log.d(TAG, "Notification: " + jsValue);
-    }
-  };
-
-  public static final FilterChangeCallback FILTER_CHANGE_CALLBACK =
-    new FilterChangeCallback()
-  {
-    @Override
-    public void filterChangeCallback(String action, JsValue jsValue)
-    {
-      Log.d(TAG, "Filter changed: " + action + (!jsValue.isUndefined() ? ", " + jsValue : ""));
-    }
-  };
-
   public static AdblockEngine create(final Context context, final AppInfo appInfo,
                                      final String basePath, boolean enableElemhide,
                                      UpdateAvailableCallback updateAvailableCallback,
                                      UpdateCheckDoneCallback updateCheckDoneCallback,
                                      ShowNotificationCallback showNotificationCallback,
                                      FilterChangeCallback filterChangeCallback)
   {
     Log.w(TAG, "Create");
 
     final AdblockEngine engine = new AdblockEngine(context, enableElemhide);
@@ skipping 30 matching lines @@
     }
 
     engine.webRequest.updateSubscriptionURLs(engine.filterEngine);
 
     return engine;
   }
 
   public static AdblockEngine create(final Context context, final AppInfo appInfo,
                                      final String basePath, boolean elemhideEnabled)
   {
-    return create(context, appInfo, basePath, elemhideEnabled,
-      UPDATE_AVAILABLE_CALLBACK, UPDATE_CHECK_DONE_CALLBACK,
-      SHOW_NOTIFICATION_CALLBACK, FILTER_CHANGE_CALLBACK);
+    return create(context, appInfo, basePath, elemhideEnabled, null, null, null, null);

[diegocarloslima, 2016/11/04 10:35:15]

Is this callback cleanup related to the crash? If not, It might be better to do
it in a separate issue 

[anton, 2016/11/07 07:35:14]

Yes, it's related as since we use smart pointers callback is fired after we no
longer need it and it causes crash since vm is released (GetJavaVM() returns
NULL
(https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...)
and cause NRE with sigsegv 11 (see issue log attachments)).

[Felix Dahlke, 2016/11/18 06:56:55]

So what if the user passes these callback objects in? Wouldn't we then have the
exact same problem as with our static versions? (Not that we seem to need those
for anything but logging.)

   }
 
   public void dispose()
   {
     Log.w(TAG, "Dispose");
 
-    // Safe disposing (just in case)
-    if (this.filterEngine != null)
-    {
-      this.filterEngine.dispose();
-      this.filterEngine = null;
-    }
-
-    if (this.jsEngine != null)
-    {
-      this.jsEngine.dispose();
-      this.jsEngine = null;
-    }
-
     if (this.logSystem != null)
     {
       this.logSystem.dispose();
       this.logSystem = null;
     }
 
     if (this.webRequest != null)
     {
       this.webRequest.dispose();
       this.webRequest = null;
     }
 
     if (this.updateAvailableCallback != null)
     {
+      if (this.filterEngine != null)
+      {
+        this.filterEngine.removeUpdateAvailableCallback();
+      }
+
       this.updateAvailableCallback.dispose();
       this.updateAvailableCallback = null;
     }
 
     if (this.updateCheckDoneCallback != null)
     {
       this.updateCheckDoneCallback.dispose();
       this.updateCheckDoneCallback = null;
     }
 
     if (this.filterChangeCallback != null)
     {
+      if (this.filterEngine != null)
+      {
+        this.filterEngine.removeFilterChangeCallback();
+      }
+
       this.filterChangeCallback.dispose();
       this.filterChangeCallback = null;
     }
 
     if (this.showNotificationCallback != null)
     {
+      if (this.filterEngine != null)
+      {
+        this.filterEngine.removeShowNotificationCallback();
+      }
+
       this.showNotificationCallback.dispose();
       this.showNotificationCallback = null;
     }
+
+    // Safe disposing (just in case)
+    if (this.filterEngine != null)
+    {
+      this.filterEngine.dispose();
+      this.filterEngine = null;
+    }
+
+    if (this.jsEngine != null)
+    {
+      this.jsEngine.dispose();
+      this.jsEngine = null;
+    }
   }
 
   public boolean isFirstRun()
   {
     return this.filterEngine.isFirstRun();
   }
 
   public boolean isElemhideEnabled()
   {
     return this.elemhideEnabled;
@@ skipping 235 matching lines @@
   public void setWhitelistedDomains(List<String> domains)
   {
     this.whitelistedDomains = domains;
   }
 
   public List<String> getWhitelistedDomains()
   {
     return whitelistedDomains;
   }
 }