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,27 +12,34 @@
  * 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";
@@ -42,40 +49,146 @@ public class AddOnBridge
   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 String ON_SAVE_EVENT = "Abb:OnSave";
+  private static final List<ChainedRequest> UNCOMPLETED_REQUESTS = new ArrayList<>();
+  private static final String UNCOMPLETED_REQUESTS_PREFS_KEY = "UNCOMPLETED_REQUESTS_PREFS_KEY";
+
+  private static SharedPreferences sharedPrefs;
 
   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(new AddOnEventListener(), ON_SAVE_EVENT);
+    loadAndResendUncompletedRequests();
+  }
+
   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 loadAndResendUncompletedRequests()
+  {
+    postToHandler(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        final String jsonString = sharedPrefs.getString(UNCOMPLETED_REQUESTS_PREFS_KEY, null);
+        UNCOMPLETED_REQUESTS.clear();
+        UNCOMPLETED_REQUESTS.addAll(jsonStringToRequestList(jsonString));
+        for (final ChainedRequest request : UNCOMPLETED_REQUESTS)
+        {
+          GeckoAppShell.sendRequestToGecko(request);
+        }
+      }
+    });
+  }
+
+  private static void clearUncompletedRequests()
+  {
+    postToHandler(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        if (!UNCOMPLETED_REQUESTS.isEmpty())
+        {
+          UNCOMPLETED_REQUESTS.clear();
+          storeStringPref(UNCOMPLETED_REQUESTS_PREFS_KEY, requestListToJsonString(UNCOMPLETED_REQUESTS));
+        }
+      }
+    });
+  }
+
+  private static void addUncompletedRequest(final ChainedRequest chainedRequest)
+  {
+    postToHandler(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        UNCOMPLETED_REQUESTS.add(chainedRequest);
+        storeStringPref(UNCOMPLETED_REQUESTS_PREFS_KEY, requestListToJsonString(UNCOMPLETED_REQUESTS));
+      }
+    });
+  }
+
+  private static String requestListToJsonString(final List<ChainedRequest> requestList)
+  {
+    if (requestList == null)
+    {
+      return null;
+    }
+    final JSONArray jsonArray = new JSONArray();
+    for (final ChainedRequest request : requestList)
+    {
+      jsonArray.put(request.value);
+    }
+    return jsonArray.toString();
+  }
+
+  private static List<ChainedRequest> jsonStringToRequestList(final String jsonString)
+  {
+    final List<ChainedRequest> requestList = new ArrayList<>();
+    if (jsonString == null)
+    {
+      return requestList;
+    }
+    try
+    {
+      final JSONArray jsonArray = new JSONArray(jsonString);
+      for (int i = 0; i < jsonArray.length(); i++)
+      {
+        final ChainedRequest request = new ChainedRequest(jsonArray.getJSONObject(i), null);
+        requestList.add(request);
+      }
+    }
+    catch (JSONException 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)
@@ -144,37 +257,50 @@ public class AddOnBridge
             createRequestData("get" + makeFirstCharacterUppercase(name)),
             callback));
   }
 
   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 ChainedRequest request =
+        new ChainedRequest(createRequestData("set" + makeFirstCharacterUppercase(name), enable), callback);
+    GeckoAppShell.sendRequestToGecko(request);
+    addUncompletedRequest(request);
   }
 
   private static void callFunction(final AdblockPlusApiCallback callback, final String name,
       final Map<String, Object> parameters)
   {
+    // By default, requests are not added to the uncompleted request list. 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 shouldAddUncompletedRequest)

[Felix Dahlke, 2016/12/21 20:23:25]

Nit: `shouldAddUncompletedRequest` sounds a bit like a leaky abstraction. How
about something like `repeatIfAborted`? Not a great name either, but something
in that vein maybe...

[diegocarloslima, 2016/12/21 20:29:31]

Acknowledged.

+  {
     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 ChainedRequest request = new ChainedRequest(requestData, callback);
+    GeckoAppShell.sendRequestToGecko(request);
+    if (shouldAddUncompletedRequest)
+    {
+      addUncompletedRequest(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 +312,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,17 +356,17 @@ 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 final JSONObject value;
     private final AdblockPlusApiCallback apiCallback;
     private final boolean checkForFiltersLoaded;
     private final long creationTime;
@@ -358,9 +484,24 @@ public class AddOnBridge
         }
         else
         {
           this.invokeFailureCallback(jsObject);
         }
       }
     }
   }
+
+  private static class AddOnEventListener implements GeckoEventListener
+  {
+    @Override
+    public void handleMessage(String event, JSONObject message)
+    {
+      if (ON_SAVE_EVENT.equals(event))
+      {
+        // This indicates that 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();
+      }
+    }
+  }
 }