Issue 3687 - Add experimental support for Safari content blockers

safari/contentBlocking.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 20 matching lines @@
   function onLegacyAPISupported(msg, sender)
   {
     port.off("safari.legacyAPISupported", onLegacyAPISupported);
     resolve(msg.legacyAPISupported);
   }
   port.on("safari.legacyAPISupported", onLegacyAPISupported);
 });
 let contentBlockingActive = false;
 let afterContentBlockingFinished = null;
 let contentBlockListDirty = true;
-let lastSetContentBlockerResult;
+let lastSetContentBlockerError;
 
 function clearBlockCounters()
 {
   ext.pages.query({}, pages =>
   {
     for (let page of pages)
       page.browserAction.setBadge();
   });
 }
 
 function setContentBlocker(callback)
 {
-  // setContentBlocker returns null if given the same blocklist as last time,
-  // even when there was an error. (It's also wasteful to re-generate the
-  // blocklist when nothing has changed!)
+  // 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)
   {
-    callback(lastSetContentBlockerResult);
+    callback(lastSetContentBlockerError);
     return;
   }
 
   let contentBlockerList = new ContentBlockerList();
   for (let subscription of FilterStorage.subscriptions)
     if (!subscription.disabled)
       for (let filter of subscription.filters)
         contentBlockerList.addFilter(filter);
 
   contentBlockListDirty = false;
   safari.extension.setContentBlocker(
-    // setContentBlocker seems to not work in Safari 9 when a callback is passed
-    // unless the rules are converted to JSON first. (An error is thrown:
-    // "Extension compilation failed: Failed to parse the JSON String.")
+    // 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 (result)

[Sebastian Noack, 2016/05/17 18:35:25]

Nit: We usually don't put a space before the arguments list.

[Sebastian Noack, 2016/05/17 18:35:25]

Nit: I think this variable should be called "error".

[kzar, 2016/05/17 19:20:22]

Done.

[kzar, 2016/05/17 19:20:22]

Done.

+    function(error)
     {
-      // Safari 9 performs the callback twice under some conditions, first with

[kzar, 2016/05/17 17:13:06]

As far as I can tell the behaviour of the callback is something like this:

 - If callback is provided and rules aren't converted to JSON the callback will
first be called with an empty string "" and then an Error. Further calls will
result in a callback of "" followed by a callback of null.
 - If the callback is provided and the rules are converted to JSON the callback
will be performed with null on success or Error on error. Further calls with the
same rule string will always result in a callback of null until a call is
performed with a different JSON string. (Even if there was a problem.)
 - If the callback isn't provided things generally work as expected, but there's
no longer a way to verify that!

My only other idea to further improve how things work is to add a flag
`buggySetContentBlocker` that defaults to false. When the callback is performed
with the value of "" the flag is set to true and we start converting the passed
rules to JSON. This adds yet more code however and could make things more
fragile. (It also would mean we are generating the content list twice in a row
for buggy versions and it's already pretty slow when generating it once.)

[Sebastian Noack, 2016/05/17 18:35:25]

Well, if future versions of Safari won't have that bug, while we still have to
target versions with that bug, I suppose we should do something like that.
However, when doing so we can simply keep the list of rules around until the
callback get's called rather than generating it twice. But I agree, that for now
we should leave it like that.

[kzar, 2016/05/17 19:20:22]

Acknowledged.

-      // an empty string and then with an Error!
-      if (result == "")
+      if (error == "")
         return;
 
-      lastSetContentBlockerResult = result;
-      callback(result);
+      lastSetContentBlockerError = error;
+      callback(error);
     }
   );
 }
 
 function updateContentBlocker(isStartup, legacyAPISupported)
 {
   afterContentBlockingFinished = new Promise(resolve =>
   {
-    setContentBlocker(result =>
+    setContentBlocker(error =>
     {
-      if (result instanceof Error)
+      if (error instanceof 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(result.message);
+          alert(error.message);
       }
       else if (!contentBlockingActive)
       {
         contentBlockingActive = true;
         clearBlockCounters();
       }
 
       resolve(contentBlockingActive);
       afterContentBlockingFinished = null;
     });
@@ skipping 27 matching lines @@
     });
   });
 }
 
 port.on("safari.contentBlockingActive", (msg, sender) =>
 {
   if (!contentBlockingActive && afterContentBlockingFinished)
     return afterContentBlockingFinished;
   return contentBlockingActive;
 });