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

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

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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.browser;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
+import java.util.concurrent.Semaphore;
 
 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.GeckoAppShell;
 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;
+  // Static members for syncing requests that might not have been saved by filter storage
+  // See https://issues.adblockplus.org/ticket/2853
+  private static final Semaphore SYNC_REQUESTS_SEMAPHORE = new Semaphore(1);
+  private static final int SYNC_REQUESTS_DELAY = 30 * 1000;
+  private static final List<ChainedRequest> PENDING_SYNC_REQUESTS = new ArrayList<>();

[Felix Dahlke, 2016/12/13 11:25:10]

I'm a bit confused as to what the point of all this is. How I see it, we never
actually do anything with PENDING_SYNC_REQUESTS, is that correct? We just keep
sending a "requestsSaved" message to the extension effectively. Does that alone
solve issue 2853? Because if so, we should be able to simplify this code, how I
see it.

Also, I'm not sure I understand the naming. What is a "sync request"? Is that a
"requestsSaved" message which did not receive a response yet? Maybe we can find
a name that describes what it does, i.e. wait for the extension to finish saving
filters?

+  private static final String PENDING_SYNC_REQUESTS_KEY = "PENDING_SYNC_REQUESTS_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)

[Felix Dahlke, 2016/12/13 11:25:10]

It makes me a bit nervous to rely on outside code initialising this once, and
only once. I think it would be more robust to make this method deal with
multiple invocations. How I see it, we could just check if sharedPrefs is set,
and if not, proceed. While at it, I would suggest to rename this from `init` to
`startSyncRequests` to make it more clear what it does - and combine it with the
method that currently has that name.

+  {
+    sharedPrefs = context.getSharedPreferences(AddOnBridge.class.getName(), Context.MODE_PRIVATE);
+    startSyncRequests();
+  }
+
   public static void postToHandler(Runnable runnable)
   {
     GLOBAL_HANDLER.post(runnable);
   }
 
   public static void postToHandlerDelayed(Runnable runnable, long delayMillis)
   {
     GLOBAL_HANDLER.postDelayed(runnable, delayMillis);
   }
 
+  public static void startSyncRequests()

[Felix Dahlke, 2016/12/13 11:25:10]

Does this have to be public? But see my suggestion above, if you implement that,
that would make this comment obsolete.

+  {
+    postToHandler(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        SYNC_REQUESTS_SEMAPHORE.acquireUninterruptibly();
+        final String jsonString = sharedPrefs.getString(PENDING_SYNC_REQUESTS_KEY, null);
+        PENDING_SYNC_REQUESTS.clear();
+        PENDING_SYNC_REQUESTS.addAll(jsonStringToRequestList(jsonString));
+        for (final ChainedRequest request : PENDING_SYNC_REQUESTS)
+        {
+          GeckoAppShell.sendRequestToGecko(request);
+        }
+        SYNC_REQUESTS_SEMAPHORE.release();
+        performSyncRequests();
+      }
+    });
+  }
+
+  private static void performSyncRequests()
+  {
+    SYNC_REQUESTS_SEMAPHORE.acquireUninterruptibly();
+    if (PENDING_SYNC_REQUESTS.isEmpty())
+    {
+      SYNC_REQUESTS_SEMAPHORE.release();
+      scheduleSyncRequests();
+      return;
+    }
+    queryValue(new AdblockPlusApiCallback()
+    {
+      @Override
+      public void onApiRequestSucceeded(NativeJSObject jsObject)
+      {
+        final boolean requestsSaved = AddOnBridge.getBooleanFromJsObject(jsObject, "value", false);
+        if (requestsSaved)
+        {
+          PENDING_SYNC_REQUESTS.clear();
+          storeStringPref(PENDING_SYNC_REQUESTS_KEY, requestListToJsonString(PENDING_SYNC_REQUESTS));
+        }
+        SYNC_REQUESTS_SEMAPHORE.release();
+        scheduleSyncRequests();
+      }
+
+      @Override
+      public void onApiRequestFailed(String errorMessage)
+      {
+        SYNC_REQUESTS_SEMAPHORE.release();
+        scheduleSyncRequests();
+      }
+    }, "requestsSaved");
+  }
+
+  private static void scheduleSyncRequests()
+  {
+    postToHandlerDelayed(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        performSyncRequests();
+      }
+    }, SYNC_REQUESTS_DELAY);
+  }
+
+  private static void addPendingSyncRequest(final ChainedRequest chainedRequest)
+  {
+    postToHandler(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        SYNC_REQUESTS_SEMAPHORE.acquireUninterruptibly();
+        PENDING_SYNC_REQUESTS.add(chainedRequest);
+        storeStringPref(PENDING_SYNC_REQUESTS_KEY, requestListToJsonString(PENDING_SYNC_REQUESTS));
+        SYNC_REQUESTS_SEMAPHORE.release();
+      }
+    });
+  }
+
+  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)
     {
       return defaultValue;
@@ skipping 58 matching lines @@
     GeckoAppShell.sendRequestToGecko(
         new ChainedRequest(
             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);
+    addPendingSyncRequest(request);
   }
 
   private static void callFunction(final AdblockPlusApiCallback callback, final String name,
       final Map<String, Object> parameters)
   {
+    // By default, requests are not synced
+    callFunction(callback, name, parameters, false);
+  }
+
+  private static void callFunction(final AdblockPlusApiCallback callback, final String name,
+      final Map<String, Object> parameters, boolean shouldSyncRequest)
+  {
     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 (shouldSyncRequest)
+    {
+      addPendingSyncRequest(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);
     callFunction(callback, "isSubscriptionListed", parameters);
   }
 
   public static void addSubscription(final AdblockPlusApiCallback callback,
       final String url, final String title)
   {
     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);
     callFunction(callback, "isLocal", parameters);
   }
 
   public static void queryIsPageWhitelisted(final AdblockPlusApiCallback callback,
       final String url)
   {
     Log.d(TAG, "queryIsPageWhitelisted for " + url);
     final Map<String, Object> parameters = new HashMap<String, Object>();
     parameters.put("url", url);
     callFunction(callback, "isPageWhitelisted", parameters);
   }
 
   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;
 
     public ChainedRequest(final JSONObject value, final AdblockPlusApiCallback callback,
@@ skipping 108 matching lines @@
           this.invokeSuccessCallback(jsObject);
         }
         else
         {
           this.invokeFailureCallback(jsObject);
         }
       }
     }
   }
 }