Issue 5395 - Make sure element hiding emulation doesn't degrade website performance too much

chrome/content/elemHideEmulation.js

Index: chrome/content/elemHideEmulation.js
===================================================================
--- a/chrome/content/elemHideEmulation.js
+++ b/chrome/content/elemHideEmulation.js
@@ -14,16 +14,17 @@
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* globals filterToRegExp */
 
 "use strict";
 
+const MIN_INVOCATION_INTERVAL = 3000;
 const abpSelectorRegexp = /:-abp-([\w-]+)\(/i;
 
 let reportError = () => {};
 
 function splitSelector(selector)
 {
   if (selector.indexOf(",") == -1)
     return [selector];
@@ -194,16 +195,21 @@ const incompletePrefixRegexp = /[\s>+~]$
 function HasSelector(selectors)
 {
   this._innerSelectors = selectors;
 }
 
 HasSelector.prototype = {
   requiresHiding: true,
 
+  get dependsOnStyles()
+  {
+    return this._innerSelectors.some(selector => selector.dependsOnStyles);
+  },
+
   *getSelectors(prefix, subtree, styles)
   {
     for (let element of this.getElements(prefix, subtree, styles))
       yield [makeSelector(element, ""), element];
   },
 
   /**
    * Generator function returning selected elements.
@@ -242,16 +248,17 @@ function PropsSelector(propertyExpressio
   else
     regexpString = filterToRegExp(propertyExpression);
 
   this._regexp = new RegExp(regexpString, "i");
 }
 
 PropsSelector.prototype = {
   preferHideWithSelector: true,
+  dependsOnStyles: true,
 
   *findPropsSelectors(styles, prefix, regexp)
   {
     for (let style of styles)
       if (regexp.test(style.style))
         for (let subSelector of style.subSelectors)
           yield prefix + subSelector;
   },
@@ -337,26 +344,34 @@ ElemHideEmulation.prototype = {
     if (suffix == null)
       return null;
 
     selectors.push(...suffix);
 
     return selectors;
   },
 
+  _lastInvocation: 0,
+
   addSelectors(stylesheets)

[kzar, 2017/07/10 14:50:17]

Nit: IMO this function could do with a comment to explain the behaviour when the
stylesheets parameter is passed / omitted. It's true I'm not familiar with this
code but that aspect took me a while to grok when it's actually kinda simple.

[Wladimir Palant, 2017/07/11 08:18:11]

Done.

[kzar, 2017/07/11 11:05:14]

Nice, thanks.

   {
+    this._lastInvocation = Date.now();
+
     let selectors = [];
     let selectorFilters = [];
 
     let elements = [];
     let elementFilters = [];
 
     let cssStyles = [];
 
+    let stylesheetOnlyChange = !!stylesheets;
+    if (!stylesheets)
+      stylesheets = this.window.document.styleSheets;
+
     for (let stylesheet of stylesheets)
     {
       // Explicitly ignore third-party stylesheets to ensure consistent behavior
       // between Firefox and Chrome.
       if (!this.isSameOrigin(stylesheet))
         continue;
 
       let rules = stylesheet.cssRules;
@@ -370,16 +385,22 @@ ElemHideEmulation.prototype = {
 
         cssStyles.push(stringifyStyle(rule));
       }
     }
 
     let {document} = this.window;
     for (let pattern of this.patterns)
     {
+      if (stylesheetOnlyChange &&
+          !pattern.selectors.some(selector => selector.dependsOnStyles))
+      {
+        continue;
+      }
+
       for (let selector of evaluate(pattern.selectors,
                                     0, "", document, cssStyles))
       {
         if (pattern.selectors.some(s => s.preferHideWithSelector) &&
             !pattern.selectors.some(s => s.requiresHiding))
         {
           selectors.push(selector);
           selectorFilters.push(pattern.text);
@@ -394,21 +415,40 @@ ElemHideEmulation.prototype = {
         }
       }
     }
 
     this.addSelectorsFunc(selectors, selectorFilters);
     this.hideElemsFunc(elements, elementFilters);
   },
 
+  _stylesheetQueue: null,
+
   onLoad(event)
   {
     let stylesheet = event.target.sheet;
     if (stylesheet)
-      this.addSelectors([stylesheet]);
+    {
+      if (!this._stylesheetQueue &&
+          Date.now() - this._lastInvocation < MIN_INVOCATION_INTERVAL)
+      {
+        this._stylesheetQueue = [];
+        this.window.setTimeout(() =>
+        {
+          let stylesheets = this._stylesheetQueue;
+          this._stylesheetQueue = null;
+          this.addSelectors(stylesheets);

[Felix Dahlke, 2017/07/10 14:37:19]

Nit: I'm probably rusty, but couldn't the above three lines be rewritten as:

  this.addSelectors(this._stylesheetQueue);
  this._stylesheetQueue = null;

[Wladimir Palant, 2017/07/10 14:38:52]

The trouble here is reentrance - if addSelector() call somehow ends up calling
onLoad() we'll have an issue.

[Felix Dahlke, 2017/07/10 14:52:14]

Makes sense.

+        }, MIN_INVOCATION_INTERVAL - (Date.now() - this._lastInvocation));
+      }
+
+      if (this._stylesheetQueue)
+        this._stylesheetQueue.push(stylesheet);
+      else
+        this.addSelectors([stylesheet]);
+    }
   },
 
   apply()
   {
     this.getFiltersFunc(patterns =>
     {
       this.patterns = [];
       for (let pattern of patterns)
@@ -416,14 +456,14 @@ ElemHideEmulation.prototype = {
         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);
+        this.addSelectors();
         document.addEventListener("load", this.onLoad.bind(this), true);
       }
     });
   }
 };