Issue 2853 - Settings changes are sometimes not saved if the user quits the app

mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java

Index: mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java
===================================================================
--- a/mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java
+++ b/mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java
@@ -12,70 +12,204 @@
  * 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.browser;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import android.annotation.SuppressLint;
+import android.content.Context;
+import android.content.SharedPreferences;
 import android.os.Handler;
 import android.os.HandlerThread;
 import android.util.Log;
 
+import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
+import org.mozilla.gecko.EventDispatcher;
 import org.mozilla.gecko.GeckoAppShell;
+import org.mozilla.gecko.util.GeckoEventListener;
 import org.mozilla.gecko.util.GeckoRequest;
 import org.mozilla.gecko.util.NativeJSObject;
 
 @SuppressLint("DefaultLocale")
 public class AddOnBridge
 {
   private static final String TAG = "AdblockBrowser.AddOnBridge";
   private static final String REQUEST_NAME = "AdblockPlus:Api";
-  // Timeout for checking filter loading (in seconds)
-  private static final int QUERY_GET_FILTERS_LOADED_TIMEOUT = 30;
-  // How long to wait between retries (in milliseconds)
-  private static final int QUERY_GET_FILTERS_LOADED_DELAY = 500;
   // Handler+HandlerThread for posting delayed re-tries without interfering with
   // other threads (e.g. the UI or Gecko thread)
   private static final HandlerThread PRIVATE_HANDLER_THREAD;
   private static final Handler PRIVATE_HANDLER;
   // Global handler, for e.g. UI tasks
   private static final HandlerThread GLOBAL_HANDLER_THREAD;
   private static final Handler GLOBAL_HANDLER;
+  // Sometimes, the app is killed before the extension is able to save all changes regarding
+  // AddOnBridge requests. Given that, we need to store the uncompleted requests on SharedPrefs,
+  // so we can resend them to the extension once the app restarts
+  // See https://issues.adblockplus.org/ticket/2853
+  private static final AddOnEventListener ADD_ON_EVENT_LISTENER = new AddOnEventListener();
+  private static final String ON_FILTERS_LOAD_EVENT = "Abb:OnFiltersLoad";
+  private static final String ON_FILTERS_SAVE_EVENT = "Abb:OnFiltersSave";
+  private static final List<AddOnRequest> PENDING_REQUESTS = new ArrayList<>();

[Felix Dahlke, 2017/01/30 15:37:53]

You've renamed this from "uncomplete" to "pending" (I prefer the latter FWIW),
but there are still some references to "uncompleted" requests in the code. Those
should be changed as well IMO.

[diegocarloslima, 2017/01/30 17:12:43]

Acknowledged.

+  private static final String UNCOMPLETED_REQUESTS_PREFS_KEY = "UNCOMPLETED_REQUESTS_PREFS_KEY";
+
+  private static SharedPreferences sharedPrefs;
+  private static boolean filtersLoaded;
 
   static
   {
     PRIVATE_HANDLER_THREAD = new HandlerThread("abp-private-handler");
     PRIVATE_HANDLER_THREAD.setDaemon(true);
     PRIVATE_HANDLER_THREAD.start();
     PRIVATE_HANDLER = new Handler(PRIVATE_HANDLER_THREAD.getLooper());
 
     GLOBAL_HANDLER_THREAD = new HandlerThread("abp-global-handler");
     GLOBAL_HANDLER_THREAD.setDaemon(true);
     GLOBAL_HANDLER_THREAD.start();
     GLOBAL_HANDLER = new Handler(GLOBAL_HANDLER_THREAD.getLooper());
   }
 
+  public static void init(Context context)
+  {
+    sharedPrefs = context.getSharedPreferences(AddOnBridge.class.getName(), Context.MODE_PRIVATE);
+    EventDispatcher.getInstance().registerGeckoThreadListener(ADD_ON_EVENT_LISTENER, ON_FILTERS_LOAD_EVENT, ON_FILTERS_SAVE_EVENT);
+    loadUncompletedRequests();
+  }
+
   public static void postToHandler(Runnable runnable)
   {
     GLOBAL_HANDLER.post(runnable);
   }
 
   public static void postToHandlerDelayed(Runnable runnable, long delayMillis)
   {
     GLOBAL_HANDLER.postDelayed(runnable, delayMillis);
   }
 
+  private static void loadUncompletedRequests()
+  {
+    PRIVATE_HANDLER.post(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        final String jsonString = sharedPrefs.getString(UNCOMPLETED_REQUESTS_PREFS_KEY, null);
+        PENDING_REQUESTS.addAll(0, jsonStringToRequestList(jsonString));
+      }
+    });
+  }
+
+  private static void sendOrEnqueueRequest(final AddOnRequest request)
+  {
+    PRIVATE_HANDLER.post(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        if (!filtersLoaded)
+        {
+          PENDING_REQUESTS.add(request);
+        }
+        else
+        {
+          GeckoAppShell.sendRequestToGecko(request);
+        }
+      }
+    });
+  }
+
+  private static void sendPendingRequests()
+  {
+    PRIVATE_HANDLER.post(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        for (final AddOnRequest request : PENDING_REQUESTS)
+        {
+          GeckoAppShell.sendRequestToGecko(request);
+        }
+        PENDING_REQUESTS.clear();
+      }
+    });
+  }
+
+  private static void clearUncompletedRequests()
+  {
+    PRIVATE_HANDLER.post(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        storeStringPref(UNCOMPLETED_REQUESTS_PREFS_KEY, null);
+      }
+    });
+  }
+
+  private static void storeUncompletedRequest(final AddOnRequest request)
+  {
+    PRIVATE_HANDLER.post(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        final String jsonString = sharedPrefs.getString(UNCOMPLETED_REQUESTS_PREFS_KEY, null);
+        try
+        {
+          final JSONArray jsonArray = jsonString != null ? new JSONArray(jsonString) : new JSONArray();
+          jsonArray.put(request.value);
+          storeStringPref(UNCOMPLETED_REQUESTS_PREFS_KEY, jsonArray.toString());
+        }
+        catch (JSONException e)
+        {
+          Log.e(TAG, "Failed to store uncompleted request with error: " + e.getMessage(), e);
+        }
+      }
+    });
+  }
+
+  private static List<AddOnRequest> jsonStringToRequestList(final String jsonString)
+  {
+    final List<AddOnRequest> requestList = new ArrayList<>();
+    if (jsonString == null)
+    {
+      return requestList;
+    }
+    try
+    {
+      final JSONArray jsonArray = new JSONArray(jsonString);
+      for (int i = 0; i < jsonArray.length(); i++)
+      {
+        final AddOnRequest request = new AddOnRequest(jsonArray.getJSONObject(i), null);
+        requestList.add(request);
+      }
+    }
+    catch (JSONException e)
+    {
+      Log.e(TAG, "Failed to parse json to request list with error: " + e.getMessage(), e);
+    }
+    return requestList;
+  }
+
+  private static void storeStringPref(String key, String value)
+  {
+    final SharedPreferences.Editor editor = sharedPrefs.edit();
+    editor.putString(key, value);
+    editor.commit();
+  }
+
   public static boolean getBooleanFromJsObject(final NativeJSObject obj, final String name,
       final boolean defaultValue)
   {
     try
     {
       return obj.getBoolean(name);
     }
     catch (final Exception e)
@@ -134,47 +268,61 @@ public class AddOnBridge
       return str;
     }
     return Character.toString(Character.toUpperCase(str.charAt(0))) + str.substring(1);
   }
 
   public static void queryValue(final AdblockPlusApiCallback callback, final String name)
   {
     Log.d(TAG, "queryValue for " + name);
-    GeckoAppShell.sendRequestToGecko(
-        new ChainedRequest(
-            createRequestData("get" + makeFirstCharacterUppercase(name)),
-            callback));
+    final AddOnRequest request =
+        new AddOnRequest(createRequestData("get" + makeFirstCharacterUppercase(name)), callback);
+    sendOrEnqueueRequest(request);
   }
 
   public static void setBoolean(final AdblockPlusApiCallback callback, final String name,
       final boolean enable)
   {
     Log.d(TAG, "setBoolean " + enable + " for " + name);
-    GeckoAppShell.sendRequestToGecko(
-        new ChainedRequest(
-            createRequestData("set" + makeFirstCharacterUppercase(name), enable),
-            callback));
+    final AddOnRequest request =
+        new AddOnRequest(createRequestData("set" + makeFirstCharacterUppercase(name), enable), callback);
+    sendOrEnqueueRequest(request);
+    storeUncompletedRequest(request);
   }
 
   private static void callFunction(final AdblockPlusApiCallback callback, final String name,
       final Map<String, Object> parameters)
   {
+    // By default, requests are not stored on the uncompleted request prefs. This should apply for
+    // requests that doesn't result in save operations performed by the extension
+    callFunction(callback, name, parameters, false);
+  }
+
+  private static void callFunction(final AdblockPlusApiCallback callback, final String name,
+      final Map<String, Object> parameters, boolean resendIfAborted)
+  {
     final JSONObject requestData = createRequestData(name);
     try
     {
       for (Map.Entry<String, Object> entry : parameters.entrySet())
+      {
         requestData.put(entry.getKey(), entry.getValue());
+      }
     }
     catch (JSONException e)
     {
       // we're only adding sane objects
       Log.e(TAG, "Creating request data failed with: " + e.getMessage(), e);
     }
-    GeckoAppShell.sendRequestToGecko(new ChainedRequest(requestData, callback));
+    final AddOnRequest request = new AddOnRequest(requestData, callback);
+    sendOrEnqueueRequest(request);
+    if (resendIfAborted)
+    {
+      storeUncompletedRequest(request);
+    }
   }
 
   public static void querySubscriptionListStatus(final AdblockPlusApiCallback callback,
       final String url)
   {
     Log.d(TAG, "querySubscriptionListStatus for " + url);
     final Map<String, Object> parameters = new HashMap<String, Object>();
     parameters.put("url", url);
@@ -186,33 +334,33 @@ public class AddOnBridge
   {
     Log.d(TAG, "addSubscription for " + url + " (" + title + ")");
     final Map<String, Object> parameters = new HashMap<String, Object>();
     parameters.put("url", url);
     if (title != null)
     {
       parameters.put("title", title);
     }
-    callFunction(callback, "addSubscription", parameters);
+    callFunction(callback, "addSubscription", parameters, true);
   }
 
   public static void queryActiveSubscriptions(final AdblockPlusApiCallback callback)
   {
     Log.d(TAG, "queryActiveSubscriptions");
     final Map<String, Object> parameters = new HashMap<String, Object>();
     callFunction(callback, "getActiveSubscriptions", parameters);
   }
 
   public static void removeSubscription(final AdblockPlusApiCallback callback,
       final String url)
   {
     Log.d(TAG, "removeSubscription for " + url);
     final Map<String, Object> parameters = new HashMap<String, Object>();
     parameters.put("url", url);
-    callFunction(callback, "removeSubscription", parameters);
+    callFunction(callback, "removeSubscription", parameters, true);
   }
 
   public static void queryIsLocal(final AdblockPlusApiCallback callback,
       final String url)
   {
     Log.d(TAG, "queryIsLocal for " + url);
     final Map<String, Object> parameters = new HashMap<String, Object>();
     parameters.put("url", url);
@@ -230,50 +378,29 @@ public class AddOnBridge
 
   public static void whitelistSite(final AdblockPlusApiCallback callback, final String url,
       final boolean whitelisted)
   {
     Log.d(TAG, "whitelistSite for " + url);
     final Map<String, Object> parameters = new HashMap<String, Object>();
     parameters.put("url", url);
     parameters.put("whitelisted", whitelisted);
-    callFunction(callback, "whitelistSite", parameters);
+    callFunction(callback, "whitelistSite", parameters, true);
   }
 
-  private static class ChainedRequest extends GeckoRequest
+  private static class AddOnRequest extends GeckoRequest
   {
     private final JSONObject value;
     private final AdblockPlusApiCallback apiCallback;
-    private final boolean checkForFiltersLoaded;
-    private final long creationTime;
 
-    public ChainedRequest(final JSONObject value, final AdblockPlusApiCallback callback,
-        final boolean checkForFiltersLoaded, final long creationTime)
+    AddOnRequest(final JSONObject value, final AdblockPlusApiCallback callback)
     {
-      super(AddOnBridge.REQUEST_NAME,
-          checkForFiltersLoaded ? createRequestData("getFiltersLoaded") : value);
+      super(AddOnBridge.REQUEST_NAME, value);
       this.value = value;
       this.apiCallback = callback;
-      this.checkForFiltersLoaded = checkForFiltersLoaded;
-      this.creationTime = creationTime;
-    }
-
-    public ChainedRequest(final JSONObject value, final AdblockPlusApiCallback callback)
-    {
-      this(value, callback, true, System.currentTimeMillis());
-    }
-
-    public ChainedRequest cloneForRetry()
-    {
-      return new ChainedRequest(this.value, this.apiCallback, true, this.creationTime);
-    }
-
-    public ChainedRequest cloneForRequest()
-    {
-      return new ChainedRequest(this.value, this.apiCallback, false, this.creationTime);
     }
 
     private void invokeSuccessCallback(final NativeJSObject jsObject)
     {
       try
       {
         if (this.apiCallback != null)
         {
@@ -294,73 +421,51 @@ public class AddOnBridge
       }
     }
 
     private void invokeFailureCallback(final NativeJSObject jsObject)
     {
       invokeFailureCallback(getStringFromJsObject(jsObject, "error", "unknown error"));
     }
 
-    private void attemptRetry()
-    {
-      if (System.currentTimeMillis() - this.creationTime > (QUERY_GET_FILTERS_LOADED_TIMEOUT * 1000))
-      {
-        this.invokeFailureCallback("getFiltersLoaded timeout");
-      }
-      else
-      {
-        Log.d(TAG, "Retrying: " + this.value);
-        final ChainedRequest next = this.cloneForRetry();
-        PRIVATE_HANDLER.postDelayed(new Runnable()
-        {
-          @Override
-          public void run()
-          {
-            GeckoAppShell.sendRequestToGecko(next);
-          }
-        }, QUERY_GET_FILTERS_LOADED_DELAY);
-      }
-    }
-
     @Override
     public void onError(final NativeJSObject error)
     {
-      if (this.checkForFiltersLoaded)
-      {
-        this.attemptRetry();
-      }
-      else
-      {
-        this.invokeFailureCallback(
-            "GeckoRequest error: " + error.optString("message", "<no message>") + "\n" +
-            error.optString("stack", "<no stack>"));
-      }
+      this.invokeFailureCallback(
+          "GeckoRequest error: " + error.optString("message", "<no message>") + "\n" +
+              error.optString("stack", "<no stack>"));
     }
 
     @Override
     public void onResponse(final NativeJSObject jsObject)
     {
-      if (this.checkForFiltersLoaded)
+      if (getBooleanFromJsObject(jsObject, "success", false))
       {
-        if (getBooleanFromJsObject(jsObject, "success", false)
-            && getBooleanFromJsObject(jsObject, "value", false))
-        {
-          GeckoAppShell.sendRequestToGecko(this.cloneForRequest());
-        }
-        else
-        {
-          this.attemptRetry();
-        }
+        this.invokeSuccessCallback(jsObject);
       }
       else
       {
-        if (getBooleanFromJsObject(jsObject, "success", false))
-        {
-          this.invokeSuccessCallback(jsObject);
-        }
-        else
-        {
-          this.invokeFailureCallback(jsObject);
-        }
+        this.invokeFailureCallback(jsObject);
+      }
+    }
+  }
+
+  private static class AddOnEventListener implements GeckoEventListener
+  {
+    @Override
+    public void handleMessage(String event, JSONObject message)
+    {
+      if (ON_FILTERS_LOAD_EVENT.equals(event))
+      {
+        // The filters have been loaded by the extension. Given that, we can send all pending requests
+        filtersLoaded = true;
+        sendPendingRequests();
+      }
+      else if (ON_FILTERS_SAVE_EVENT.equals(event))
+      {
+        // All changes have been saved by the extension. That way, we can clear our list of
+        // uncompleted requests
+        // See https://issues.adblockplus.org/ticket/2853
+        clearUncompletedRequests();
       }
     }
   }
 }