Issue 6298 - Split injected CSS hiding rule into groups of 1,024 selectors

lib/cssInjection.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
@@ skipping 11 matching lines @@
 const {RegExpFilter} = require("filterClasses");
 const {ElemHide} = require("elemHide");
 const {ElemHideEmulation} = require("elemHideEmulation");
 const {checkWhitelisted} = require("whitelisting");
 const {extractHostFromFrame} = require("url");
 const {port} = require("messaging");
 const devtools = require("devtools");
 
 const userStyleSheetsSupported = "extensionTypes" in browser &&
                                  "CSSOrigin" in browser.extensionTypes;
-const selectorGroupSize = 200;
+const selectorGroupSize = 1024;
+
+function* splitSelectors(selectors)
+{
+  // Chromium's Blink engine supports only up to 8,192 simple selectors, and
+  // even fewer compound selectors, in a rule. The exact number of selectors
+  // that would work depends on their sizes (e.g. "#foo .bar" has a size of 2).
+  // Since we don't know the sizes of the selectors here, we simply split them
+  // into groups of 1,024, based on the reasonable assumption that the average
+  // selector won't have a size greater than 8. The alternative would be to
+  // calculate the sizes of the selectors and divide them up accordingly, but
+  // this approach is more efficient and has worked well in practice. In theory
+  // this could still lead to some selectors not working on Chromium, but it is
+  // highly unlikely.
+  // See issue #6298 and https://crbug.com/804179
+  for (let i = 0; i < selectors.length; i += selectorGroupSize)
+    yield selectors.slice(i, i + selectorGroupSize);
+}
+
+function* createRules(selectors)
+{
+  for (let selectorGroup of splitSelectors(selectors))
+    yield selectorGroup.join(", ") + " {display: none !important;}";
+}
 
 function createStyleSheet(selectors)
 {
-  let styleSheet = "";
-
-  // Chromium's Blink engine supports only up to 8,192 selectors; more

[kzar, 2018/01/25 17:46:47]

If we're limited to 8192 why do we go with 200?

[Manish Jethani, 2018/01/25 18:42:41]

It's "up to 8,192" selectors.

If you have 8,192 simple selectors ("#foo", ".bar", ...), they'll all be taken
into account. If you have 8,191 simple selectors and one compound selector
consisting of a million plain selectors ("#foo .bar div a:visited ..."), they
will still all work. On the other hand, if you move the last selector from the
aforementioned list to the beginning of the list, such that now the list starts
with a compound selector consisting of a million plain selectors, then all but
the first selector will be ignored.

The minimum therefore is 1, the maximum is 8,192.

Now ideally we would count the plain selectors the way Chromium does and divide
them up correctly so we can have the fewest possible rules, but this is not
worth it. It's reasonable to assume that the average selector in EasyList won't
exceed a certain size. Maybe 200 is too low a number, but it's been working OK
for inline style sheets so there's no good reason to change this number.

[Manish Jethani, 2018/01/25 18:44:28]

This was not technically true by the way but now [1] it is.

[1]:
https://chromium.googlesource.com/chromium/src.git/+/5814e68d9cf56da285688558...

[Manish Jethani, 2018/01/30 06:19:43]

Now that I think about it, maybe we should increase this from 200 to something
less arbitrary and more reasonable like 1,024. This would assume that the
average selector in the group can consist of up to 8 plain selectors (e.g.
"lorem > ipsum > dolor > sit > amet > consectetur > adipiscing > elit"). This is
only the average, most selectors are smaller (usually just one), therefore even
if there are a few very long selectors 1,024 should be a reasonable size for the
group.

[kzar1, 2018/01/30 11:39:59]

Sounds good to me, but let's rewrite the comment to make that plain:

// Chromium's Blink engine does not support rules with more than 8192
// selectors, it discards the excess selectors. We don't know for sure
// how many plain selectors are in a selector group, so to be safe we'll
// insert the groups in batches of 1024. The assumption is that there
// will be less than 8 plain selectors in a group on average.
// See issue #6298 and https://crbug.com/804179

[Manish Jethani, 2018/01/30 13:15:43]

Unfortunately this comment is confusing and technically incorrect (it's 8 plain
selectors per compound selector, not per group), and it still doesn't explain
why we can't just add the selectors in batches of 8,192.

Here's my second attempt at explaining what we're doing in the comment:

  // Chromium's Blink engine supports only up to 8,192 simple selectors, and
  // even fewer compound selectors, in a rule. The exact number of selectors
  // that would work depends on their sizes (e.g. "#foo .bar" has a size of 2).
  // Since we don't know the sizes of the selectors here, we simply split them
  // into groups of 1,024, based on the reasonable assumption that the average
  // selector won't have a size greater than 8. The alternative would be to
  // calculate the sizes of the selectors and divide them up accordingly, but
  // this approach is more efficient and has worked well in practice. In theory
  // this could still lead to some selectors not working on Chromium, but it is
  // highly unlikely.
  // See issue #6298 and https://crbug.com/804179

-  // specifically, it ignores any selectors that start at index 8192 or beyond
-  // in the list of plain selectors. In order to avoid spilling outside of this
-  // range, we simply add multiple rules in groups of up to 200 selectors each.
-  // See issue #6298 and https://crbug.com/804179
-  for (let i = 0; i < selectors.length; i += selectorGroupSize)
-  {
-    styleSheet += selectors.slice(i, i + selectorGroupSize).join(", ") +
-                  " {display: none !important;}\n";
-  }
-
-  return styleSheet;
+  return Array.from(createRules(selectors)).join("\n");
 }
 
 function addStyleSheet(tabId, frameId, styleSheet)
 {
   browser.tabs.insertCSS(tabId, {
     code: styleSheet,
     cssOrigin: "user",
     frameId,
     matchAboutBlank: true,
     runAt: "document_start"
@@ skipping 73 matching lines @@
     response.selectors = selectors;
 
   return response;
 });
 
 port.on("elemhide.injectSelectors", (message, sender) =>
 {
   updateFrameStyles(sender.page.id, sender.frame.id, message.selectors,
                     message.groupName);
 });