Bug 1511138 - Improve performance of LightweightThemeConsumer when setting properties, and also avoid _sanitizeCSSColor from getting fooled. r=jaws
☠☠ backed out by 5fcc6bd202c4 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 03 Dec 2018 21:06:44 -0500
changeset 449657 d23c9c3e1566f64b0257d97597daaae3c3ed0897
parent 449656 a99600391704f8752e6f05e312fe8e8cd588715a
child 449658 daee82295b3c81d57991a7db1b6851dc7bc8d963
push id35179
push useraciure@mozilla.com
push dateSun, 09 Dec 2018 21:43:27 +0000
treeherdermozilla-central@53fd96ca5aa4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1511138, 1363805
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
toolkit/modules/LightweightThemeConsumer.jsm
--- a/toolkit/components/extensions/test/browser/browser_ext_themes_sanitization.js
+++ b/toolkit/components/extensions/test/browser/browser_ext_themes_sanitization.js
@@ -49,16 +49,57 @@ add_task(async function test_sanitizatio
     window.getComputedStyle(navbar).color,
     "rgb(0, 0, 0)",
     "All CSS variables should always compute to black."
   );
 
   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({
     manifest: {
       "theme": {
         "colors": {
           "frame": ACCENT_COLOR,
           "tab_background_text": TEXT_COLOR,
--- a/toolkit/modules/LightweightThemeConsumer.jsm
+++ b/toolkit/modules/LightweightThemeConsumer.jsm
@@ -214,57 +214,61 @@ LightweightThemeConsumer.prototype = {
   _setExperiment(active, experiment, properties) {
     const root = this._doc.documentElement;
     if (this._lastExperimentData) {
       const { stylesheet, usedVariables } = this._lastExperimentData;
       if (stylesheet) {
         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.properties) {
+      for (const property in properties.properties) {
+        const cssVariable = experiment.properties[property];
+        usedVariables.push([cssVariable, properties.properties[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];
-          _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;
     }
   },
 };
 
 function _getContentProperties(doc, active, data) {
   if (!active) {
     return {};
   }
@@ -288,54 +292,61 @@ function _setProperty(elem, active, vari
   if (active && value) {
     elem.style.setProperty(variableName, value);
   } else {
     elem.style.removeProperty(variableName);
   }
 }
 
 function _setProperties(root, active, themeData) {
+  let properties = [];
+
   for (let map of [toolkitVariableMap, ThemeVariableMap]) {
     for (let [cssVarName, definition] of map) {
       const {
         lwtProperty,
         optionalElementID,
         processColor,
         isColor = true,
       } = definition;
       let elem = optionalElementID ? root.ownerDocument.getElementById(optionalElementID)
                                    : root;
-
       let val = themeData[lwtProperty];
       if (isColor) {
         val = _sanitizeCSSColor(root.ownerDocument, val);
         if (processColor) {
           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) {
   if (!cssColor) {
     return null;
   }
   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 = "";
   }
 
   div.appendChild(span);
   doc.documentElement.appendChild(div);
   cssColor = doc.defaultView.getComputedStyle(span).color;
   div.remove();
   return cssColor;