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
@@ -70,16 +70,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 +161,32 @@
 
 function* evaluate(chain, index, prefix, subtree, styles)
 {
   if (index >= chain.length)
   {
     yield prefix;
     return;
   }
+  if (prefix == null)

[Wladimir Palant, 2017/08/10 10:12:20]

How would we get into this situation? From what I can tell, the only case where
evaluate() is being called with a non-constant prefix is right below - and you
already avoid passing in null by checking for `selector == null`.

[hub, 2017/08/11 16:26:51]

Acknowledged.

+  {
+    yield null;
+    return;
+  }
+  let count = 0;
   for (let [selector, element] of
        chain[index].getSelectors(prefix, subtree, styles))
+  {
+    if (selector == null)
+      continue;

[Wladimir Palant, 2017/08/10 10:12:21]

getSelectors() generator might be performing non-trivial work without producing
any non-null values, e.g. if it is a HasSelector instance. So we shouldn't be
just dropping any null values it yields but rather pass them through:

  if (selector == null)
    yield null;
  else
    yield* evaluate(...);

[hub, 2017/08/11 16:26:50]

Acknowledged.

+    count++;
     yield* evaluate(chain, index + 1, selector, element, styles);
+  }
+  if (count == 0)
+    yield null;

[Wladimir Palant, 2017/08/10 10:12:22]

I don't think we need this counter, doing this unconditionally should be fine.
But maybe add a comment explaining why this is necessary, it's not really
obvious. Something like this:

  // 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.
  yield null;

[hub, 2017/08/11 16:26:50]

Acknowledged.

 }
 
 function PlainSelector(selector)
 {
   this._selector = selector;
 }
 
 PlainSelector.prototype = {
@@ -216,26 +231,37 @@
    * @param {Node} subtree the subtree we work on.
    * @param {StringifiedStyle[]} styles the stringified style objects.
    */
   *getElements(prefix, subtree, styles)
   {
     let actualPrefix = (!prefix || incompletePrefixRegexp.test(prefix)) ?
         prefix + "*" : prefix;
     let elements = subtree.querySelectorAll(actualPrefix);
+    if (elements.length == 0)
+      yield null;

[Wladimir Palant, 2017/08/10 10:12:20]

We don't need to protect against this scenario more than once, it is already
accounted for in evaluate().

[hub, 2017/08/11 16:26:48]

Acknowledged.

     for (let element of elements)
     {
+      let count = 0;
       let iter = evaluate(this._innerSelectors, 0, "", element, styles);
       for (let selector of iter)
       {
+        count++;
+        if (selector == null)
+        {
+          yield null;
+          continue;
+        }
         if (relativeSelector.test(selector))
           selector = ":scope" + selector;
         if (element.querySelector(selector))
           yield element;
       }
+      if (count == 0)
+        yield null;

[Wladimir Palant, 2017/08/10 10:12:22]

Like above, we don't need the counter - an unconditional `yield null` should do.

[hub, 2017/08/11 16:26:50]

Acknowledged.

     }
   }
 };
 
 function ContainsSelector(textContent)
 {
   this._text = textContent;
 }
@@ -249,19 +275,30 @@
       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;
+    if (elements.length == 0)
+      yield null;

[Wladimir Palant, 2017/08/10 10:12:21]

We don't need to protect against this scenario more than once, it is already
accounted for in evaluate().

[hub, 2017/08/11 16:26:50]

Acknowledged.

+    else
+    {
+      let count = 0;
+      for (let element of elements)
+        if (element.textContent.includes(this._text))
+        {
+          count++;
+          yield element;
+        }

[Wladimir Palant, 2017/08/10 10:12:21]

We don't need this counter, just make sure to yield on each loop iteration:

  if (element.textContent.includes(this._text))
    yield element;
  else
    yield null;

Nit: multi-line for statement should have brackets around its body.

[hub, 2017/08/11 16:26:50]

Acknowledged.

+      if (count == 0)
+        yield null;
+    }
   }
 };
 
 function PropsSelector(propertyExpression)
 {
   let regexpString;
   if (propertyExpression.length >= 2 && propertyExpression[0] == "/" &&
       propertyExpression[propertyExpression.length - 1] == "/")
@@ -301,23 +338,30 @@
 
   *getSelectors(prefix, subtree, styles)
   {
     for (let selector of this.findPropsSelectors(styles, prefix, this._regexp))
       yield [selector, subtree];
   }
 };
 
+function isSelectorHidingOnlyPattern(pattern)

[Wladimir Palant, 2017/08/10 10:12:22]

Note that this function is called exactly once, so you could keep this logic
inline just as well. I'm fine with either, giving this a name provides some
value.

[hub, 2017/08/11 16:26:48]

There was a time it was called twice. but that was unecessary.

+{
+  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 +443,34 @@
 
   /**
    * 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 {boolean} [domUpdate]
+   *    Indicate this is a DOM update.

[Wladimir Palant, 2017/08/10 10:12:21]

Why do we need this parameter? Its purpose seems to be summed up as "ignore the
stylesheets" parameter. The logic seems wrong right now, if a DOM modification
includes a <style> tag we will generally limit the processing of
:-abp-properties() clauses. So in case of :-abp-has(:-abp-properties()) we might
miss cases where :-abp-properties() matches new elements but due to old CSS
rules.

From the look of it, we should generally null out the stylesheets parameter for
any DOM modifications.

[hub, 2017/08/11 16:26:51]

It doesn't mean "ignore the stylesheets". Both can be together. It means to tell
"this is not only a stylesheet only change (line 468 below).

But it will become unneeded, so I'm removing it.

+   * @param {function} [done]
+   *    Callback to call when done.
    */
-  addSelectors(stylesheets)
+  addSelectors(stylesheets, domUpdate, done)
   {
     this._lastInvocation = Date.now();

[Wladimir Palant, 2017/08/10 10:12:22]

With this method being asynchronous now, I think that we need to set this when
it is done, not when it starts.

While at it, we should probably switch to Performance.now(). It has an important
property: "the values returned by Performance.now() always increase at a
constant rate," so no issues with clocks being set by NTP or such.

[hub, 2017/08/11 16:26:48]

Ah right, Date.now() isn't monotonic.

 
     let selectors = [];
     let selectorFilters = [];
 
     let elements = [];
     let elementFilters = [];
 
     let cssStyles = [];
 
-    let stylesheetOnlyChange = !!stylesheets;
+    let stylesheetOnlyChange = !!stylesheets && !domUpdate;
     if (!stylesheets)
       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 +488,164 @@
         if (rule.type != rule.STYLE_RULE)
           continue;
 
         cssStyles.push(stringifyStyle(rule));
       }
     }
 
     let {document} = this.window;
-    for (let pattern of this.patterns)
+
+    let lastCycle = Date.now();
+
+    let processPatterns = function(patternIterator)
     {
-      if (stylesheetOnlyChange &&
-          !pattern.selectors.some(selector => selector.dependsOnStyles))
+      for (let pattern of patternIterator)
       {
-        continue;
+        if (stylesheetOnlyChange &&
+            !pattern.selectors.some(selector => selector.dependsOnStyles))
+        {
+          continue;
+        }
+
+        for (let selector of evaluate(pattern.selectors, 0, "", document,
+                                            cssStyles))
+        {
+          if (selector == null)
+            continue;
+          if (isSelectorHidingOnlyPattern(pattern))
+          {
+            selectors.push(selector);
+            selectorFilters.push(pattern.text);
+          }
+          else
+          {
+            for (let element of document.querySelectorAll(selector))
+            {
+              elements.push(element);
+              elementFilters.push(pattern.text);
+            }
+          }
+        }
+
+        let now = Date.now();
+        if (now - lastCycle > 50)

[Wladimir Palant, 2017/08/10 10:12:21]

No magic numbers please, you should declare a constant like
MAX_SYNCHRONOUS_PROCESSING_TIME.

The logic is also wrong, it's not the end of the last cycle that's relevant here
but the start of the current one. So you need a cycleStart variable being set to
Date.now() at the beginning of processPatterns().

[hub, 2017/08/11 16:26:47]

Done.

+        {
+          lastCycle = now;
+          this.window.setTimeout(() =>
+          {
+            processPatterns(patternIterator);
+          }, 0);
+          return;

[Wladimir Palant, 2017/08/10 10:12:20]

So, why did we go through all the effort yielding null values if we only ever
pause at the end of chain processing? :)

I think the logic should be something like this:

  let patterns = this.patterns.slice();
  let pattern = null;
  let generator = null;

  let processPatterns = () =>
  {
    if (!pattern)
    {
      if (!patterns.length)
      {
        ...
        done();
        return;
      }

      pattern = patterns.shift();
      generator = evaluate(...);
    }

    for (let selector of generator)
    {
      if (selector != null)
      {
        ...
      }
      if (Date.now() - cycleStart > MAX_SYNCHRONOUS_PROCESSING_TIME)
        this.window.setTimeout(processPatterns, 0);
    }

    pattern = null;
    return processPatterns();
  };
  processPatterns();

Two things to note here:

* We are creating a copy of this.patterns here. This is important because its
value might change now that our processing is asynchronous.

* In the recursive call at the end, we return the resulting value. This isn't
really necessary, but allows tail call optimization to kick in.

[hub, 2017/08/11 16:26:50]

Done.

+        }
       }
 
-      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);
-        }
-        else
-        {
-          for (let element of document.querySelectorAll(selector))
-          {
-            elements.push(element);
-            elementFilters.push(pattern.text);
-          }
-        }
-      }
-    }
+      this.addSelectorsFunc(selectors, selectorFilters);
+      this.hideElemsFunc(elements, elementFilters);
+      if (typeof done == "function")
+        done();
+    }.bind(this);

[Wladimir Palant, 2017/08/10 10:12:20]

Don't call bind(), use an arrow function instead.

[hub, 2017/08/11 16:26:51]

Done.

 
-    this.addSelectorsFunc(selectors, selectorFilters);
-    this.hideElemsFunc(elements, elementFilters);
+    processPatterns(this.patterns[Symbol.iterator]());
   },
 
   _stylesheetQueue: null,
+  _domUpdate: false,
+  _filteringInProgress: false,
+  _nextRun: null,
+
+  /** Filtering reason

[Wladimir Palant, 2017/08/10 10:12:21]

Nit: We usually have /** on a line of its own and start JSDoc comments on the
next line. Same below.

[hub, 2017/08/11 16:26:47]

Acknowledged.

+   * @typedef {Object} FilteringReason
+   * @property {boolean} dom Indicate the DOM changed (tree or attributes)

[Wladimir Palant, 2017/08/10 10:12:21]

I get what this sentence means, but I think that it can be phrased better.
"Indicates that a DOM change triggered reprocessing" maybe? Note that "tree or
attributes" is an implementation detail, one that should already be outdated due
to my comments below. We shouldn't be documenting this.

[hub, 2017/08/11 16:26:49]

Acknowledged.

+   * @property {CSSStyleSheet[]} [stylesheets]
+   *    Indicate the stylesheets that needs refresh

[Wladimir Palant, 2017/08/10 10:12:22]

Nit: need, not needs

However, it's not the stylesheets that need a refresh. Rather, it's new
stylesheets that have been added and have to be considered for our filtering.

[hub, 2017/08/11 16:26:47]

Acknowledged.

+   */
+
+  /** Re-run filtering either immediately or queued.
+   * @param {FilteringReason} reason why the filtering must be queued.
+   */
+  queueFiltering(reason)
+  {
+    if (!this._stylesheetQueue &&
+        Date.now() - this._lastInvocation < MIN_INVOCATION_INTERVAL)

[Wladimir Palant, 2017/08/10 10:12:20]

It seems that we will go into this case if addSelectors() has been running for
more than 3 seconds but isn't finished yet. You should be checking the
_filteringInProgress flag first.

[hub, 2017/08/11 16:26:48]

Oops. Done.

+    {
+      this._stylesheetQueue = [];

[Wladimir Palant, 2017/08/10 10:12:22]

This should be `reason.stylesheets || []` I guess?

[hub, 2017/08/11 16:26:51]

Done.

+      this._domUpdate = reason.dom;
+      this.window.setTimeout(() =>
+      {
+        this._filteringInProgress = true;
+        let domUpdate = this._domUpdate;
+        this._domUpdate = false;
+        let stylesheets = this._stylesheetQueue;
+        this._stylesheetQueue = null;
+        this.addSelectors(stylesheets, domUpdate, () =>
+        {
+          this._filteringInProgress = false;
+          if (this._nextRun)
+          {
+            let nextReason = this._nextRun;
+            this._nextRun = null;
+            this.queueFiltering(nextReason);
+          }
+        });
+      }, MIN_INVOCATION_INTERVAL - (Date.now() - this._lastInvocation));
+    }
+    if (this._filteringInProgress)

[Wladimir Palant, 2017/08/10 10:12:21]

This should be `else if` I guess? As it is now, the code might both create a
timeout and run processing immediately.

[hub, 2017/08/11 16:26:48]

Done.

+    {
+      if (!this._nextRun)

[Wladimir Palant, 2017/08/10 10:12:22]

Why use a new variable here instead of reusing _stylesheetQueue and _domUpdate?
This would allow you to merge the `if (this._nextRun)` and `if
(this._stylesheetQueue)` scenarios, these are essentially the same. In fact, I'd
probably keep _nextRun and merely rename it into _scheduledProcessing for
clarity. So the logic would be:

  if (this._scheduledProcessing)
    // merge reason with this._scheduledProcessing
  else if (this._filteringInProgress)
    this._scheduledProcessing = reason;
  else if (Date.now() < ...)
  {
    this._scheduledProcessing = reason;
    setTimeout(...);
  }
  else
    this.addSelectors(...);

[hub, 2017/08/11 16:26:47]

Done.

[hub, 2017/08/11 16:26:47]

Done.

+        this._nextRun = reason;
+      else
+      {
+        this._nextRun.dom = this._nextRun.dom || reason.dom;
+        if (!this._nextRun.stylesheets)
+          this._nextRun.stylesheets = [];
+        this._nextRun.stylesheets.push(...reason.stylesheets);

[Wladimir Palant, 2017/08/10 10:12:23]

reason.stylesheets is optional, this will fail if it isn't present.

[hub, 2017/08/11 16:26:47]

Acknowledged.

+      }
+    }
+    else if (this._stylesheetQueue)
+    {
+      if (reason.stylesheets)
+        this._stylesheetQueue.push(...reason.stylesheets);
+      this._domUpdate = this._domUpdate || reason.domUpdate;
+    }
+    else
+    {
+      let stylesheets = reason.stylesheets || [];
+      this.addSelectors(stylesheets, reason.domUpdate);
+    }
+  },
 
   onLoad(event)
   {
     let stylesheet = event.target.sheet;
     if (stylesheet)
+      this.queueFiltering({stylesheets: [stylesheet]});
+  },
+
+  observe(mutations)
+  {
+    let reason = {};
+    reason.dom = true;
+    let stylesheets = [];
+    for (let mutation of mutations)
     {
-      if (!this._stylesheetQueue &&
-          Date.now() - this._lastInvocation < MIN_INVOCATION_INTERVAL)
+      if (mutation.type == "childList")

[Wladimir Palant, 2017/08/10 10:12:21]

What if the website adds the style element first and sets style.textContent
later?

[hub, 2017/08/11 16:26:47]

It might not work.

I think we should observe for specific characterData mutations into a
HTMLStyleElement.

Done.

       {
-        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.nodeType == this.window.Node.ELEMENT_NODE &&
+              (added.tagName == "STYLE" || added.tagName == "style") &&

[Wladimir Palant, 2017/08/10 10:12:22]

This should be checking for `added.localName == "style"`. Or you might want to
replace both nodeType and localName checks by `added instanceof
this.window.HTMLStyleElement`.

[hub, 2017/08/11 16:26:51]

Done.

+              added.styesheet)

[Wladimir Palant, 2017/08/10 10:12:22]

Typo: added.stylesheet

I guess that this code path isn't covered by tests?

[hub, 2017/08/11 16:26:49]

oops.
no. I should add more test.

+            stylesheets.push(added.stylesheet);
+        }
       }
-
-      if (this._stylesheetQueue)
-        this._stylesheetQueue.push(stylesheet);
-      else
-        this.addSelectors([stylesheet]);
     }
+    if (stylesheets.length > 0)
+      reason.stylesheets = stylesheets;
+    this.queueFiltering(reason);
   },
 
   apply()
   {
     this.getFiltersFunc(patterns =>
     {
       this.patterns = [];
       for (let pattern of patterns)
@@ -513,14 +653,24 @@
         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.addEventListener("load", this.onLoad.bind(this), true);
+        this.addSelectors(null, true, () => {

[Wladimir Palant, 2017/08/10 10:12:22]

If you want queueFiltering() to set important status flags such as
_filteringInProgress, I don't think that addSelectors() should ever be called
directly any more. However, seeing that you also fail to set
_filteringInProgress in queueFiltering() if processing is started immediately, I
think that this flag is better maintained by addSelectors().

[hub, 2017/08/11 16:26:47]

I'll call queueFiltering() instead. Totally make sense.

+          this.observer.observe(
+            document,
+            {
+              childList: true,
+              attributes: true,
+              subtree: true,
+              attributeFilter: ["class", "id"]

[Wladimir Palant, 2017/08/10 10:12:20]

We need to observe characterData changes as well. These are relevant for
ContainsSelector and <style> tags.

We shouldn't filter attribute changes, you don't know which attributes the rules
depend on.

[hub, 2017/08/11 16:26:51]

I kept them deliberately a bit more restricted as to not have too many.
Definitely a hole into properly catching all the relevant changes.

Done.

+            }
+          );
+          document.addEventListener("load", this.onLoad.bind(this), true);
+        });

[Wladimir Palant, 2017/08/10 10:12:21]

The logic here is wrong, we should register the mutation observer and load event
handler immediately rather than in the addSelectors() callback. The issue is:
with addSelectors() being asynchronous now, DOM modifications can occur while
our processing is still in progress. It might be that addSelectors() is early
enough in the process that handling these DOM modifications doesn't require a
new processing round, but we cannot know that. So we should always schedule new
processing just in case.

[hub, 2017/08/11 16:26:50]

make sense. Done.

       }
     });
   }
 };