Issue 5464 - Upgrade to new asynchronous version of abp2blocklist

safari/contentBlocking.js

Index: safari/contentBlocking.js
===================================================================
--- a/safari/contentBlocking.js
+++ b/safari/contentBlocking.js
@@ -32,102 +32,166 @@
   {
     port.off("safari.legacyAPISupported", onLegacyAPISupported);
     resolve(msg.legacyAPISupported);
   }
   port.on("safari.legacyAPISupported", onLegacyAPISupported);
 });
 let contentBlockingActive = false;
 let afterContentBlockingFinished = null;
+let pendingContentBlockerUpdate = null;
 let contentBlockListDirty = true;
 let lastSetContentBlockerError;
 
 function clearBlockCounters()
 {
   ext.pages.query({}, pages =>
   {
     for (let page of pages)
       page.browserAction.setBadge();
   });
 }
 
-function setContentBlocker(callback)
+function setContentBlocker()
 {
-  // When given the same rules as last time setContentBlocker will always give
-  // null (success) to the callback, even when there was actually an error. We
-  // cache the last result therefore so that we can provide a consistent result
-  // and also to avoid wastefully regenerating an identical blocklist.
-  if (!contentBlockListDirty)
+  return new Promise((resolve, reject) =>
   {
-    callback(lastSetContentBlockerError);
-    return;
-  }
+    // Reset state and either fulfill or reject this promise.
+    let completePromise = error =>
+    {
+      lastSetContentBlockerError = error;
+      contentBlockListDirty = false;
 
-  let contentBlockerList = new ContentBlockerList();
-  for (let subscription of FilterStorage.subscriptions)
-    if (!subscription.disabled)
-      for (let filter of subscription.filters)
-        contentBlockerList.addFilter(filter);
+      if (error)
+        reject(error);
+      else
+        resolve();
+    };
+
+    // When given the same rules as last time setContentBlocker will always
+    // resolve with null (success), even when there was actually an
+    // error. We cache the last result therefore so that we can provide a
+    // consistent result and also to avoid wastefully regenerating an identical
+    // blocklist.
+    if (!contentBlockListDirty)
+    {
+      completePromise(lastSetContentBlockerError);
+      return;
+    }
 
-  contentBlockListDirty = false;
-  safari.extension.setContentBlocker(
-    // There is a strange bug in setContentBlocker for Safari 9 where if both
-    // the callback parameter is provided and the rules aren't converted to a
-    // JSON string it fails. Worse still it actually performs the callback twice
-    // too, firstly with an empty string and then with an Error:
-    //   "Extension compilation failed: Failed to parse the JSON String."
-    // To mitigate this we convert the rules to JSON here and also ignore
-    // callback values of "". (Usually the callback is performed with either
-    // null for success or an Error on failure.)
-    // Bug #26322821 filed on bugreport.apple.com
-    JSON.stringify(contentBlockerList.generateRules()),
-    function(error)
+    let contentBlockerList = new ContentBlockerList();
+    for (let subscription of FilterStorage.subscriptions)
+    {
+      if (!subscription.disabled)
+      {
+        for (let filter of subscription.filters)
+          contentBlockerList.addFilter(filter);
+      }
+    }
+
+    contentBlockerList.generateRules().then(rules =>
     {
-      if (error == "")
-        return;
+      safari.extension.setContentBlocker(
+        // There is a strange bug in setContentBlocker for Safari 9 where if
+        // both the callback parameter is provided and the rules aren't
+        // converted to a JSON string it fails. Worse still it actually
+        // performs the callback twice too, firstly with an empty string and
+        // then with an Error: "Extension compilation failed: Failed to parse
+        // the JSON String." To mitigate this we convert the rules to JSON here
+        // and also ignore callback values of "". (Usually the callback is
+        // performed with either null for success or an Error on failure.)
+        // Bug #26322821 filed on bugreport.apple.com
+        JSON.stringify(rules),
+        function(error)
+        {
+          if (error == "")
+            return;
 
-      lastSetContentBlockerError = error;
-      callback(error);
-    }
-  );
+          completePromise(error);
+        }
+      );
+    })
+    .catch(completePromise);
+  });
 }
 
 function updateContentBlocker(isStartup, legacyAPISupported)
 {
+  // Another update can be requested while one is still in progress (e.g. the
+  // user adds filter lists in quick succession). When this happens, save the
+  // request and execute it later.
+  if (afterContentBlockingFinished)
+  {
+    pendingContentBlockerUpdate = {
+      params: Array.from(arguments),
+      // Save the current dirty state so we can set it later before calling
+      // this function again.
+      setDirty: contentBlockListDirty
+    };
+    return;
+  }
+
   afterContentBlockingFinished = new Promise(resolve =>
   {
-    setContentBlocker(error =>
+    setContentBlocker().catch(error =>
     {
       if (error instanceof Error)

[Manish Jethani, 2017/08/16 10:23:02]

The code was ignoring errors that weren't instances of Error for some reason.
Maybe Safari 9 throws an error that is a string, but isn't really an error
(operation successful). I'm leaving this here for now, but maybe we should
remove it and let any bugs get discovered in testing.

[Sebastian Noack, 2017/08/16 10:30:34]

It seems, it would be simpler, if you move that logic to where you
reject/resolve the Promise in the first place.

[Manish Jethani, 2017/08/16 11:13:38]

Done.

-      {
-        let suppressErrorMessage = false;
-
-        // If the content blocking API fails the first time it's used the
-        // legacy blocking API (if available) won't have been disabled.
-        if (!contentBlockingActive && legacyAPISupported)
-        {
-          Prefs.safariContentBlocker = false;
-          // If content blocking failed on startup and we're switching back to
-          // the legacy API anyway we don't need to show an error message.
-          if (isStartup)
-            suppressErrorMessage = true;
-        }
-
-        if (!suppressErrorMessage)
-          alert(error.message);
-      }
-      else if (!contentBlockingActive)
+        throw error;
+    })
+    // There's no need for a catch handler at the end because the handler is
+    // not expected to throw any errors. If this changes, however, then the
+    // error must be handled appropriately.
+    .then(() =>
+    {
+      if (!contentBlockingActive)
       {
         contentBlockingActive = true;
         clearBlockCounters();
       }
+    },
+    error =>
+    {
+      let suppressErrorMessage = false;
 
+      // If the content blocking API fails the first time it's used the
+      // legacy blocking API (if available) won't have been disabled.
+      if (!contentBlockingActive && legacyAPISupported)
+      {
+        Prefs.safariContentBlocker = false;
+        // If content blocking failed on startup and we're switching back to
+        // the legacy API anyway we don't need to show an error message.
+        if (isStartup)
+          suppressErrorMessage = true;
+      }
+
+      if (!suppressErrorMessage)
+        alert(error.message);
+    })
+    .then(() =>
+    {
+      afterContentBlockingFinished = null;

[Manish Jethani, 2017/08/16 10:23:02]

Code that should run regardless of success/failure is here in a separate then
handler at the end.

       resolve(contentBlockingActive);
-      afterContentBlockingFinished = null;
     });
+  })
+  .then(active =>

[Sebastian Noack, 2017/08/16 10:30:34]

I wonder if this logic, couldn't just go into the success callback above?

[Manish Jethani, 2017/08/16 11:13:38]

You're right, done.

Note that we have to add a catch handler at the end now because the then handler
before it could throw an error, possibly.

+  {
+    // If there's another update pending, execute it now.
+    if (pendingContentBlockerUpdate)
+    {
+      let {params, setDirty} = pendingContentBlockerUpdate;
+      pendingContentBlockerUpdate = null;
+
+      if (setDirty)
+        contentBlockListDirty = true;
+
+      updateContentBlocker.apply(null, params);
+      return afterContentBlockingFinished;
+    }
+
+    return active;
   });
 }
 
 if (contentBlockingSupported)
 {
   Promise.all([Prefs.untilLoaded,
                FilterNotifier.once("load"),
                legacyAPISupported]).then(resolvedValues =>