Bug 1511138 - Improve performance of LightweightThemeConsumer when setting properties, and also avoid _sanitizeCSSColor from getting fooled. r=jaws

This probably deserves a comment as of why it belongs to this bug.

This patch series caused a single, reproducible timeout on
browser_ext_themes_toolbars.js, where the transitionend event it awaits for
stops triggering.

I got fascinated by it and I decided to poke around it in rr instead of just
removing the await line, and here's what's going on.

In the previous implementation of _sanitizeCSSColor, we were not flushing style
because of the optimization bug 1363805 introduced (which wasn't supposed to
deal with out-of-document elements, but it accidentally did so).

In any case, the fact that we were not flushing style in _sanitizeCSSColor
caused us to flush style sometime later when the lwtheme attribute was already
set up, and thus the selector in here matched:

  https://searchfox.org/mozilla-central/rev/cfaa5a1d48d6bc6552199e73004ecb05d0a9c921/browser/themes/shared/browser.inc.css#40

And thus caused the transition rule to apply at a time where the
background-color change happened.

Now we were flushing on getComputedStyle on every call, and in the most
inefficient way possible (changing a custom property on the root before each
property change, which causes us to restyle the whole document to propagate it
down to all descendants).

Furthermore, we were flushing style at a time where the lwtheme attribute
change had not yet happened, and thus when the background-color changed, there
was no transition rule applicable, and the transition didn't fire.

This patch changes LightweightThemeConsumer to avoid restyling the whole
document over and over.

Also, while at it I realized that you could fool the sanitizer with !important
in an experiment stylesheet or with other !important rule in the page really.
It's not clear why you'd do that, but it may be worth to just making that
function completely sound, so I did that and added a test for it.

Differential Revision: https://phabricator.services.mozilla.com/D13716

toolkit/components/extensions/test/browser/browser_ext_themes_sanitization.js

@@ -54,6 +54,47 @@ add_task(async function test_sanitization_css_variables() {
   await extension.unload();
 });
 
+add_task(async function test_sanitization_important() {
+  // This test checks that the sanitizer cannot be fooled with !important
+  let stylesheetAttr = `href="data:text/css,*{color:red!important}" type="text/css"`;
+  let stylesheet =
+    document.createProcessingInstruction("xml-stylesheet", stylesheetAttr);
+  let load = BrowserTestUtils.waitForEvent(stylesheet, "load");
+  document.insertBefore(stylesheet, document.documentElement);
+  await load;
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      "theme": {
+        "colors": {
+          "frame": ACCENT_COLOR,
+          "tab_background_text": TEXT_COLOR,
+          "bookmark_text": "green",
+        },
+      },
+    },
+  });
+
+  await extension.startup();
+
+  let navbar = document.querySelector("#nav-bar");
+  Assert.equal(
+    window.getComputedStyle(navbar).color,
+    "rgb(255, 0, 0)",
+    "Sheet applies"
+  );
+
+  stylesheet.remove();
+
+  Assert.equal(
+    window.getComputedStyle(navbar).color,
+    "rgb(0, 128, 0)",
+    "Shouldn't be able to fool the color sanitizer with !important"
+  );
+
+  await extension.unload();
+});
+
 add_task(async function test_sanitization_transparent() {
   // This test checks whether transparent values are applied properly
   let extension = ExtensionTestUtils.loadExtension({

toolkit/modules/LightweightThemeConsumer.jsm

@@ -219,47 +219,51 @@ LightweightThemeConsumer.prototype = {
         stylesheet.remove();
       }
       if (usedVariables) {
-        for (const variable of usedVariables) {
+        for (const [variable] of usedVariables) {
           _setProperty(root, false, variable);
         }
       }
     }
-    if (active && experiment) {
-      this._lastExperimentData = {};
-      if (experiment.stylesheet) {
-        /* Stylesheet URLs are validated using WebExtension schemas */
-        let stylesheetAttr = `href="${experiment.stylesheet}" type="text/css"`;
-        let stylesheet = this._doc.createProcessingInstruction("xml-stylesheet",
-          stylesheetAttr);
-        this._doc.insertBefore(stylesheet, root);
-        this._lastExperimentData.stylesheet = stylesheet;
+
+    this._lastExperimentData = {};
+
+    if (!active || !experiment) {
+      return;
+    }
+
+    let usedVariables = [];
+    if (properties.colors) {
+      for (const property in properties.colors) {
+        const cssVariable = experiment.colors[property];
+        const value = _sanitizeCSSColor(root.ownerDocument, properties.colors[property]);
+        usedVariables.push([cssVariable, value]);
       }
-      let usedVariables = [];
-      if (properties.colors) {
-        for (const property in properties.colors) {
-          const cssVariable = experiment.colors[property];
-          const value = _sanitizeCSSColor(root.ownerDocument, properties.colors[property]);
-          _setProperty(root, active, cssVariable, value);
-          usedVariables.push(cssVariable);
-        }
+    }
+
+    if (properties.images) {
+      for (const property in properties.images) {
+        const cssVariable = experiment.images[property];
+        usedVariables.push([cssVariable, `url(${properties.images[property]})`]);
       }
-      if (properties.images) {
-        for (const property in properties.images) {
-          const cssVariable = experiment.images[property];
-          _setProperty(root, active, cssVariable, `url(${properties.images[property]})`);
-          usedVariables.push(cssVariable);
-        }
+    }
+    if (properties.properties) {
+      for (const property in properties.properties) {
+        const cssVariable = experiment.properties[property];
+        usedVariables.push([cssVariable, properties.properties[property]]);
       }
-      if (properties.properties) {
-        for (const property in properties.properties) {
-          const cssVariable = experiment.properties[property];
-          _setProperty(root, active, cssVariable, properties.properties[property]);
-          usedVariables.push(cssVariable);
-        }
-      }
-      this._lastExperimentData.usedVariables = usedVariables;
-    } else {
-      this._lastExperimentData = null;
+    }
+    for (const [variable, value] of usedVariables) {
+      _setProperty(root, true, variable, value);
+    }
+    this._lastExperimentData.usedVariables = usedVariables;
+
+    if (experiment.stylesheet) {
+      /* Stylesheet URLs are validated using WebExtension schemas */
+      let stylesheetAttr = `href="${experiment.stylesheet}" type="text/css"`;
+      let stylesheet = this._doc.createProcessingInstruction("xml-stylesheet",
+        stylesheetAttr);
+      this._doc.insertBefore(stylesheet, root);
+      this._lastExperimentData.stylesheet = stylesheet;
     }
   },
 };
@@ -293,6 +297,8 @@ function _setProperty(elem, active, variableName, value) {
 }
 
 function _setProperties(root, active, themeData) {
+  let properties = [];
+
   for (let map of [toolkitVariableMap, ThemeVariableMap]) {
     for (let [cssVarName, definition] of map) {
       const {
@@ -303,7 +309,6 @@ function _setProperties(root, active, themeData) {
       } = definition;
       let elem = optionalElementID ? root.ownerDocument.getElementById(optionalElementID)
                                    : root;
-
       let val = themeData[lwtProperty];
       if (isColor) {
         val = _sanitizeCSSColor(root.ownerDocument, val);
@@ -311,9 +316,14 @@ function _setProperties(root, active, themeData) {
           val = processColor(_parseRGBA(val), elem);
         }
       }
-      _setProperty(elem, active, cssVarName, val);
+      properties.push([elem, cssVarName, val]);
     }
   }
+
+  // Set all the properties together, since _sanitizeCSSColor flushes.
+  for (const [elem, cssVarName, val] of properties) {
+    _setProperty(elem, active, cssVarName, val);
+  }
 }
 
 function _sanitizeCSSColor(doc, cssColor) {
@@ -323,14 +333,15 @@ function _sanitizeCSSColor(doc, cssColor) {
   const HTML_NS = "http://www.w3.org/1999/xhtml";
   // style.color normalizes color values and makes invalid ones black, so a
   // simple round trip gets us a sanitized color value.
+  // Use !important so that the theme's stylesheets cannot override us.
   let div = doc.createElementNS(HTML_NS, "div");
-  div.style.color = "black";
-  div.style.display = "none";
+  div.style.setProperty("color", "black", "important");
+  div.style.setProperty("display", "none", "important");
   let span = doc.createElementNS(HTML_NS, "span");
-  span.style.color = cssColor;
+  span.style.setProperty("color", cssColor, "important");
 
   // CSS variables are not allowed and should compute to black.
-  if (span.style.color.indexOf("var(") !== -1) {
+  if (span.style.color.includes("var(")) {
     span.style.color = "";
   }