Issue 6437 - Skip elements not affected by DOM mutations

lib/content/elemHideEmulation.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 const {textToRegExp, filterToRegExp, splitSelector} = require("../common");
 const {indexOf} = require("../coreUtils");
 
 let MIN_INVOCATION_INTERVAL = 3000;
 const MAX_SYNCHRONOUS_PROCESSING_TIME = 50;
 const abpSelectorRegexp = /:-abp-([\w-]+)\(/i;
 
+let testInfo = null;
+
+function setTestMode()
+{
+  testInfo = {
+    lastProcessedElements: new Set()
+  };
+}
+
+exports.setTestMode = setTestMode;
+
+function getTestInfo()
+{
+  return testInfo;
+}
+
+exports.getTestInfo = getTestInfo;

[hub, 2018/05/24 19:19:09]

I'm a bit skeptical with that way or test, including leaving unused code in the
release.

Sadly we don't have spies in our test framework (maybe we should).

[Manish Jethani, 2018/05/25 07:21:25]

We can work on improving our test framework, but for now this is what we have.
Between not having this testing code at all and having it as "dead code" in
production, I'd still go with the latter. At least we're checking our
assumptions in the tests and making sure nothing breaks.

+
 function getCachedPropertyValue(object, name, defaultValueFunc = () => {})
 {
   let value = object[name];
   if (typeof value == "undefined")
     Object.defineProperty(object, name, {value: value = defaultValueFunc()});
   return value;
 }
 
 /** Return position of node from parent.
  * @param {Node} node the node to find the position of.
@@ skipping 269 matching lines @@
         let iter = evaluate(this._innerSelectors, 0, "", element, styles,
                             targets);
         for (let selector of iter)
         {
           if (selector == null)
             yield null;
           else if (scopedQuerySelector(element, selector))
             yield element;
         }
         yield null;
+
+        if (testInfo)
+          testInfo.lastProcessedElements.add(element);
       }
     }
   }
 };
 
 function ContainsSelector(textContent)
 {
   this._regexp = makeRegExpParameter(textContent);
 }
 
@@ skipping 34 matching lines @@
                                                target.contains(element)))
         {
           yield null;
           continue;
         }
 
         if (this._regexp && this._regexp.test(element.textContent))
           yield element;
         else
           yield null;
+
+        if (testInfo)
+          testInfo.lastProcessedElements.add(element);
       }
     }
   }
 };
 
 function PropsSelector(propertyExpression)
 {
   let regexpString;
   if (propertyExpression.length >= 2 && propertyExpression[0] == "/" &&
       propertyExpression[propertyExpression.length - 1] == "/")
@@ skipping 151 matching lines @@
   let targets = new Set();
 
   for (let mutation of mutations)
   {
     if (mutation.type == "childList")
     {
       // When new nodes are added, we're interested in the added nodes rather
       // than the parent.
       for (let node of mutation.addedNodes)
         targets.add(node);
-
-      // Ideally we would also be interested in removed nodes, but since we

[Manish Jethani, 2018/05/11 16:32:00]

Well we're already using CSS selectors for element hiding emulation by now, I'll
have to update this.

See also: https://codereview.adblockplus.org/29728690/

-      // never unhide an element once hidden we can simply ignore any removed
-      // nodes. Note that this will change once we start using CSS selectors
-      // for -abp-has and -abp-contains, i.e. we'll have to remove the
-      // selectors for any removed nodes.
     }
     else
     {
       targets.add(mutation.target);
     }
   }
 
   return [...targets];
 }
 
@@ skipping 117 matching lines @@
    * @param {MutationRecord[]} [mutations]
    *    The list of DOM mutations that have been applied to the document and
    *    made reprocessing necessary. This parameter shouldn't be passed in for
    *    the initial processing, the entire document will be considered
    *    then and all rules, including the ones not dependent on the DOM.
    * @param {function} [done]
    *    Callback to call when done.
    */
   _addSelectors(stylesheets, mutations, done)
   {
+    if (testInfo)
+      testInfo.lastProcessedElements.clear();
+
     let patterns = filterPatterns(this.patterns, {stylesheets, mutations});
 
     let selectors = [];
     let selectorFilters = [];
 
     let elements = [];
     let elementFilters = [];
 
     let cssStyles = [];
 
@@ skipping 68 matching lines @@
 
         pattern = patterns.shift();
 
         let evaluationTargets = targets;
 
         // If the pattern appears to contain any sibling combinators, we can't
         // easily optimize based on the mutation targets. Since this is a
         // special case, skip the optimization. By setting it to null here we
         // make sure we process the entire DOM.
         if (pattern.maybeContainsSiblingCombinators)
+          evaluationTargets = null;
+
+        // Ignore mutation targets when using style sheets, because we may have
+        // to update all the CSS selectors.
+        if (!this.useInlineStyles)
           evaluationTargets = null;
 
         generator = evaluate(pattern.selectors, 0, "",
                              this.document, cssStyles, evaluationTargets);
       }
       for (let selector of generator)
       {
         if (selector != null)
         {
           if (!this.useInlineStyles)
@@ skipping 123 matching lines @@
 
   onLoad(event)
   {
     let stylesheet = event.target.sheet;
     if (stylesheet)
       this.queueFiltering([stylesheet]);
   },
 
   observe(mutations)
   {
+    if (testInfo)
+    {
+      // In test mode, filter out any mutations likely done by us
+      // (i.e. style="display: none !important"). This makes it easier to
+      // observe how the code responds to DOM mutations.
+      mutations = mutations.filter(
+        ({type, attributeName, target: {style: newValue}, oldValue}) =>
+        !(type == "attributes" && attributeName == "style" &&
+          newValue.display == "none" && oldValue.display != "none")
+      );
+
+      if (mutations.length == 0)
+        return;
+    }

[hub, 2018/05/24 19:19:09]

... and this :-/

There should be a better way to go at this.

[Manish Jethani, 2018/05/25 07:21:25]

OK, do you have any suggestions? This is only done in test mode, so it's
literally dead code otherwise. We don't have preprocessing or we could have
stripped it out.

+
     this.queueFiltering(null, mutations);
   },
 
   apply(patterns)
   {
     this.patterns = [];
     for (let pattern of patterns)
     {
       let selectors = this.parseSelector(pattern.selector);
       if (selectors != null && selectors.length > 0)
@@ skipping 11 matching lines @@
           characterData: shouldObserveCharacterData(this.patterns),
           subtree: true
         }
       );
       this.document.addEventListener("load", this.onLoad.bind(this), true);
     }
   }
 };
 
 exports.ElemHideEmulation = ElemHideEmulation;