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.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;
-  // Static members for syncing requests that might not have been saved by filter storage
+  // Sometimes, the app is killed before the extension is able to save all changes regarding
+  // AddOnBridge requests. Given that, we need to store the pending 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 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 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<>();
+  private static final String PENDING_REQUESTS_PREFS_KEY = "PENDING_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)

[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();
+    EventDispatcher.getInstance().registerGeckoThreadListener(ADD_ON_EVENT_LISTENER, ON_FILTERS_LOAD_EVENT, ON_FILTERS_SAVE_EVENT);
+    loadPendingRequests();
   }
 
   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()
+  private static void loadPendingRequests()
+  {
+    PRIVATE_HANDLER.post(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();
+        final String jsonString = sharedPrefs.getString(PENDING_REQUESTS_PREFS_KEY, null);
+        PENDING_REQUESTS.addAll(0, jsonStringToRequestList(jsonString));
       }
     });
   }
 
-  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()
+  private static void sendOrEnqueueRequest(final AddOnRequest request)
+  {
+    PRIVATE_HANDLER.post(new Runnable()
     {
       @Override
       public void run()
       {
-        performSyncRequests();
-      }
-    }, SYNC_REQUESTS_DELAY);
-  }
-
-  private static void addPendingSyncRequest(final ChainedRequest chainedRequest)
-  {
-    postToHandler(new Runnable()
+        if (!filtersLoaded)
+        {
+          PENDING_REQUESTS.add(request);
+        }
+        else
+        {
+          GeckoAppShell.sendRequestToGecko(request);
+        }
+      }
+    });
+  }
+
+  private static void sendPendingRequests()
+  {
+    PRIVATE_HANDLER.post(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();
+        for (final AddOnRequest request : PENDING_REQUESTS)
+        {
+          GeckoAppShell.sendRequestToGecko(request);
+        }
+        PENDING_REQUESTS.clear();
       }
     });
   }
 
-  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<>();
+  private static void clearPendingRequests()
+  {
+    PRIVATE_HANDLER.post(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        storeStringPref(PENDING_REQUESTS_PREFS_KEY, null);
+      }
+    });
+  }
+
+  private static void storePendingRequest(final AddOnRequest request)
+  {
+    PRIVATE_HANDLER.post(new Runnable()
+    {
+      @Override
+      public void run()
+      {
+        final String jsonString = sharedPrefs.getString(PENDING_REQUESTS_PREFS_KEY, null);
+        try
+        {
+          final JSONArray jsonArray = jsonString != null ? new JSONArray(jsonString) : new JSONArray();
+          jsonArray.put(request.value);
+          storeStringPref(PENDING_REQUESTS_PREFS_KEY, jsonArray.toString());
+        }
+        catch (JSONException e)
+        {
+          Log.e(TAG, "Failed to store pending 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 ChainedRequest request = new ChainedRequest(jsonArray.getJSONObject(i), null);
+        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();
   }
@@ skipping 59 matching lines @@
     if (Character.isUpperCase(str.charAt(0)))
     {
       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);
-    final ChainedRequest request = new ChainedRequest(createRequestData("set" + makeFirstCharacterUppercase(name), enable), callback);
-    GeckoAppShell.sendRequestToGecko(request);
-    addPendingSyncRequest(request);
+    final AddOnRequest request =
+        new AddOnRequest(createRequestData("set" + makeFirstCharacterUppercase(name), enable), callback);
+    sendOrEnqueueRequest(request);
+    storePendingRequest(request);
   }
 
   private static void callFunction(final AdblockPlusApiCallback callback, final String name,
       final Map<String, Object> parameters)
   {
-    // By default, requests are not synced
+    // By default, requests are not stored on the pending 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 shouldSyncRequest)
+      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);
     }
-    final ChainedRequest request = new ChainedRequest(requestData, callback);
-    GeckoAppShell.sendRequestToGecko(request);
-    if (shouldSyncRequest)
-    {
-      addPendingSyncRequest(request);
+    final AddOnRequest request = new AddOnRequest(requestData, callback);
+    sendOrEnqueueRequest(request);
+    if (resendIfAborted)
+    {
+      storePendingRequest(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);
@@ skipping 49 matching lines @@
   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, 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)
-    {
-      super(AddOnBridge.REQUEST_NAME,
-          checkForFiltersLoaded ? createRequestData("getFiltersLoaded") : value);
+
+    AddOnRequest(final JSONObject value, final AdblockPlusApiCallback callback)
+    {
+      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)
         {
           this.apiCallback.onApiRequestSucceeded(jsObject);
         }
       }
       catch (final Exception e)
       {
         Log.e(TAG, "onApiRequestSucceeded threw exception: " + e.getMessage(), e);
       }
     }
 
     private void invokeFailureCallback(final String msg)
     {
       if (this.apiCallback != null)
       {
         this.apiCallback.onApiRequestFailed(msg);
       }
     }
 
     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)
-            && getBooleanFromJsObject(jsObject, "value", false))
-        {
-          GeckoAppShell.sendRequestToGecko(this.cloneForRequest());
-        }
-        else
-        {
-          this.attemptRetry();
-        }
+      if (getBooleanFromJsObject(jsObject, "success", false))
+      {
+        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
+        // pending requests
+        // See https://issues.adblockplus.org/ticket/2853
+        clearPendingRequests();
       }
     }
   }
 }