Don't crash on parsing bad CSS

Fixes #1477. This was a regression in 780b5dc since we no longer parse CSS inside mutation events, where errors just go to window.onerror. Instead, bad CSS now fires a jsdomError. Also cleaned up and consolidated duplicate code in the HTMLLinkElement and HTMLStyleElement implementations.
jsdom · May 15, 2016 · e955ac7 · e955ac7
1 parent 1805283
commit e955ac7
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 127 deletions.
diff --git a/README.md b/README.md
@@ -498,7 +498,7 @@ var virtualConsole = jsdom.getVirtualConsole(window);
 
 Besides the usual events, corresponding to `console` methods, the virtual console is also used for reporting errors from jsdom itself. This is similar to how error messages often show up in web browser consoles, even if they are not initiated by `console.error`. So far, the following errors are output this way:
 
-- Errors loading external resources (scripts, stylesheets, frames, and iframes)
+- Errors loading or parsing external resources (scripts, stylesheets, frames, and iframes)
 - Script execution errors that are not handled by a window `onerror` event handler that returns `true` or calls `event.preventDefault()`
 - Calls to methods, like `window.alert`, which jsdom does not implement, but installs anyway for web compatibility
 

diff --git a/lib/jsdom/living/helpers/stylesheets.js b/lib/jsdom/living/helpers/stylesheets.js
@@ -0,0 +1,60 @@
+"use strict";
+const cssom = require("cssom");
+const normalizeEncoding = require("../helpers/encoding").normalizeEncoding;
+const resourceLoader = require("../../browser/resource-loader");
+const resolveHref = require("../../utils").resolveHref;
+
+exports.fetchStylesheet = (elementImpl, url, sheet) => {
+  let defaultEncoding = elementImpl._ownerDocument._encoding;
+  if (elementImpl.localName === "link" && elementImpl.hasAttribute("charset")) {
+    defaultEncoding = normalizeEncoding(elementImpl.getAttribute("charset"));
+  }
+
+  resourceLoader.load(elementImpl, url, { defaultEncoding }, data => {
+    // TODO: MIME type checking?
+    exports.evaluateStylesheet(elementImpl, data, sheet, url);
+  });
+};
+
+exports.evaluateStylesheet = (elementImpl, data, sheet, baseURL) => {
+  let newStyleSheet;
+  try {
+    newStyleSheet = cssom.parse(data);
+  } catch (e) {
+    if (elementImpl._ownerDocument._defaultView) {
+      const error = new Error("Could not parse CSS stylesheet");
+      error.detail = data;
+      elementImpl._ownerDocument._defaultView._virtualConsole.emit("jsdomError", error);
+    }
+
+    elementImpl._ownerDocument.styleSheets.push(sheet);
+    return;
+  }
+
+  const spliceArgs = newStyleSheet.cssRules;
+  spliceArgs.unshift(0, sheet.cssRules.length);
+  Array.prototype.splice.apply(sheet.cssRules, spliceArgs);
+
+  scanForImportRules(elementImpl, sheet.cssRules, baseURL);
+
+  elementImpl._ownerDocument.styleSheets.push(sheet);
+};
+
+function scanForImportRules(elementImpl, cssRules, baseURL) {
+  if (!cssRules) {
+    return;
+  }
+
+  for (let i = 0; i < cssRules.length; ++i) {
+    if (cssRules[i].cssRules) {
+      // @media rule: keep searching inside it.
+      scanForImportRules(elementImpl, cssRules[i].cssRules, baseURL);
+    } else if (cssRules[i].href) {
+      // @import rule: fetch the resource and evaluate it.
+      // See http://dev.w3.org/csswg/cssom/#css-import-rule
+      //     If loading of the style sheet fails its cssRules list is simply
+      //     empty. I.e. an @import rule always has an associated style sheet.
+      exports.fetchStylesheet(elementImpl, resolveHref(baseURL, cssRules[i].href), elementImpl.sheet);
+    }
+  }
+}
diff --git a/lib/jsdom/living/nodes/HTMLLinkElement-impl.js b/lib/jsdom/living/nodes/HTMLLinkElement-impl.js
@@ -1,70 +1,9 @@
 "use strict";
-
-const cssom = require("cssom");
-
 const HTMLElementImpl = require("./HTMLElement-impl").implementation;
 const LinkStyleImpl = require("./LinkStyle-impl").implementation;
 const idlUtils = require("../generated/utils");
-const normalizeEncoding = require("../helpers/encoding").normalizeEncoding;
-
 const resourceLoader = require("../../browser/resource-loader");
-const resolveHref = require("../../utils").resolveHref;
-
-/**
- * @this {core.HTMLLinkElement|core.HTMLStyleElement}
- * @param {string} url
- * @param {cssom.CSSStyleSheet} sheet
- * @see http://dev.w3.org/csswg/cssom/#requirements-on-user-agents-implementing0
- */
-function fetchStylesheet(url, sheet) {
-  resourceLoader.load(this,
-  url,
-  { defaultEncoding: normalizeEncoding(this.getAttribute("charset")) || this._ownerDocument._encoding },
-  data => {
-    // TODO: abort if the content-type is not text/css, and the document is
-    // in strict mode
-    url = sheet.href = resourceLoader.resolveResourceUrl(this.ownerDocument, url);
-    evaluateStylesheet.call(this, data, sheet, url);
-  });
-}
-/**
- * @this {core.HTMLLinkElement|core.HTMLStyleElement}
- * @param {string} data
- * @param {cssom.CSSStyleSheet} sheet
- * @param {string} baseUrl
- */
-function evaluateStylesheet(data, sheet, baseUrl) {
-  // this is the element
-  const newStyleSheet = cssom.parse(data);
-  const spliceArgs = newStyleSheet.cssRules;
-  spliceArgs.unshift(0, sheet.cssRules.length);
-  Array.prototype.splice.apply(sheet.cssRules, spliceArgs);
-  scanForImportRules.call(this, sheet.cssRules, baseUrl);
-  this.ownerDocument.styleSheets.push(sheet);
-}
-/**
- * @this {core.HTMLLinkElement|core.HTMLStyleElement}
- * @param {cssom.CSSStyleSheet} sheet
- * @param {string} baseUrl
- */
-function scanForImportRules(cssRules, baseUrl) {
-  if (!cssRules) {
-    return;
-  }
-
-  for (let i = 0; i < cssRules.length; ++i) {
-    if (cssRules[i].cssRules) {
-      // @media rule: keep searching inside it.
-      scanForImportRules.call(this, cssRules[i].cssRules, baseUrl);
-    } else if (cssRules[i].href) {
-      // @import rule: fetch the resource and evaluate it.
-      // See http://dev.w3.org/csswg/cssom/#css-import-rule
-      //     If loading of the style sheet fails its cssRules list is simply
-      //     empty. I.e. an @import rule always has an associated style sheet.
-      fetchStylesheet.call(this, resolveHref(baseUrl, cssRules[i].href), this.sheet);
-    }
-  }
-}
+const fetchStylesheet = require("../helpers/stylesheets").fetchStylesheet;
 
 class HTMLLinkElementImpl extends HTMLElementImpl {
   _attach() {
@@ -75,7 +14,7 @@ class HTMLLinkElementImpl extends HTMLElementImpl {
       return;
     }
     if (this.href) {
-      fetchStylesheet.call(this, this.href, this.sheet);
+      fetchStylesheet(this, this.href, this.sheet);
     }
 
     super._attach();

diff --git a/lib/jsdom/living/nodes/HTMLStyleElement-impl.js b/lib/jsdom/living/nodes/HTMLStyleElement-impl.js
@@ -1,71 +1,10 @@
 "use strict";
-
-const cssom = require("cssom");
-
 const HTMLElementImpl = require("./HTMLElement-impl").implementation;
 const LinkStyleImpl = require("./LinkStyle-impl").implementation;
 const idlUtils = require("../generated/utils");
-
 const domSymbolTree = require("../helpers/internal-constants").domSymbolTree;
 const NODE_TYPE = require("../node-type");
-const resourceLoader = require("../../browser/resource-loader");
-const resolveHref = require("../../utils").resolveHref;
-
-/**
- * @this {core.HTMLLinkElement|core.HTMLStyleElement}
- * @param {string} url
- * @param {cssom.CSSStyleSheet} sheet
- * @see http://dev.w3.org/csswg/cssom/#requirements-on-user-agents-implementing0
- */
-function fetchStylesheet(url, sheet) {
-  resourceLoader.load(this,
-  url,
-  { defaultEncoding: this._ownerDocument._encoding },
-  data => {
-    // TODO: abort if the content-type is not text/css, and the document is
-    // in strict mode
-    url = sheet.href = resourceLoader.resolveResourceUrl(this.ownerDocument, url);
-    evaluateStylesheet.call(this, data, sheet, url);
-  });
-}
-/**
- * @this {core.HTMLLinkElement|core.HTMLStyleElement}
- * @param {string} data
- * @param {cssom.CSSStyleSheet} sheet
- * @param {string} baseUrl
- */
-function evaluateStylesheet(data, sheet, baseUrl) {
-  // this is the element
-  const newStyleSheet = cssom.parse(data);
-  const spliceArgs = newStyleSheet.cssRules;
-  spliceArgs.unshift(0, sheet.cssRules.length);
-  Array.prototype.splice.apply(sheet.cssRules, spliceArgs);
-  scanForImportRules.call(this, sheet.cssRules, baseUrl);
-  this.ownerDocument.styleSheets.push(sheet);
-}
-/**
- * @this {core.HTMLLinkElement|core.HTMLStyleElement}
- * @param {cssom.CSSStyleSheet} sheet
- * @param {string} baseUrl
- */
-function scanForImportRules(cssRules, baseUrl) {
-  if (!cssRules) {
-    return;
-  }
-
-  for (let i = 0; i < cssRules.length; ++i) {
-    if (cssRules[i].cssRules) {
-      // @media rule: keep searching inside it.
-      scanForImportRules.call(this, cssRules[i].cssRules, baseUrl);
-    } else if (cssRules[i].href) {
-      // @import rule: fetch the resource and evaluate it.
-      // See http://dev.w3.org/csswg/cssom/#css-import-rule
-      //     If loading of the style sheet fails its cssRules list is simply
-      //     empty. I.e. an @import rule always has an associated style sheet.
-      fetchStylesheet.call(this, resolveHref(baseUrl, cssRules[i].href), this.sheet);
-    }
-  }
-}
+const evaluateStylesheet = require("../helpers/stylesheets").evaluateStylesheet;
 
 class HTMLStyleElementImpl extends HTMLElementImpl {
   _attach() {
@@ -80,7 +19,7 @@ class HTMLStyleElementImpl extends HTMLElementImpl {
       }
     }
 
-    evaluateStylesheet.call(this, content, this.sheet, this._ownerDocument.URL);
+    evaluateStylesheet(this, content, this.sheet, this._ownerDocument.URL);
 
     super._attach();
   }

diff --git a/test/jsdom/misc.js b/test/jsdom/misc.js
@@ -88,6 +88,10 @@ describe("jsdom/miscellaneous", () => {
     assert.equal(window.a, 1);
   });
 
+  specify("bad <style> tag contents do not cause exceptions (GH-1477)", () => {
+    assert.doesNotThrow(() => jsdom.jsdom("<style>[}]</style>"));
+  });
+
   specify("jquerify_url", { async: true }, t => {
     const jQueryUrl = "http://code.jquery.com/jquery-1.4.4.min.js";
     jsdom.jQueryify(tmpWindow(), jQueryUrl, (window, jQuery) => {