Issue 3584 - Work around WebKit uppercase ID matching bug

abp2blocklist.js

 "use strict";
 
 let readline = require("readline");
 let punycode = require("punycode");
 let tldjs = require("tldjs");
 let filterClasses = require("./adblockplus.js");
 
 let typeMap = filterClasses.RegExpFilter.typeMap;
 
 const selectorLimit = 5000;
@@ skipping 236 matching lines @@
           return true;
 
     for (let name of Object.getOwnPropertyNames(obj))
       if (hasNonASCI(obj[name]))
         return true;
   }
 
   return false;
 }
 
-function attributeSyntaxForIDs(selector)

[Sebastian Noack, 2016/02/17 14:40:59]

Perhaps a better name for this function: convertIDSelectorsToAttributeSelectors

[kzar, 2016/02/17 15:13:52]

Done.

-{
-  if (selector.indexOf("#") == -1)

[Sebastian Noack, 2016/02/17 14:22:19]

Does this optimization has any measurable impact on the overall performance?

[kzar, 2016/02/17 15:13:52]

No, removed.

-    return selector;
-
+function convertIDSelectorsToAttributeSelectors(selector)
+{
   // First we figure out where all the IDs are
   let sep = "";
-  let IDstart = null;

[Sebastian Noack, 2016/02/17 14:40:59]

Nit: Since we only have one kind of start value and positions, omit the ID
prefix? It looks kinda ugly as it breaks the camel case notation.

[kzar, 2016/02/17 15:13:52]

Done.

-  let IDpositions = [];
+  let start = null;
+  let positions = [];
   for (let i = 0; i < selector.length; i++)
   {
     let chr = selector[i];
 
     if (chr == "\\")        // ignore escaped characters
       i++;
-    else if (chr == sep)    // don't split within quoted text
-      sep = "";             // e.g. [attr=","]
+    else if (chr == sep)    // don't match IDs within quoted text
+      sep = "";             // e.g. [attr="#Hello"]
     else if (sep == "")
     {
       if (chr == '"' || chr == "'")
         sep = chr;
-      else if (IDstart == null)  // look for the start of an ID
+      else if (start == null)  // look for the start of an ID
       {
         if (chr == "#")
-          IDstart = i;
+          start = i;
       }
-      else if (chr < "0" && chr != "-" ||
-               chr > "9" && chr < "A" ||
-               chr > "Z" && chr != "_" && chr < "a" ||
-               chr > "z" && chr < "\x80") // look for the end of the ID
+      else if (chr != "-" && chr != "_" &&
+               (chr < "0" ||
+                chr > "9" && chr < "A" ||
+                chr > "Z" && chr < "a" ||
+                chr > "z" && chr < "\x80")) // look for the end of the ID
       {
-        IDpositions.push({start: IDstart, end: i});

[Sebastian Noack, 2016/02/17 14:22:19]

I wonder we should do the replace inline:

selector = selector.substring(0, IDstart) +
           "[id=" + selector.substring(IDstart + 1, i) + "]" +
           selector.substring(i);
i += 4;

This logic would have to move to a function, to avoid duplication. Also we might
want to calculate the offset (4) rather than hardcoding it. But I could imagine
that it will still be simpler than the dance we do below, plus we would need
less state.

[Sebastian Noack, 2016/02/17 14:41:00]

Here is an example how that could look like:

  function replaceIDSubSelector(selector, start , end)
  {
    return selector.substring(0, start) +
           "[id=" + selector.substring(start + 1, end) + "]" +
           selector.substring(end);
  }

  ...

  let newSelector = replaceIDSubSelector(selector, IDStart, i);
  i += newSelector.length - selector.length;
  selector = newSelector;

[kzar, 2016/02/17 15:13:52]

Mind if I leave this as is? It works fine and I find it easier to understand how
it is.

-        IDstart = null;
+        positions.push({start: start, end: i});
+        start = null;
       }
     }
   }
-  if (IDstart != null)
-    IDpositions.push({start: IDstart, end: selector.length});
-
-  if (IDpositions.length == 0)

[Sebastian Noack, 2016/02/17 14:22:20]

Does this optimization has any measurable impact on the overall performance?

[kzar, 2016/02/17 15:13:52]

No, removed.

-    return selector;
+  if (start != null)
+    positions.push({start: start, end: selector.length});
 
   // Now replace them all with the [id="someID"] form
-  let newSelector = "";
-  let position = 0;
-  for (let ID of IDpositions)
-  {
-    newSelector += selector.substring(position, ID.start);
-    newSelector += '[id="' + selector.substring(ID.start + 1, ID.end) + '"]';

[Sebastian Noack, 2016/02/17 14:22:19]

The quotes are actually redundant, as we copy an unquoted token here, which
cannot possibly have any spaces.

[kzar, 2016/02/17 15:13:52]

Acknowledged.

-    position = ID.end;
-  }
-  newSelector += selector.substring(position);
-
-  return newSelector;
+  let newSelector = [];
+  let i = 0;
+  for (let pos of positions)
+  {
+    newSelector.push(selector.substring(i, pos.start));
+    newSelector.push('[id=' + selector.substring(pos.start + 1, pos.end) + ']');
+    i = pos.end;
+  }
+  newSelector.push(selector.substring(i));
+
+  return newSelector.join("");
 }
 
 function logRules()
 {
   let rules = [];
 
   function addRule(rule)
   {
     if (!hasNonASCI(rule))
       rules.push(rule);
@@ skipping 18 matching lines @@
   }
 
   groupedElemhideFilters.forEach((selectors, matchDomain) =>
   {
     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 = attributeSyntaxForIDs(selector);
+      selector = convertIDSelectorsToAttributeSelectors(selector);
 
       addRule({
         trigger: {"url-filter": matchDomain},
         action: {type: "css-display-none",
                  selector: selector}
       });
     }
   });
 
   for (let filter of elemhideExceptions)
     addRule(convertFilter(filter, "ignore-previous-rules", false));
 
   for (let filter of requestFilters)
     addRule(convertFilter(filter, "block", true));
   for (let filter of requestExceptions)
     addRule(convertFilter(filter, "ignore-previous-rules", true));
 
   console.log(JSON.stringify(rules, null, "\t"));
 }
 
 let rl = readline.createInterface({input: process.stdin, terminal: false});
 rl.on("line", parseFilter);
 rl.on("close", logRules);