Issue 5313 - Make error reporting more robust.

chrome/content/elemHideEmulation.js

Index: chrome/content/elemHideEmulation.js
===================================================================
--- a/chrome/content/elemHideEmulation.js
+++ b/chrome/content/elemHideEmulation.js
@@ -125,71 +125,16 @@
     }
   }
 
   if (parens > 0)
     return null;
   return {text: content.substring(startIndex, i), end: i};
 }
 
-/** Parse the selector
- * @param {string} selector the selector to parse
- * @return {Object} selectors is an array of objects,
- * or null in case of errors. hide is true if we'll hide
- * elements instead of styles..
- */
-function parseSelector(selector)
-{
-  if (selector.length == 0)
-    return [];
-
-  let match = abpSelectorRegexp.exec(selector);
-  if (!match)
-    return [new PlainSelector(selector)];
-
-  let selectors = [];
-  if (match.index > 0)
-    selectors.push(new PlainSelector(selector.substr(0, match.index)));
-
-  let startIndex = match.index + match[0].length;
-  let content = parseSelectorContent(selector, startIndex);
-  if (!content)
-  {
-    reportError(new SyntaxError("Failed to parse Adblock Plus " +
-                                `selector ${selector}, ` +
-                                "due to unmatched parentheses."));
-    return null;
-  }
-  if (match[1] == "properties")
-    selectors.push(new PropsSelector(content.text));
-  else if (match[1] == "has")
-  {
-    let hasSelector = new HasSelector(content.text);
-    if (!hasSelector.valid())
-      return null;
-    selectors.push(hasSelector);
-  }
-  else
-  {
-    // this is an error, can't parse selector.
-    reportError(new SyntaxError("Failed to parse Adblock Plus " +
-                                `selector ${selector}, invalid ` +
-                                `pseudo-class :-abp-${match[1]}().`));
-    return null;
-  }
-
-  let suffix = parseSelector(selector.substr(content.end + 1));
-  if (suffix == null)
-    return null;
-
-  selectors.push(...suffix);
-
-  return selectors;
-}
-
 /** Stringified style objects
  * @typedef {Object} StringifiedStyle
  * @property {string} style CSS style represented by a string.
  * @property {string[]} subSelectors selectors the CSS properties apply to.
  */
 
 /**
  * Produce a string representation of the stylesheet entry.
@@ -241,29 +186,24 @@
   *getSelectors(prefix, subtree, styles)
   {
     yield [prefix + this._selector, subtree];
   }
 };
 
 const incompletePrefixRegexp = /[\s>+~]$/;
 
-function HasSelector(selector)
+function HasSelector(selectors)
 {
-  this._innerSelectors = parseSelector(selector);
+  this._innerSelectors = selectors;
 }
 
 HasSelector.prototype = {
   requiresHiding: true,
 
-  valid()
-  {
-    return this._innerSelectors != null;
-  },
-
   *getSelectors(prefix, subtree, styles)
   {
     for (let element of this.getElements(prefix, subtree, styles))
       yield [makeSelector(element, ""), element];
   },
 
   /**
    * Generator function returning selected elements.
@@ -339,16 +279,73 @@
     }
     catch (e)
     {
       // Invalid URL, assume that it is first-party.
       return true;
     }
   },
 
+  /** Parse the selector
+   * @param {string} selector the selector to parse
+   * @return {Array} selectors is an array of objects,
+   * or null in case of errors.
+   */
+  parseSelector(selector)
+  {
+    if (selector.length == 0)
+      return [];
+
+    let match = abpSelectorRegexp.exec(selector);
+    if (!match)
+      return [new PlainSelector(selector)];
+
+    let selectors = [];
+    if (match.index > 0)
+      selectors.push(new PlainSelector(selector.substr(0, match.index)));
+
+    let startIndex = match.index + match[0].length;
+    let content = parseSelectorContent(selector, startIndex);
+    if (!content)
+    {
+      this.window.console.error(
+        new SyntaxError("Failed to parse Adblock Plus " +
+                        `selector ${selector}, ` +

[Wladimir Palant, 2017/06/19 11:22:37]

Nit: There should be no comma here, I vaguely remember noting that in the
previous review.

[hub, 2017/06/19 13:24:42]

Done.

+                        "due to unmatched parentheses."));
+      return null;
+    }
+    if (match[1] == "properties")
+      selectors.push(new PropsSelector(content.text));
+    else if (match[1] == "has")
+    {
+      let hasSelectors = this.parseSelector(content.text);
+      if (hasSelectors == null)
+        return null;
+      let hasSelector = new HasSelector(hasSelectors);
+      selectors.push(hasSelector);

[Wladimir Palant, 2017/06/19 11:22:37]

Nit: The temporary hasSelector variable no longer has a point, just push the
instance directly like for PropsSelector.

[hub, 2017/06/19 13:24:42]

Done.

+    }
+    else
+    {
+      // this is an error, can't parse selector.
+      this.window.console.error(
+        new SyntaxError("Failed to parse Adblock Plus " +
+                        `selector ${selector}, invalid ` +
+                        `pseudo-class :-abp-${match[1]}().`));
+      return null;
+    }
+
+    let suffix = this.parseSelector(selector.substr(content.end + 1));
+    if (suffix == null)
+      return null;
+
+    selectors.push(...suffix);
+
+    return selectors;
+  },
+
   addSelectors(stylesheets)
   {
     let selectors = [];
     let selectorFilters = [];
 
     let elements = [];
     let elementFilters = [];
 
@@ -406,29 +403,25 @@
     if (stylesheet)
       this.addSelectors([stylesheet]);
   },
 
   apply()
   {
     this.getFiltersFunc(patterns =>
     {
-      let oldReportError = reportError;
-      reportError = error => this.window.console.error(error);
-
       this.patterns = [];
       for (let pattern of patterns)
       {
-        let selectors = parseSelector(pattern.selector);
+        let selectors = this.parseSelector(pattern.selector);
         if (selectors != null && selectors.length > 0)
           this.patterns.push({selectors, text: pattern.text});
       }
 
       if (this.patterns.length > 0)
       {
         let {document} = this.window;
         this.addSelectors(document.styleSheets);
         document.addEventListener("load", this.onLoad.bind(this), true);
       }
-      reportError = oldReportError;
     });
   }
 };