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 debugInfo = null;
-
-function setDebugMode()
-{
-  debugInfo = {
+let testInfo = null;
+
+function setTestMode()
+{
+  testInfo = {
     lastProcessedElements: new Set()

[Manish Jethani, 2018/05/16 07:22:12]

We could add another entry here, lastProcessedPatterns, but that would be
another patch

   };
 }
 
-exports.setDebugMode = setDebugMode;
-
-function getDebugInfo()
-{
-  return debugInfo;
-}
-
-exports.getDebugInfo = getDebugInfo;
+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.
@@ skipping 271 matching lines @@
                             targets);
         for (let selector of iter)
         {
           if (selector == null)
             yield null;
           else if (scopedQuerySelector(element, selector))
             yield element;
         }
         yield null;
 
-        if (debugInfo)
-          debugInfo.lastProcessedElements.add(element);
+        if (testInfo)
+          testInfo.lastProcessedElements.add(element);
       }
     }
   }
 };
 
 function ContainsSelector(textContent)
 {
   this._regexp = makeRegExpParameter(textContent);
 }
 
@@ skipping 35 matching lines @@
         {
           yield null;
           continue;
         }
 
         if (this._regexp && this._regexp.test(element.textContent))
           yield element;
         else
           yield null;
 
-        if (debugInfo)
-          debugInfo.lastProcessedElements.add(element);
+        if (testInfo)
+          testInfo.lastProcessedElements.add(element);
       }
     }
   }
 };
 
 function PropsSelector(propertyExpression)
 {
   let regexpString;
   if (propertyExpression.length >= 2 && propertyExpression[0] == "/" &&
       propertyExpression[propertyExpression.length - 1] == "/")
@@ skipping 288 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 (debugInfo)
-      debugInfo.lastProcessedElements.clear();
+    if (testInfo)
+      testInfo.lastProcessedElements.clear();
 
     let patterns = filterPatterns(this.patterns, {stylesheets, mutations});
 
     let selectors = [];
     let selectorFilters = [];
 
     let elements = [];
     let elementFilters = [];
 
     let cssStyles = [];
@@ skipping 217 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;