Issue 5438 - Observer DOM changes to reapply filters.

chrome/content/elemHideEmulation.js

Index: chrome/content/elemHideEmulation.js
===================================================================
--- a/chrome/content/elemHideEmulation.js
+++ b/chrome/content/elemHideEmulation.js
@@ -15,16 +15,17 @@
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* globals filterToRegExp */
 
 "use strict";
 
 const MIN_INVOCATION_INTERVAL = 3000;
+const MAX_SYNCHRONOUS_PROCESSING_TIME = 50;
 const abpSelectorRegexp = /:-abp-([\w-]+)\(/i;
 
 function splitSelector(selector)
 {
   if (selector.indexOf(",") == -1)
     return [selector];
 
   let selectors = [];
@@ -70,16 +71,18 @@
   for (let i = 0; i < children.length; i++)
     if (children[i] == node)
       return i + 1;
   return 0;
 }
 
 function makeSelector(node, selector)
 {
+  if (node == null)
+    return null;
   if (!node.parentElement)
   {
     let newSelector = ":root";
     if (selector)
       newSelector += " > " + selector;
     return newSelector;
   }
   let idx = positionInParent(node);
@@ -159,19 +162,31 @@
 
 function* evaluate(chain, index, prefix, subtree, styles)
 {
   if (index >= chain.length)
   {
     yield prefix;
     return;
   }
+  let count = 0;
   for (let [selector, element] of
        chain[index].getSelectors(prefix, subtree, styles))
-    yield* evaluate(chain, index + 1, selector, element, styles);
+  {
+    count++;
+    if (selector == null)
+      yield null;
+    else
+      yield* evaluate(chain, index + 1, selector, element, styles);
+  }
+  // Just in case the getSelectors() generator above had to run some heavy
+  // document.querySelectorAll() call which didn't produce any results, make
+  // sure there is at least one point where execution can pause.
+  if (count == 0)
+    yield null;

[Wladimir Palant, 2017/08/15 11:40:11]

I still don't think we need that counter - we can simply yield unconditionally
here.

[hub, 2017/08/15 16:39:09]

Done.

 }
 
 function PlainSelector(selector)
 {
   this._selector = selector;
 }
 
 PlainSelector.prototype = {
@@ -221,21 +236,27 @@
     let actualPrefix = (!prefix || incompletePrefixRegexp.test(prefix)) ?
         prefix + "*" : prefix;
     let elements = subtree.querySelectorAll(actualPrefix);
     for (let element of elements)
     {
       let iter = evaluate(this._innerSelectors, 0, "", element, styles);
       for (let selector of iter)
       {
+        if (selector == null)
+        {
+          yield null;
+          continue;
+        }
         if (relativeSelector.test(selector))
           selector = ":scope" + selector;
         if (element.querySelector(selector))
           yield element;
       }
+      yield null;
     }
   }
 };
 
 function ContainsSelector(textContent)
 {
   this._text = textContent;
 }
@@ -249,19 +270,24 @@
       yield [makeSelector(element, ""), subtree];
   },
 
   *getElements(prefix, subtree, stylesheet)
   {
     let actualPrefix = (!prefix || incompletePrefixRegexp.test(prefix)) ?
         prefix + "*" : prefix;
     let elements = subtree.querySelectorAll(actualPrefix);
+
     for (let element of elements)
+    {
       if (element.textContent.includes(this._text))
         yield element;
+      else
+        yield null;
+    }
   }
 };
 
 function PropsSelector(propertyExpression)
 {
   let regexpString;
   if (propertyExpression.length >= 2 && propertyExpression[0] == "/" &&
       propertyExpression[propertyExpression.length - 1] == "/")
@@ -301,23 +327,30 @@
 
   *getSelectors(prefix, subtree, styles)
   {
     for (let selector of this.findPropsSelectors(styles, prefix, this._regexp))
       yield [selector, subtree];
   }
 };
 
+function isSelectorHidingOnlyPattern(pattern)
+{
+  return pattern.selectors.some(s => s.preferHideWithSelector) &&
+    !pattern.selectors.some(s => s.requiresHiding);
+}
+
 function ElemHideEmulation(window, getFiltersFunc, addSelectorsFunc,
                            hideElemsFunc)
 {
   this.window = window;
   this.getFiltersFunc = getFiltersFunc;
   this.addSelectorsFunc = addSelectorsFunc;
   this.hideElemsFunc = hideElemsFunc;
+  this.observer = new window.MutationObserver(this.observe.bind(this));
 }
 
 ElemHideEmulation.prototype = {
   isSameOrigin(stylesheet)
   {
     try
     {
       return new URL(stylesheet.href).origin == this.window.location.origin;
@@ -399,30 +432,31 @@
 
   /**
    * Processes the current document and applies all rules to it.
    * @param {CSSStyleSheet[]} [stylesheets]
    *    The list of new stylesheets that have been added to the document and
    *    made reprocessing necessary. This parameter shouldn't be passed in for
    *    the initial processing, all of document's stylesheets will be considered
    *    then and all rules, including the ones not dependent on styles.
+   * @param {function} [done]
+   *    Callback to call when done.
    */
-  addSelectors(stylesheets)
+  addSelectors(stylesheets, done)

[Wladimir Palant, 2017/08/15 11:40:10]

Please rename into _addSelectors() - this is a private function now, only to be
called from queueFiltering().

[hub, 2017/08/15 16:39:09]

Done.

   {
     this._lastInvocation = Date.now();

[Wladimir Palant, 2017/08/15 11:40:11]

This still needs to be set when addSelectors is done, not when it starts. Given
that it is being used in queueFiltering(), it makes sense to set this value in
the callback there.

[hub, 2017/08/15 16:39:09]

Done.

 
     let selectors = [];
     let selectorFilters = [];
 
     let elements = [];
     let elementFilters = [];
 
     let cssStyles = [];
 
-    let stylesheetOnlyChange = !!stylesheets;
     if (!stylesheets)

[Wladimir Palant, 2017/08/15 11:40:11]

Please put the stylesheetOnlyChange logic back, it still makes sense. It should
be set to !!stylesheets as before however.

[hub, 2017/08/15 16:39:10]

Done.

       stylesheets = this.window.document.styleSheets;
 
     // Chrome < 51 doesn't have an iterable StyleSheetList
     // https://issues.adblockplus.org/ticket/5381
     for (let i = 0; i < stylesheets.length; i++)
     {
       let stylesheet = stylesheets[i];
@@ -440,72 +474,163 @@
         if (rule.type != rule.STYLE_RULE)
           continue;
 
         cssStyles.push(stringifyStyle(rule));
       }
     }
 
     let {document} = this.window;
-    for (let pattern of this.patterns)
+
+    let patterns = this.patterns.slice();
+    let pattern = null;
+    let generator = null;
+
+    let processPatterns = () =>
     {
-      if (stylesheetOnlyChange &&
-          !pattern.selectors.some(selector => selector.dependsOnStyles))
-      {
-        continue;
-      }
+      let cycleStart = Date.now();
 
-      for (let selector of evaluate(pattern.selectors,
-                                    0, "", document, cssStyles))
+      if (!pattern)
       {
-        if (pattern.selectors.some(s => s.preferHideWithSelector) &&
-            !pattern.selectors.some(s => s.requiresHiding))
+        if (!patterns.length)
         {
-          selectors.push(selector);
-          selectorFilters.push(pattern.text);
+          this.addSelectorsFunc(selectors, selectorFilters);
+          this.hideElemsFunc(elements, elementFilters);
+          if (typeof done == "function")
+            done();
+          return;
         }
-        else
+
+        pattern = patterns.shift();
+        generator = evaluate(pattern.selectors, 0, "", document, cssStyles);
+      }
+      for (let selector of generator)
+      {
+        if (selector != null)
         {
-          for (let element of document.querySelectorAll(selector))
+          if (isSelectorHidingOnlyPattern(pattern))
           {
-            elements.push(element);
-            elementFilters.push(pattern.text);
+            selectors.push(selector);
+            selectorFilters.push(pattern.text);
+          }
+          else
+          {
+            for (let element of document.querySelectorAll(selector))
+            {
+              elements.push(element);
+              elementFilters.push(pattern.text);
+            }
           }
         }
       }
-    }
+      if (Date.now() - cycleStart > MAX_SYNCHRONOUS_PROCESSING_TIME)

[Wladimir Palant, 2017/08/15 11:40:11]

This check needs to be inside the loop above, so that we can pause while looping
through a generator, not merely when done with it.

[hub, 2017/08/15 16:39:09]

Done.

+      {
+        this.window.setTimeout(processPatterns, 0);
+        return;
+      }
+      pattern = null;
+      return processPatterns();
+    };
 
-    this.addSelectorsFunc(selectors, selectorFilters);
-    this.hideElemsFunc(elements, elementFilters);
+    processPatterns();
   },
 
-  _stylesheetQueue: null,
+  _filteringInProgress: false,
+  _scheduledProcessing: null,
+
+  /**
+   * Filtering reason
+   * @typedef {Object} FilteringReason
+   * @property {CSSStyleSheet[]} [stylesheets]
+   *    The list of new stylesheets that have been added to the document
+   *    and made reprocessing necessary.
+   */
+
+  /**
+   * Re-run filtering either immediately or queued.
+   * @param {FilteringReason} reason why the filtering must be queued.

[Wladimir Palant, 2017/08/15 11:40:11]

We don't really need this to be a FilteringReason structure any more, an
optional stylesheets parameter will do as well.

[hub, 2017/08/15 16:39:09]

Done.

+   */
+  queueFiltering(reason)
+  {
+    let completion = () =>
+    {
+      this._filteringInProgress = false;
+      if (this._scheduledProcessing)
+      {
+        let nextReason = this._scheduledProcessing;
+        this._scheduledProcessing = null;
+        this.queueFiltering(nextReason);
+      }
+    };
+
+    if (this._scheduledProcessing)
+    {
+      if (reason.stylesheets)
+      {
+        if (this._scheduledProcessing.stylesheets)
+          this._scheduledProcessing.stylesheets.push(...reason.stylesheets);
+        else
+          this._scheduledProcessing.stylesheets = reason.stylesheets;
+      }

[Wladimir Palant, 2017/08/15 11:40:11]

This logic is wrong. this._scheduledProcessing.stylesheets should always be null
if we've seen DOM modifications, regardless of any stylesheets that loaded.
Meaning that we have three scenarios here:

1. this._scheduledProcessing.stylesheets is already null (we've seen DOM
modifications) - nothing to do, we need a full reprocessing.
2. this._scheduledProcessing.stylesheets is non-null but reason.stylesheets is
null - this is a DOM modification so make sure
this._scheduledProcessing.stylesheets is null.
3. Both this._scheduledProcessing.stylesheets and reason.stylesheets are
non-null - we've only seen new stylesheets so far, merge them.

[hub, 2017/08/15 16:39:10]

Done.

+    }
+    else if (this._filteringInProgress)
+    {
+      this._scheduledProcessing = reason;
+    }
+    else if (Date.now() - this._lastInvocation < MIN_INVOCATION_INTERVAL)
+    {
+      this._scheduledProcessing = reason;
+      this.window.setTimeout(() =>
+      {
+        let stylesheets = this._scheduledProcessing.stylesheets || [];

[Wladimir Palant, 2017/08/15 11:40:11]

The `|| []` part here means that we will never do a full reprocessing, only
process the additional stylesheets (if any). We should really pass in null if
this._scheduledProcessing.stylesheets is null.

[hub, 2017/08/15 16:39:10]

Done.

+        this._filteringInProgress = true;
+        this._scheduledProcessing = null;
+        this.addSelectors(stylesheets, completion);
+      }, MIN_INVOCATION_INTERVAL - (Date.now() - this._lastInvocation));
+    }
+    else
+    {
+      this._filteringInProgress = true;
+      this.addSelectors(reason.stylesheets, completion);
+    }
+  },
 
   onLoad(event)
   {
     let stylesheet = event.target.sheet;
     if (stylesheet)
+      this.queueFiltering({stylesheets: [stylesheet]});
+  },
+
+  observe(mutations)
+  {
+    let reason = {};
+    let stylesheets = [];

[Wladimir Palant, 2017/08/15 11:40:11]

stylesheets should always be null for DOM modifications, making sure that we do
a full reprocessing.

[hub, 2017/08/15 16:39:10]

which is why line 626, we check for the array length before passing it to
queueFiltering().

But this code is changing anyway.

+    for (let mutation of mutations)
     {
-      if (!this._stylesheetQueue &&
-          Date.now() - this._lastInvocation < MIN_INVOCATION_INTERVAL)
+      if (mutation.type == "childList")
       {
-        this._stylesheetQueue = [];
-        this.window.setTimeout(() =>
+        for (let added of mutation.addedNodes)
         {
-          let stylesheets = this._stylesheetQueue;
-          this._stylesheetQueue = null;
-          this.addSelectors(stylesheets);
-        }, MIN_INVOCATION_INTERVAL - (Date.now() - this._lastInvocation));
+          if (added instanceof this.window.HTMLStyleElement &&
+              added.stylesheet)

[Wladimir Palant, 2017/08/15 11:40:10]

Please drop that special logic. A new <style> element is always a DOM
modification, potentially affecting :-abp-has() and :-abp-contains() rules. It
means that it warrants a full reprocessing and we don't need to go into the
"style changes only" shortcut.

[hub, 2017/08/15 16:39:09]

Done.

+            stylesheets.push(added.stylesheet);
+        }
       }
-
-      if (this._stylesheetQueue)
-        this._stylesheetQueue.push(stylesheet);
-      else
-        this.addSelectors([stylesheet]);
+      else if (mutation.type == "characterData")
+      {
+        let element = mutation.target.parentElement;
+        if (element instanceof this.window.HTMLStyleElement &&
+            element.stylesheet)
+          stylesheets.push(element.stylesheet);
+      }
     }
+    if (stylesheets.length > 0)
+      reason.stylesheets = stylesheets;
+    this.queueFiltering(reason);
   },
 
   apply()
   {
     this.getFiltersFunc(patterns =>
     {
       this.patterns = [];
       for (let pattern of patterns)
@@ -513,14 +638,23 @@
         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();
+        this.queueFiltering({});
+        this.observer.observe(
+          document,
+          {
+            childList: true,
+            attributes: true,
+            characterData: true,
+            subtree: true
+          }
+        );
         document.addEventListener("load", this.onLoad.bind(this), true);
       }
     });
   }
 };