From 8a55dca08c1308ae2c3af94af4655126965eb36d Mon Sep 17 00:00:00 2001
From: "L. David Baron" <dbaron@dbaron.org>
Date: Mon, 10 Aug 2009 15:52:29 -0700
Subject: [PATCH] Make computed style (and canvas text styling, which shares
 the same code) avoid using style data that was influenced by pseudo-elements.
  (Bug 505515)  r=bzbarsky

---
 layout/style/nsComputedDOMStyle.cpp           | 42 ++++++++++++-----
 layout/style/nsInspectorCSSUtils.cpp          | 12 ++---
 layout/style/test/Makefile.in                 |  1 +
 .../test/test_computed_style_no_pseudo.html   | 45 +++++++++++++++++++
 4 files changed, 83 insertions(+), 17 deletions(-)
 create mode 100644 layout/style/test/test_computed_style_no_pseudo.html

diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
index aa765b863e12..9d74acb3fe63 100644
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -95,8 +95,6 @@ NS_NewComputedDOMStyle(nsIDOMElement *aElement, const nsAString &aPseudoElt,
                        nsIPresShell *aPresShell,
                        nsComputedDOMStyle **aComputedStyle)
 {
-  NS_ENSURE_ARG_POINTER(aComputedStyle);
-
   nsRefPtr<nsComputedDOMStyle> computedStyle;
   if (sCachedComputedDOMStyle) {
     // There's an unused nsComputedDOMStyle cached, use it.
@@ -302,7 +300,6 @@ nsComputedDOMStyle::GetLength(PRUint32* aLength)
 NS_IMETHODIMP
 nsComputedDOMStyle::GetParentRule(nsIDOMCSSRule** aParentRule)
 {
-  NS_ENSURE_ARG_POINTER(aParentRule);
   *aParentRule = nsnull;
 
   return NS_OK;
@@ -332,7 +329,8 @@ NS_IMETHODIMP
 nsComputedDOMStyle::GetPropertyCSSValue(const nsAString& aPropertyName,
                                         nsIDOMCSSValue** aReturn)
 {
-  NS_ENSURE_ARG_POINTER(aReturn);
+  NS_ASSERTION(!mStyleContextHolder, "bad state");
+
   *aReturn = nsnull;
 
   nsCOMPtr<nsIDocument> document = do_QueryReferent(mDocumentWeak);
@@ -377,14 +375,7 @@ nsComputedDOMStyle::GetPropertyCSSValue(const nsAString& aPropertyName,
 
   mOuterFrame = mPresShell->GetPrimaryFrameFor(mContent);
   mInnerFrame = mOuterFrame;
-  if (!mOuterFrame || mPseudo) {
-    // Need to resolve a style context
-    mStyleContextHolder =
-      nsInspectorCSSUtils::GetStyleContextForContent(mContent,
-                                                     mPseudo,
-                                                     mPresShell);
-    NS_ENSURE_TRUE(mStyleContextHolder, NS_ERROR_OUT_OF_MEMORY);
-  } else {
+  if (mOuterFrame && !mPseudo) {
     nsIAtom* type = mOuterFrame->GetType();
     if (type == nsGkAtoms::tableOuterFrame) {
       // If the frame is an outer table frame then we should get the style
@@ -400,6 +391,33 @@ nsComputedDOMStyle::GetPropertyCSSValue(const nsAString& aPropertyName,
     NS_ASSERTION(mStyleContextHolder, "Frame without style context?");
   }
 
+  if (!mStyleContextHolder || mStyleContextHolder->HasPseudoElementData()) {
+#ifdef DEBUG
+    if (mStyleContextHolder) {
+      // We want to check that going through this path because of
+      // HasPseudoElementData is rare, because it slows us down a good
+      // bit.  So check that we're really inside something associated
+      // with a pseudo-element that contains elements.
+      nsStyleContext *topWithPseudoElementData = mStyleContextHolder;
+      while (topWithPseudoElementData->GetParent()->HasPseudoElementData()) {
+        topWithPseudoElementData = topWithPseudoElementData->GetParent();
+      }
+      NS_ASSERTION(nsCSSPseudoElements::PseudoElementContainsElements(
+                     topWithPseudoElementData->GetPseudoType()),
+                   "we should be in a pseudo-element that is expected to "
+                   "contain elements");
+    }
+#endif
+    // Need to resolve a style context
+    mStyleContextHolder =
+      nsInspectorCSSUtils::GetStyleContextForContent(mContent,
+                                                     mPseudo,
+                                                     mPresShell);
+    NS_ENSURE_TRUE(mStyleContextHolder, NS_ERROR_OUT_OF_MEMORY);
+    NS_ASSERTION(mPseudo || !mStyleContextHolder->HasPseudoElementData(),
+                 "should not have pseudo-element data");
+  }
+
   // Call our pointer-to-member-function.
   nsresult rv = (this->*(propEntry->mGetter))(aReturn);
 
diff --git a/layout/style/nsInspectorCSSUtils.cpp b/layout/style/nsInspectorCSSUtils.cpp
index 0e38d7f8d38f..6d2263101760 100644
--- a/layout/style/nsInspectorCSSUtils.cpp
+++ b/layout/style/nsInspectorCSSUtils.cpp
@@ -101,8 +101,6 @@ nsStyleContext*
 nsInspectorCSSUtils::GetStyleContextForFrame(nsIFrame* aFrame)
 {
     nsStyleContext* styleContext = aFrame->GetStyleContext();
-    if (!styleContext)
-        return nsnull;
 
     /* For tables the primary frame is the "outer frame" but the style
      * rules are applied to the "inner frame".  Luckily, the "outer
@@ -126,10 +124,14 @@ nsInspectorCSSUtils::GetStyleContextForContent(nsIContent* aContent,
         nsIFrame* frame = aPresShell->GetPrimaryFrameFor(aContent);
         if (frame) {
             nsStyleContext* result = GetStyleContextForFrame(frame);
-            // this function returns an addrefed style context
-            if (result)
+            // Don't use the style context if it was influenced by
+            // pseudo-elements, since then it's not the primary style
+            // for this element.
+            if (!result->HasPseudoElementData()) {
+                // this function returns an addrefed style context
                 result->AddRef();
-            return result;
+                return result;
+            }
         }
     }
 
diff --git a/layout/style/test/Makefile.in b/layout/style/test/Makefile.in
index 3e7fc8ccb46c..4d30ab4b76ca 100644
--- a/layout/style/test/Makefile.in
+++ b/layout/style/test/Makefile.in
@@ -105,6 +105,7 @@ _TEST_FILES =	test_acid3_test46.html \
 		test_bug499655.xhtml \
 		test_cascade.html \
 		test_compute_data_with_start_struct.html \
+		test_computed_style_no_pseudo.html \
 		test_css_eof_handling.html \
 		test_descriptor_storage.html \
 		test_descriptor_syntax_errors.html \
diff --git a/layout/style/test/test_computed_style_no_pseudo.html b/layout/style/test/test_computed_style_no_pseudo.html
new file mode 100644
index 000000000000..feb518b9647f
--- /dev/null
+++ b/layout/style/test/test_computed_style_no_pseudo.html
@@ -0,0 +1,45 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=505515
+-->
+<head>
+  <title>Test for Bug 505515</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"/>
+  <style type="text/css">
+
+  #display { color: black; background: white; }
+  #display:first-line { color: blue; }
+
+  </style>
+</head>
+<body onload="run()">
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=505515">Mozilla Bug 505515</a>
+<p id="display" style="width: 30em">This <span id="sp">is</span> some text in which the first line is in a different color.</p>
+<pre id="test">
+<script type="application/javascript">
+
+SimpleTest.waitForExplicitFinish();
+
+/** Test for Bug 505515 **/
+
+function run() {
+  var p = document.getElementById("display");
+  var span = document.getElementById("sp");
+
+  isnot(span.offsetWidth, 0,
+        "span should have width (and we flushed layout)");
+  is(getComputedStyle(span, "").color, "rgb(0, 0, 0)",
+     "span should be black");
+  is(getComputedStyle(p, "").color, "rgb(0, 0, 0)",
+     "p should be black too");
+
+  SimpleTest.finish();
+}
+
+</script>
+</pre>
+</body>
+</html>