Issue 5345 - Whitelist $elemhide and $generichide domains where possible

lib/abp2blocklist.js

Index: lib/abp2blocklist.js
===================================================================
--- a/lib/abp2blocklist.js
+++ b/lib/abp2blocklist.js
@@ -73,16 +73,28 @@
   {
     if (name.length > suffixLength && name.slice(-suffixLength) == "." + domain)
       subdomains.push(name.slice(0, -suffixLength));
   }
 
   return subdomains;
 }
 
+function extractFilterDomains(filters)
+{
+  let domains = [];
+  for (let filter of filters)
+  {
+    let parsed = parseFilterRegexpSource(filter.regexpSource);
+    if (parsed.justHostname)
+      domains.push(parsed.hostname);
+  }
+  return domains;

[kzar, 2017/07/07 11:40:13]

Why not make domains a Set instead of an Array here? Then we wouldn't have to
keep converting it later on.

[Manish Jethani, 2017/07/08 05:33:59]

That's a good point, it does seem to make a huge difference.

Done.

+}
+
 function convertElemHideFilter(filter, elemhideSelectorExceptions)
 {
   let included = [];
   let excluded = [];
   let rules = [];
 
   parseDomains(filter.domains, included, excluded);
 
@@ -128,17 +140,17 @@
     if (hostnameStart != null && !hostnameFinished)
     {
       let endingChar = (c == "*" || c == "^" ||
                         c == "?" || c == "/" || c == "|");
       if (!endingChar && i != lastIndex)
         continue;
 
       hostname = punycode.toASCII(
-        text.substring(hostnameStart, endingChar ? i : i + 1)
+        text.substring(hostnameStart, endingChar ? i : i + 1).toLowerCase()

[Manish Jethani, 2017/06/24 14:54:15]

punycode.toASCII doesn't lower-case the string, we have to do it ourselves.

       );
       hostnameFinished = justHostname = true;
       regexp.push(escapeRegExp(hostname));
       if (!endingChar)
         break;
     }
 
     switch (c)
@@ -395,32 +407,39 @@
     newSelector.push('[id=', selector.substring(pos.start + 1, pos.end), ']');
     i = pos.end;
   }
   newSelector.push(selector.substring(i));
 
   return newSelector.join("");
 }
 
-function addCSSRules(rules, selectors, matchDomain)
+function addCSSRules(rules, selectors, matchDomain, exceptionDomains)
 {
+  exceptionDomains = Array.from(new Set(exceptionDomains));

[Manish Jethani, 2017/06/24 14:54:15]

Ensure no duplicates.

+
   while (selectors.length)
   {
     let selector = selectors.splice(0, selectorLimit).join(", ");
 
     // As of Safari 9.0 element IDs are matched as lowercase. We work around
     // this by converting to the attribute format [id="elementID"]
     selector = convertIDSelectorsToAttributeSelectors(selector);
 
-    rules.push({
+    let rule = {
       trigger: {"url-filter": matchDomain,
                 "url-filter-is-case-sensitive": true},
       action: {type: "css-display-none",
                selector: selector}
-    });
+    };
+
+    if (exceptionDomains.length > 0)
+      rule.trigger["unless-domain"] = exceptionDomains.map(name => "*" + name);

[kzar, 2017/07/07 11:40:13]

Maybe we should do this work outside of the while loop?

[Manish Jethani, 2017/07/08 05:33:59]

We have to make a copy of the array as a rule, because later when we implement
rule merging we want to be able to modify one rule object without it affecting
other rule objects. If this turns out not to be a concern in practice for this
particular case, then we can optimize here.

I actually tried moving it outside the while loop but then calling [].slice on
it for each rule, it did not make any difference in performance.

[kzar, 2017/07/10 12:33:07]

I'd rather you did the work outside the loop here for now, then moved it if
required for the merging patchset.

[Manish Jethani, 2017/07/11 11:19:18]

Done.

+
+    rules.push(rule);
   }
 }
 
 let ContentBlockerList =
 /**
  * Create a new Adblock Plus filter to content blocker list converter
  *
  * @constructor
@@ -507,31 +526,39 @@
       {
         let group = groupedElemhideFilters.get(matchDomain) || [];
         group.push(result.selector);
         groupedElemhideFilters.set(matchDomain, group);
       }
     }
   }
 
-  addCSSRules(rules, genericSelectors, "^https?://");
+  // Separate out the element hiding exceptions that have only a hostname part
+  // from the rest. This allows us to implement a workaround for issue #5345
+  // (WebKit bug #167423), but as a bonus it also reduces the number of

[kzar, 2017/07/07 11:40:13]

Mind giving the full URL for the WebKit bug?

[Manish Jethani, 2017/07/08 05:33:59]

Done.

By the way, there are new comments on that bug report. Maybe we have
misunderstood how css-display-none and ignore-previous-rules are supposed to
work. I'm going to post an update there that definitely shows that this is
broken in WebKit.

[kzar, 2017/07/10 12:33:08]

Acknowledged.

+  // generated rules. The downside is that the exception will only apply to the
+  // top-level document, not to iframes. We have to live with this until the
+  // WebKit bug is fixed in all supported versions of Safari.
+  //
+  // Note that as a result of this workaround we end up with a huge rule set in
+  // terms of the amount of memory used. This can cause Node.js to throw

[kzar, 2017/07/07 11:40:13]

Have you tested that rule generation still works OK with Adblock Plus for Safari
when this change is included? It kinda sucks that we'll be using a bunch more
memory there.

[Manish Jethani, 2017/07/08 05:33:59]

I tested it there and it works without problems.

[kzar, 2017/07/10 12:33:07]

Acknowledged.

+  // "JavaScript heap out of memory". To avoid this, call Node.js with
+  // --max_old_space_size=4096
+  let generichideExceptionDomains =
+    extractFilterDomains(this.generichideExceptions);
+  let elemhideExceptionDomains = extractFilterDomains(this.elemhideExceptions);
 
-  // Right after the generic element hiding filters, add the exceptions that

[Manish Jethani, 2017/06/24 14:54:15]

We could continue generating individual rules for these as well, but since it's
not working anyway we might as well drop these for now. When the WebKit bug is
fixed, we should go back to this.

[kzar, 2017/07/07 11:40:13]

Maybe add a comment explaining what needs to change in the future once the bug's
fixed?

-  // should apply only to those filters.
-  for (let filter of this.generichideExceptions)
-    convertFilterAddRules(rules, filter, "ignore-previous-rules", false);
+  addCSSRules(rules, genericSelectors, "^https?://",
+              generichideExceptionDomains.concat(elemhideExceptionDomains));
 
   groupedElemhideFilters.forEach((selectors, matchDomain) =>
   {
-    addCSSRules(rules, selectors, matchDomain);
+    addCSSRules(rules, selectors, matchDomain, elemhideExceptionDomains);
   });
 
-  for (let filter of this.elemhideExceptions)
-    convertFilterAddRules(rules, filter, "ignore-previous-rules", false);
-
   let requestFilterExceptionDomains = [];
   for (let filter of this.genericblockExceptions)
   {
     let parsed = parseFilterRegexpSource(filter.regexpSource);
     if (parsed.hostname)
       requestFilterExceptionDomains.push(parsed.hostname);
   }