Audit for places in style rule code that need to check for a null sheet. (Bug 634373) r=bzbarsky
authorL. David Baron <dbaron@dbaron.org>
Mon, 02 May 2011 18:43:44 -0700
changeset 68894 064d7c5425a6b70e0e7d4f3c6aebdf812334b65e
parent 68893 e089a54230bf71965c9c74bb0e3aa9771fd40247
child 68895 86248f7209b70f8a381320075fd2ea7aa858d201
push id19788
push userdbaron@mozilla.com
push dateTue, 03 May 2011 01:45:09 +0000
treeherdermozilla-central@86248f7209b7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs634373
milestone6.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
Audit for places in style rule code that need to check for a null sheet. (Bug 634373) r=bzbarsky
layout/style/StyleRule.cpp
layout/style/nsCSSRules.cpp
layout/style/test/Makefile.in
layout/style/test/test_rules_out_of_sheets.html
--- a/layout/style/StyleRule.cpp
+++ b/layout/style/StyleRule.cpp
@@ -1373,20 +1373,23 @@ StyleRule::DeclarationChanged(Declaratio
   StyleRule* clone = new StyleRule(*this, aDecl);
   if (!clone) {
     return nsnull;
   }
 
   NS_ADDREF(clone); // for return
 
   if (aHandleContainer) {
-    NS_ASSERTION(mSheet, "rule must be in a sheet");
     if (mParentRule) {
-      mSheet->ReplaceRuleInGroup(mParentRule, this, clone);
-    } else {
+      if (mSheet) {
+        mSheet->ReplaceRuleInGroup(mParentRule, this, clone);
+      } else {
+        mParentRule->ReplaceStyleRule(this, clone);
+      }
+    } else if (mSheet) {
       mSheet->ReplaceStyleRule(this, clone);
     }
   }
 
   return clone;
 }
 
 /* virtual */ void
--- a/layout/style/nsCSSRules.cpp
+++ b/layout/style/nsCSSRules.cpp
@@ -539,16 +539,17 @@ GroupRule::GroupRule(const GroupRule& aC
   : Rule(aCopy)
 {
   const_cast<GroupRule&>(aCopy).mRules.EnumerateForwards(CloneRuleInto, &mRules);
   mRules.EnumerateForwards(SetParentRuleReference, this);
 }
 
 GroupRule::~GroupRule()
 {
+  NS_ABORT_IF_FALSE(!mSheet, "SetStyleSheet should have been called");
   mRules.EnumerateForwards(SetParentRuleReference, nsnull);
   if (mRuleCollection) {
     mRuleCollection->DropReference();
   }
 }
 
 IMPL_STYLE_RULE_INHERIT_MAP_RULE_INFO_INTO(GroupRule, Rule)
 
@@ -1903,17 +1904,19 @@ nsCSSKeyframeRule::SetKeyText(const nsAS
   nsTArray<float> newSelectors;
   // FIXME: pass filename and line number
   if (parser.ParseKeyframeSelectorString(aKeyText, nsnull, 0, newSelectors)) {
     newSelectors.SwapElements(mKeys);
   } else {
     // for now, we don't do anything if the parse fails
   }
 
-  mSheet->SetModifiedByChildRule();
+  if (mSheet) {
+    mSheet->SetModifiedByChildRule();
+  }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsCSSKeyframeRule::GetStyle(nsIDOMCSSStyleDeclaration** aStyle)
 {
   if (!mDOMDeclaration) {
@@ -1923,17 +1926,19 @@ nsCSSKeyframeRule::GetStyle(nsIDOMCSSSty
   return NS_OK;
 }
 
 void
 nsCSSKeyframeRule::ChangeDeclaration(mozilla::css::Declaration* aDeclaration)
 {
   mDeclaration = aDeclaration;
 
-  mSheet->SetModifiedByChildRule();
+  if (mSheet) {
+    mSheet->SetModifiedByChildRule();
+  }
 }
 
 // -------------------------------------------
 // nsCSSKeyframesRule
 //
 
 nsCSSKeyframesRule::nsCSSKeyframesRule(const nsCSSKeyframesRule& aCopy)
   // copy everything except our reference count.  GroupRule's copy
@@ -2038,17 +2043,19 @@ nsCSSKeyframesRule::GetName(nsAString& a
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsCSSKeyframesRule::SetName(const nsAString& aName)
 {
   mName = aName;
 
-  mSheet->SetModifiedByChildRule();
+  if (mSheet) {
+    mSheet->SetModifiedByChildRule();
+  }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsCSSKeyframesRule::GetCssRules(nsIDOMCSSRuleList* *aRuleList)
 {
   NS_ADDREF(*aRuleList = GroupRule::GetCssRules());
@@ -2063,17 +2070,19 @@ nsCSSKeyframesRule::InsertRule(const nsA
   // http://lists.w3.org/Archives/Public/www-style/2011Apr/0034.html
   nsCSSParser parser;
 
   // FIXME: pass filename and line number
   nsRefPtr<nsCSSKeyframeRule> rule =
     parser.ParseKeyframeRule(aRule, nsnull, 0);
   if (rule) {
     mRules.AppendObject(rule);
-    mSheet->SetModifiedByChildRule();
+    if (mSheet) {
+      mSheet->SetModifiedByChildRule();
+    }
   }
 
   return NS_OK;
 }
 
 static const PRUint32 RULE_NOT_FOUND = PRUint32(-1);
 
 PRUint32
@@ -2100,17 +2109,19 @@ nsCSSKeyframesRule::FindRuleIndexForKey(
 }
 
 NS_IMETHODIMP
 nsCSSKeyframesRule::DeleteRule(const nsAString& aKey)
 {
   PRUint32 index = FindRuleIndexForKey(aKey);
   if (index != RULE_NOT_FOUND) {
     mRules.RemoveObjectAt(index);
-    mSheet->SetModifiedByChildRule();
+    if (mSheet) {
+      mSheet->SetModifiedByChildRule();
+    }
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsCSSKeyframesRule::FindRule(const nsAString& aKey,
                              nsIDOMMozCSSKeyframeRule** aResult)
 {
--- a/layout/style/test/Makefile.in
+++ b/layout/style/test/Makefile.in
@@ -163,16 +163,17 @@ GARBAGE += css_properties.js
 		test_parse_rule.html \
 		test_parse_url.html \
 		test_pixel_lengths.html \
 		test_pointer-events.html \
 		test_property_database.html \
 		test_priority_preservation.html \
 		test_property_syntax_errors.html \
 		test_rem_unit.html \
+		test_rules_out_of_sheets.html \
 		test_selectors.html \
 		test_selectors_on_anonymous_content.html \
 		test_shorthand_property_getters.html \
 		test_style_struct_copy_constructors.html \
 		test_system_font_serialization.html \
 		test_transitions_and_zoom.html \
 		test_transitions_cancel_near_end.html \
 		test_transitions_computed_values.html \
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_rules_out_of_sheets.html
@@ -0,0 +1,116 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=634373
+-->
+<head>
+  <title>Test for Bug 634373</title>
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=634373">Mozilla Bug 634373</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 634373 **/
+
+function make_rule_and_remove_sheet(text, getter) {
+  var style = document.createElement("style");
+  style.setAttribute("type", "text/css");
+  style.appendChild(document.createTextNode(text));
+  document.head.appendChild(style);
+  var result = style.sheet.cssRules[0];
+  if (getter) {
+    result = getter(result);
+  }
+  document.head.removeChild(style);
+  style = null;
+  SpecialPowers.DOMWindowUtils.garbageCollect();
+  return result;
+}
+
+var gDisplayCS = getComputedStyle(document.getElementById("display"), "");
+
+function keep_rule_alive_by_matching(rule) {
+  // It's the caller's job to guarantee that the rule matches a p.
+  // This just causes a style flush, which in turn keeps the rule alive
+  // until the next style flush.
+  var color = gDisplayCS.color;
+  return rule;
+}
+
+function get_rule_and_child(rule) {
+  return [rule, rule.cssRules[0]];
+}
+
+function get_only_child(rule) {
+  return rule.cssRules[0];
+}
+
+var rule;
+
+// In this case, the rule goes away immediately, so we're testing
+// the DOM wrapper's handling of a null rule, rather than the rule's
+// handling of a null sheet.
+rule = make_rule_and_remove_sheet("p { color: blue }");
+rule.style.color = "";
+try {
+  rule.style.color = "fuchsia";
+} catch(ex) {}
+
+rule = make_rule_and_remove_sheet("p { color: blue }",
+                                  keep_rule_alive_by_matching);
+try {
+  rule.style.color = "";
+} catch(ex) {}
+try {
+  rule.style.color = "fuchsia";
+} catch(ex) {}
+
+rule = make_rule_and_remove_sheet("@media screen { p { color: blue } }",
+                                   get_rule_and_child);
+rule[1].style.color = "";
+try {
+  rule[1].style.color = "fuchsia";
+} catch(ex) {}
+
+// In this case, the rule goes away immediately, so we're testing
+// the DOM wrapper's handling of a null rule, rather than the rule's
+// handling of a null sheet.
+rule = make_rule_and_remove_sheet("@media screen { p { color: blue } }",
+                                   get_only_child);
+rule.style.color = "";
+try {
+  rule.style.color = "fuchsia";
+} catch(ex) {}
+
+rule = make_rule_and_remove_sheet("@media screen { p { color: blue } }",
+                                  function(rule) {
+                                    return keep_rule_alive_by_matching(
+                                             get_only_child(rule));
+                                  });
+try {
+  rule.style.color = "";
+} catch(ex) {}
+try {
+  rule.style.color = "fuchsia";
+} catch(ex) {}
+
+rule = make_rule_and_remove_sheet("@-moz-keyframes a { from { color: blue } }");
+rule.insertRule("from { color: fuchsia}");
+rule.deleteRule("from");
+rule.name = "b";
+rule.cssRules[0].keyText = "50%";
+
+ok(true, "didn't crash");
+
+</script>
+</pre>
+</body>
+</html>