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

lib/abp2blocklist.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 62 matching lines @@
   {
     if (name.length > suffixLength && name.slice(-suffixLength) == "." + domain)
       subdomains.push(name.slice(0, -suffixLength));
   }
 
   return subdomains;
 }
 
 function extractFilterDomains(filters)
 {
-  let domains = [];
+  let domains = new Set();
   for (let filter of filters)
   {
     let parsed = parseFilterRegexpSource(filter.regexpSource);
     if (parsed.justHostname)
-      domains.push(parsed.hostname);
+      domains.add(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);
 
@@ skipping 13 matching lines @@
  *                   case, a hostname string (or undefined) and a bool
  *                   indicating if the source only contains a hostname or not:
  *                     {regexp: "...",
  *                      canSafelyMatchAsLowercase: true/false,
  *                      hostname: "...",
  *                      justHostname: true/false}
  */
 function parseFilterRegexpSource(text)
 {
   let regexp = [];
-  let lastIndex = text.length - 1;
+
+  // Convert the text into an array of Unicode characters.
+  //
+  // In the case of surrogate pairs (the smiley emoji, for example), one
+  // Unicode code point is represented by two JavaScript characters together.
+  // We want to iterate over Unicode code points rather than JavaScript
+  // characters.
+  let characters = Array.from(text);
+
+  let lastIndex = characters.length - 1;
   let hostname;
   let hostnameStart = null;
   let hostnameFinished = false;
   let justHostname = false;
   let canSafelyMatchAsLowercase = false;
 
-  for (let i = 0; i < text.length; i++)
-  {
-    let c = text[i];
+  for (let i = 0; i < characters.length; i++)
+  {
+    let c = characters[i];
 
     if (hostnameFinished)
       justHostname = false;
 
     // If we're currently inside the hostname we have to be careful not to
     // escape any characters until after we have converted it to punycode.
     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).toLowerCase()

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

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

+        characters.slice(hostnameStart, endingChar ? i : i + 1).join("")

[Manish Jethani, 2017/07/12 08:59:56]

This was the only conflict during rebase, since text (String) changed to
characters (Array) in another commit.

+                  .toLowerCase()
       );
       hostnameFinished = justHostname = true;
       regexp.push(escapeRegExp(hostname));
       if (!endingChar)
         break;
     }
 
     switch (c)
     {
       case "*":
-        if (regexp.length > 0 && i < lastIndex && text[i + 1] != "*")
+        if (regexp.length > 0 && i < lastIndex && characters[i + 1] != "*")
           regexp.push(".*");
         break;
       case "^":
         if (i < lastIndex)
           regexp.push(".");
         break;
       case "|":
         if (i == 0)
         {
           regexp.push("^");
           break;
         }
         if (i == lastIndex)
         {
           regexp.push("$");
           break;
         }
-        if (i == 1 && text[0] == "|")
+        if (i == 1 && characters[0] == "|")
         {
           hostnameStart = i + 1;
           canSafelyMatchAsLowercase = true;
           regexp.push("https?://([^/]+\\.)?");
           break;
         }
         regexp.push("\\|");
         break;
       case "/":
         if (!hostnameFinished &&
-            text.charAt(i-2) == ":" && text.charAt(i-1) == "/")
+            characters[i - 2] == ":" && characters[i - 1] == "/")
         {
           hostnameStart = i + 1;
           canSafelyMatchAsLowercase = true;
         }
         regexp.push("/");
         break;
       case ".": case "+": case "$": case "?":
       case "{": case "}": case "(": case ")":
       case "[": case "]": case "\\":
         regexp.push("\\", c);
         break;
       default:
         if (hostnameFinished && (c >= "a" && c <= "z" ||
                                  c >= "A" && c <= "Z"))
           canSafelyMatchAsLowercase = false;
-        regexp.push(c);
+        regexp.push(c == "%" ? c : encodeURI(c));
     }
   }
 
   return {
     regexp: regexp.join(""),
     canSafelyMatchAsLowercase: canSafelyMatchAsLowercase,
     hostname: hostname,
     justHostname: justHostname
   };
 }
@@ skipping 66 matching lines @@
   let included = [];
   let excluded = [];
 
   parseDomains(filter.domains, included, excluded);
 
   if (exceptionDomains)
     excluded = excluded.concat(exceptionDomains);
 
   if (withResourceTypes)
   {
-    trigger["resource-type"] = getResourceTypes(filter);
-
-    if (trigger["resource-type"].length == 0)
+    let resourceTypes = getResourceTypes(filter);
+
+    // Content blocker rules can't differentiate between sub-document requests
+    // (iframes) and top-level document requests. To avoid too many false
+    // positives, we prevent rules with no hostname part from blocking document
+    // requests.
+    //
+    // Once Safari 11 becomes our minimum supported version, we could change
+    // our approach here to use the new "unless-top-url" property instead.
+    if (filter instanceof filterClasses.BlockingFilter && !parsed.hostname)
+      resourceTypes = resourceTypes.filter(type => type != "document");
+
+    if (resourceTypes.length == 0)
       return;
+
+    trigger["resource-type"] = resourceTypes;
   }
 
   if (filter.thirdParty != null)
     trigger["load-type"] = [filter.thirdParty ? "third-party" : "first-party"];
 
   if (included.length > 0)
   {
     trigger["if-domain"] = [];
 
     for (let name of included)
@@ skipping 15 matching lines @@
       {
         trigger["if-domain"].push("*" + name);
       }
     }
   }
   else if (excluded.length > 0)
   {
     trigger["unless-domain"] = excluded.map(name => "*" + name);
   }
   else if (filter instanceof filterClasses.BlockingFilter &&
-           filter.contentType & typeMap.SUBDOCUMENT)
-  {
+           filter.contentType & typeMap.SUBDOCUMENT && parsed.hostname)
+  {
+    // Rules with a hostname part are still allowed to block document requests,
+    // but we add an exception for top-level documents.
+    //
+    // Note that we can only do this if there's no "unless-domain" property for
+    // now. This also only works in Safari 11 onwards, while older versions
+    // simply ignore this property. Once Safari 11 becomes our minimum
+    // supported version, we can merge "unless-domain" into "unless-top-url".
     trigger["unless-top-url"] = [trigger["url-filter"]];
     if (trigger["url-filter-is-case-sensitive"])
       trigger["top-url-filter-is-case-sensitive"] = true;
   }
 
   rules.push({trigger: trigger, action: {type: action}});
-}
-
-function hasNonASCI(obj)
-{
-  if (typeof obj == "string")
-  {
-    if (/[^\x00-\x7F]/.test(obj))
-      return true;
-  }
-
-  if (typeof obj == "object")
-  {
-    if (obj instanceof Array)
-      for (let item of obj)
-        if (hasNonASCI(item))
-          return true;
-
-    let names = Object.getOwnPropertyNames(obj);
-    for (let name of names)
-      if (hasNonASCI(obj[name]))
-        return true;
-  }
-
-  return false;
 }
 
 function convertIDSelectorsToAttributeSelectors(selector)
 {
   // First we figure out where all the IDs are
   let sep = "";
   let start = null;
   let positions = [];
   for (let i = 0; i < selector.length; i++)
   {
@@ skipping 35 matching lines @@
     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, exceptionDomains)
 {
-  exceptionDomains = Array.from(new Set(exceptionDomains));

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

Ensure no duplicates.

+  let unlessDomain = exceptionDomains.size > 0 ? [] : null;
+
+  exceptionDomains.forEach(name => unlessDomain.push("*" + name));
 
   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);
 
     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.

+    if (unlessDomain)
+      rule.trigger["unless-domain"] = unlessDomain;
 
     rules.push(rule);
   }
 }
 
 let ContentBlockerList =
 /**
  * Create a new Adblock Plus filter to content blocker list converter
  *
  * @constructor
@@ skipping 80 matching lines @@
       {
         let group = groupedElemhideFilters.get(matchDomain) || [];
         group.push(result.selector);
         groupedElemhideFilters.set(matchDomain, group);
       }
     }
   }
 
   // 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.
+  // https://bugs.webkit.org/show_bug.cgi?id=167423
   //
   // 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 =
+  let elemhideExceptionDomains = extractFilterDomains(this.elemhideExceptions);
+
+  let genericSelectorExceptionDomains =
     extractFilterDomains(this.generichideExceptions);
-  let elemhideExceptionDomains = extractFilterDomains(this.elemhideExceptions);
+  elemhideExceptionDomains.forEach(name =>
+  {
+    genericSelectorExceptionDomains.add(name);
+  });
 
   addCSSRules(rules, genericSelectors, "^https?://",
-              generichideExceptionDomains.concat(elemhideExceptionDomains));
+              genericSelectorExceptionDomains);
 
   groupedElemhideFilters.forEach((selectors, matchDomain) =>
   {
     addCSSRules(rules, selectors, matchDomain, elemhideExceptionDomains);
   });
 
   let requestFilterExceptionDomains = [];
   for (let filter of this.genericblockExceptions)
   {
     let parsed = parseFilterRegexpSource(filter.regexpSource);
     if (parsed.hostname)
       requestFilterExceptionDomains.push(parsed.hostname);
   }
 
   for (let filter of this.requestFilters)
   {
     convertFilterAddRules(rules, filter, "block", true,
                           requestFilterExceptionDomains);
   }
 
   for (let filter of this.requestExceptions)
     convertFilterAddRules(rules, filter, "ignore-previous-rules", true);
 
-  return rules.filter(rule => !hasNonASCI(rule));
+  return rules;
 };