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

lib/cssInjection.js

Index: lib/cssInjection.js
===================================================================
--- a/lib/cssInjection.js
+++ b/lib/cssInjection.js
@@ -24,16 +24,35 @@
 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;
+
+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;
+}
 
 function addStyleSheet(tabId, frameId, styleSheet)
 {
   browser.tabs.insertCSS(tabId, {
     code: styleSheet,
     cssOrigin: "user",
     frameId,
     matchAboutBlank: true,
@@ -50,17 +69,17 @@
     matchAboutBlank: true
   });
 }
 
 function updateFrameStyles(tabId, frameId, selectors, groupName)
 {
   let styleSheet = null;
   if (selectors.length > 0)
-    styleSheet = selectors.join(", ") + "{display: none !important;}";
+    styleSheet = createStyleSheet(selectors);
 
   let frame = ext.getFrame(tabId, frameId);
   if (!frame.injectedStyleSheets)
     frame.injectedStyleSheets = new Map();
 
   let oldStyleSheet = frame.injectedStyleSheets.get(groupName);
 
   // Ideally we would compare the old and new style sheets and skip this code