Issue 5464 - Upgrade to new asynchronous version of abp2blocklist

safari/contentBlocking.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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
@@ skipping 37 matching lines @@
     for (let page of pages)
       page.browserAction.setBadge();
   });
 }
 
 function setContentBlocker()
 {
   return new Promise((resolve, reject) =>
   {
     // Reset state and either fulfill or reject this promise.
-    let completePromise = error =>
+    function completePromise(error)
     {
       lastSetContentBlockerError = error;
       contentBlockListDirty = false;
 
-      if (error)
+      if (error instanceof 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;
@@ skipping 44 matching lines @@
   {
     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().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.

-        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 = setContentBlocker().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(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.

-  {
+  .then(() =>
+  {
+    afterContentBlockingFinished = null;
+
     // 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;
+    return contentBlockingActive;
   });
 }
 
 if (contentBlockingSupported)
 {
   Promise.all([Prefs.untilLoaded,
                FilterNotifier.once("load"),
                legacyAPISupported]).then(resolvedValues =>
   {
     let legacyAPISupported = resolvedValues[2];
@@ skipping 17 matching lines @@
     });
   });
 }
 
 port.on("safari.contentBlockingActive", (msg, sender) =>
 {
   if (!contentBlockingActive && afterContentBlockingFinished)
     return afterContentBlockingFinished;
   return contentBlockingActive;
 });