Issue 4298 - Enforce 'display: none' for ElemHide

include.preload.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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
@@ skipping 316 matching lines @@
   },
 
   disconnect: function()
   {
     this.document.removeEventListener("DOMContentLoaded", this.trace);
     this.observer.disconnect();
     clearTimeout(this.timeout);
   }
 };
 
-function enforceStyleSheetRules(style)
-{
-  style.id = id;
-  injectJS(
-    function(id)
-    {
-      var style = document.getElementById(id) ||
-                  document.documentElement.shadowRoot.getElementById(id);
-      style.removeAttribute("id");
-
-      Object.defineProperty(style.sheet, "disabled",

[Sebastian Noack, 2016/08/04 20:44:23]

Where did the code go, that addressed deleteRule and removeRule?

[kzar, 2016/08/05 11:01:57]

This review is based upon the changes in a couple of other reviews. They're
listed in this review's description. That stuff is somewhere near line 400 here,
you'll have to expand the code to see it. Equally if it's easier these changes
are in GitHub too:
https://github.com/kzar/adblockpluschrome/commits/4298-enforce-display-none

I would be happy to combine this review with the review for 4191 since they're
very similar and things are getting confusing. Any objections?

-                            {value: false, enumerable: true});
-
-      for (var i = 0; i < style.sheet.rules.length; i += 1)
-        Object.defineProperty(style.sheet.rules[i].style, "display",

[Sebastian Noack, 2016/08/04 20:44:23]

How about overriding style.sheet.rules (and sheet.cssRules) so that it always
returns an empty array? That way we wouldn't have to monkey-patch every single
rule which might cause performance issues.

[kzar, 2016/08/05 11:01:57]

Cool idea, but I actually had to overwrite the getter on
CSSStyleSheet.prototype. 

(It also turns out my old implementation only happened to work for the website
in question by luck. Setting a rule's style to read only doesn't do anything
apparently, it's still writable!)

-                              {value: "none", enumerable: true});
-    }, id
-  );
-}
-
 function reinjectStyleSheetWhenRemoved(document, style)
 {
   if (!MutationObserver)
     return null;
 
   var rules = style.sheet.rules;
   var parentNode = style.parentNode;
   var observer = new MutationObserver(function()
   {
     if (style.parentNode != parentNode)
     {
       parentNode.appendChild(style);
 
+      // Our style element's sheet is sometimes lost when the element is removed
+      // from the DOM. A new empty sheet is then created in its place so we have
+      // to restore the rules ourselves.
       if (style.sheet.rules.length == 0)
       {
         for (var i = 0; i < rules.length; i += 1)
-          style.sheet.addRule(rules[i].selectorText);
-
-        enforceStyleSheetRules(style);
+          style.sheet.addRule(rules[i].selectorText, "display: none !important;");
+
+        style.id = id;
+        injectJS(
+          function(id)
+          {
+            var style = document.getElementById(id) ||
+                        document.documentElement.shadowRoot.getElementById(id);
+            style.removeAttribute("id");
+
+            Object.defineProperty(style.sheet, "disabled",
+                                  {value: false, enumerable: true});
+          }, id
+        );
       }
     }
   });
 
   observer.observe(parentNode, {childList: true});
   return observer;
 }
 
 function injectJS(f)
 {
@@ skipping 11 matching lines @@
 function protectStyleSheet(document, style)
 {
   style.id = id;
 
   var protector = function(id)
   {
     var style = document.getElementById(id) ||
                 document.documentElement.shadowRoot.getElementById(id);
     style.removeAttribute("id");
 
-    var i;
-    var disableables = [style, style.sheet];
-    for (i = 0; i < disableables.length; i += 1)
-      Object.defineProperty(disableables[i], "disabled",
-                            {value: false, enumerable: true});
-
-    var methods = ["deleteRule", "removeRule"];
-    for (i = 0; i < methods.length; i += 1)
-    {
-      if (methods[i] in CSSStyleSheet.prototype)
-      {
-        (function(method)
+    Object.defineProperty(style, "disabled",
+                          {value: false, enumerable: true});
+    Object.defineProperty(style.sheet, "disabled",
+                          {value: false, enumerable: true});
+
+    function wrapMethods(methodNames, descKey, defaultReturnValue)
+    {
+      for (var i = 0; i < methodNames.length; i += 1)
+      {
+        if (methodNames[i] in CSSStyleSheet.prototype)
         {
-          var original = CSSStyleSheet.prototype[method];
-          CSSStyleSheet.prototype[method] = function(index)
+          (function(method)
           {
-            if (this != style.sheet)
-              original.call(this, index);
-          };
-        }(methods[i]));
-      }
-    }
+            var desc = Object.getOwnPropertyDescriptor(CSSStyleSheet.prototype,
+                                                       method);
+            var original = desc[descKey];
+            desc[descKey] = function()
+            {
+              if (this != style.sheet)
+                return original.apply(this, arguments);
+              return defaultReturnValue;
+            };
+            Object.defineProperty(CSSStyleSheet.prototype, method, desc);
+          }(methodNames[i]));
+        }
+      }
+    }
+
+    wrapMethods(["deleteRule", "removeRule"], "value");
+    wrapMethods(["rules", "cssRules"], "get", []);
   };
 
   injectJS(protector, id);
 }
 
 // Neither Chrome[1] nor Safari allow us to intercept WebSockets, and therefore
 // some ad networks are misusing them as a way to serve adverts and circumvent
 // us. As a workaround we wrap WebSocket, preventing blocked WebSocket
 // connections from being opened.
 // [1] - https://bugs.chromium.org/p/chromium/issues/detail?id=129353
@@ skipping 159 matching lines @@
     }
 
     // Safari only allows 8192 primitive selectors to be injected at once[1], we
     // therefore chunk the inserted selectors into groups of 200 to be safe.
     // (Chrome also has a limit, larger... but we're not certain exactly what it
     //  is! Edge apparently has no such limit.)
     // [1] - https://github.com/WebKit/webkit/blob/1cb2227f6b2a1035f7bdc46e5ab69debb75fc1de/Source/WebCore/css/RuleSet.h#L68
     for (var i = 0; i < selectors.length; i += SELECTOR_GROUP_SIZE)
     {
       var selector = selectors.slice(i, i + SELECTOR_GROUP_SIZE).join(", ");
-      style.sheet.addRule(selector);
-    }
-    enforceStyleSheetRules(style);
+      style.sheet.addRule(selector, "display: none !important;");
+    }
   };
 
   var updateStylesheet = function()
   {
     var selectors = null;
     var CSSPropertyFiltersLoaded = false;
 
     var checkLoaded = function()
     {
       if (!selectors || !CSSPropertyFiltersLoaded)
@@ skipping 75 matching lines @@
   }, true);
 
   return updateStylesheet;
 }
 
 if (document instanceof HTMLDocument)
 {
   checkSitekey();
   window.updateStylesheet = init(document);
 }