Bug 569719 part 16: Don't refcount declarations and data blocks; don't have style rules hold direct pointers to data blocks. r+a=dbaron
authorZack Weinberg <zweinberg@mozilla.com>
Fri, 23 Jul 2010 11:00:52 -0700
changeset 48864 6f0dae17da5367e45f84c32def6093d45209c21c
parent 48863 5f209226dc8660dd1cbe2084a59ed44b2b81ff9f
child 48865 79d6ab3394845924fc0002b318b667aff689f2b2
push idunknown
push userunknown
push dateunknown
bugs569719
milestone2.0b4pre
Bug 569719 part 16: Don't refcount declarations and data blocks; don't have style rules hold direct pointers to data blocks. r+a=dbaron
content/base/test/test_bug338679.html
layout/style/Declaration.cpp
layout/style/Declaration.h
layout/style/nsCSSDataBlock.cpp
layout/style/nsCSSDataBlock.h
layout/style/nsCSSStyleRule.cpp
layout/style/nsDOMCSSDeclaration.cpp
layout/style/nsStyleAnimation.cpp
--- a/content/base/test/test_bug338679.html
+++ b/content/base/test/test_bug338679.html
@@ -31,17 +31,17 @@ New:            width: auto;
 </div>
 <pre id="test">
 <script>
 
 /* This is our event handler function */
 function attr_modified(ev) {
     $("out").textContent = "Previous:\t" + ev.prevValue + "\nNew:\t\t" + ev.newValue;
     is(ev.newValue, "width: auto;", "DOMAttrModified event reports correct newValue");
-    todo(ev.prevValue == "width: 20em;", "DOMAttrModified event reports correct prevValue");
+    is(ev.prevValue == "width: 20em;", "DOMAttrModified event reports correct prevValue");
     SimpleTest.finish(); // trigger the end of our test sequence
 }
 
 /* Call this to tell SimpleTest to wait for SimpleTest.finish() */
 SimpleTest.waitForExplicitFinish();
 
 $("out").addEventListener("DOMAttrModified", attr_modified, false);
 $("out").style.width = "auto";
--- a/layout/style/Declaration.cpp
+++ b/layout/style/Declaration.cpp
@@ -57,32 +57,32 @@
 #include "nsReadableUtils.h"
 #include "nsStyleUtil.h"
 #include "nsStyleConsts.h"
 #include "nsCOMPtr.h"
 
 namespace mozilla {
 namespace css {
 
+// check that we can fit all the CSS properties into a PRUint8
+// for the mOrder array - if not, might need to use PRUint16!
+PR_STATIC_ASSERT(eCSSProperty_COUNT_no_shorthands - 1 <= PR_UINT8_MAX);
+
 Declaration::Declaration()
+  : mImmutable(PR_FALSE)
 {
-  // check that we can fit all the CSS properties into a PRUint8
-  // for the mOrder array - if not, might need to use PRUint16!
-  PR_STATIC_ASSERT(eCSSProperty_COUNT_no_shorthands - 1 <= PR_UINT8_MAX);
-
   MOZ_COUNT_CTOR(mozilla::css::Declaration);
 }
 
 Declaration::Declaration(const Declaration& aCopy)
   : mOrder(aCopy.mOrder),
-    mData(aCopy.mData ? aCopy.mData->Clone()
-                      : already_AddRefed<nsCSSCompressedDataBlock>(nsnull)),
+    mData(aCopy.mData ? aCopy.mData->Clone() : nsnull),
     mImportantData(aCopy.mImportantData
-                      ? aCopy.mImportantData->Clone()
-                      : already_AddRefed<nsCSSCompressedDataBlock>(nsnull))
+                   ? aCopy.mImportantData->Clone() : nsnull),
+    mImmutable(PR_FALSE)
 {
   MOZ_COUNT_CTOR(mozilla::css::Declaration);
 }
 
 Declaration::~Declaration()
 {
   MOZ_COUNT_DTOR(mozilla::css::Declaration);
 }
@@ -883,34 +883,28 @@ Declaration::GetNthProperty(PRUint32 aIn
   if (aIndex < mOrder.Length()) {
     nsCSSProperty property = OrderValueAt(aIndex);
     if (0 <= property) {
       AppendASCIItoUTF16(nsCSSProps::GetStringValue(property), aReturn);
     }
   }
 }
 
-Declaration*
-Declaration::Clone() const
-{
-  return new Declaration(*this);
-}
-
 void
 Declaration::InitializeEmpty()
 {
   NS_ASSERTION(!mData && !mImportantData, "already initialized");
   mData = nsCSSCompressedDataBlock::CreateEmptyBlock();
 }
 
 Declaration*
 Declaration::EnsureMutable()
 {
   NS_ASSERTION(mData, "should only be called when not expanded");
   if (!IsMutable()) {
-    return Clone();
+    return new Declaration(*this);
   } else {
     return this;
   }
 }
 
 } // namespace mozilla::css
 } // namespace mozilla
--- a/layout/style/Declaration.h
+++ b/layout/style/Declaration.h
@@ -56,33 +56,40 @@
 #include "nsString.h"
 #include "nsCoord.h"
 #include "nsCSSValue.h"
 #include "nsCSSProps.h"
 #include "nsTArray.h"
 #include "nsCSSDataBlock.h"
 #include "nsCSSStruct.h"
 
-// must be forward-declared in root namespace
-class CSSStyleRuleImpl;
-
 namespace mozilla {
 namespace css {
 
+// Declaration objects have unusual lifetime rules.  Every declaration
+// begins life in an invalid state which ends when InitializeEmpty or
+// CompressFrom is called upon it.  After that, it can be attached to
+// exactly one style rule, and will be destroyed when that style rule
+// is destroyed.  A declaration becomes immutable when its style rule's
+// |RuleMatched| method is called; after that, it must be copied before
+// it can be modified, which is taken care of by |EnsureMutable|.
+
 class Declaration {
 public:
   /**
    * Construct an |Declaration| that is in an invalid state (null
    * |mData|) and cannot be used until its |CompressFrom| method or
    * |InitializeEmpty| method is called.
    */
   Declaration();
 
   Declaration(const Declaration& aCopy);
 
+  ~Declaration();
+
   /**
    * |ValueAppended| must be called to maintain this declaration's
    * |mOrder| whenever a property is parsed into an expanded data block
    * for this declaration.  aProperty must not be a shorthand.
    */
   void ValueAppended(nsCSSProperty aProperty);
 
   void RemoveProperty(nsCSSProperty aProperty);
@@ -95,53 +102,63 @@ public:
 
   PRUint32 Count() const {
     return mOrder.Length();
   }
   void GetNthProperty(PRUint32 aIndex, nsAString& aReturn) const;
 
   void ToString(nsAString& aString) const;
 
-  Declaration* Clone() const;
-
   nsCSSCompressedDataBlock* GetNormalBlock() const { return mData; }
   nsCSSCompressedDataBlock* GetImportantBlock() const { return mImportantData; }
 
   /**
    * Initialize this declaration as holding no data.  Cannot fail.
    */
   void InitializeEmpty();
 
   /**
    * Transfer all of the state from |aExpandedData| into this declaration.
    * After calling, |aExpandedData| should be in its initial state.
    */
   void CompressFrom(nsCSSExpandedDataBlock *aExpandedData) {
     NS_ASSERTION(!mData, "oops");
     NS_ASSERTION(!mImportantData, "oops");
-    aExpandedData->Compress(getter_AddRefs(mData),
-                            getter_AddRefs(mImportantData));
+    aExpandedData->Compress(getter_Transfers(mData),
+                            getter_Transfers(mImportantData));
     aExpandedData->AssertInitialState();
   }
 
   /**
    * Transfer all of the state from this declaration into
    * |aExpandedData| and put this declaration temporarily into an
    * invalid state (ended by |CompressFrom| or |InitializeEmpty|) that
    * should last only during parsing.  During this time only
    * |ValueAppended| should be called.
    */
   void ExpandTo(nsCSSExpandedDataBlock *aExpandedData) {
     AssertMutable();
     aExpandedData->AssertInitialState();
 
     NS_ASSERTION(mData, "oops");
-    aExpandedData->Expand(&mData, &mImportantData);
-    NS_ASSERTION(!mData && !mImportantData,
-                 "Expand didn't null things out");
+    aExpandedData->Expand(mData.forget(), mImportantData.forget());
+  }
+
+  /**
+   * Do what |nsIStyleRule::MapRuleInfoInto| needs to do for a style
+   * rule using this declaration for storage.
+   */
+  void MapNormalRuleInfoInto(nsRuleData *aRuleData) const {
+    NS_ABORT_IF_FALSE(mData, "called while expanded");
+    mData->MapRuleInfoInto(aRuleData);
+  }
+  void MapImportantRuleInfoInto(nsRuleData *aRuleData) const {
+    NS_ABORT_IF_FALSE(mData, "called while expanded");
+    NS_ABORT_IF_FALSE(mImportantData, "must have important data");
+    mImportantData->MapRuleInfoInto(aRuleData);
   }
 
   /**
    * Return a pointer to our current value for this property.
    * Only returns non-null if the property is longhand, set, and
    * has the indicated importance level.
    *
    * May only be called when not expanded, and the caller must call
@@ -172,28 +189,40 @@ public:
   }
 
   PRBool HasNonImportantValueFor(nsCSSProperty aProperty) const {
     NS_ABORT_IF_FALSE(!nsCSSProps::IsShorthand(aProperty), "must be longhand");
     return !!mData->StorageFor(aProperty);
   }
 
   /**
+   * Return whether |this| may be modified.
+   */
+  bool IsMutable() const {
+    return !mImmutable;
+  }
+
+  /**
    * Copy |this|, if necessary to ensure that it can be modified.
    */
   Declaration* EnsureMutable();
 
   /**
-   * Crash if |this| is not mutable.
+   * Crash if |this| cannot be modified.
    */
   void AssertMutable() const {
     NS_ABORT_IF_FALSE(IsMutable(), "someone forgot to call EnsureMutable");
   }
 
   /**
+   * Mark this declaration as unmodifiable.
+   */
+  void SetImmutable() { mImmutable = PR_TRUE; }
+
+  /**
    * Clear the data, in preparation for its replacement with entirely
    * new data by a call to |CompressFrom|.
    */
   void ClearData() {
     AssertMutable();
     mData = nsnull;
     mImportantData = nsnull;
     mOrder.Clear();
@@ -205,75 +234,35 @@ public:
 
 private:
   // Not implemented, and not supported.
   Declaration& operator=(const Declaration& aCopy);
   PRBool operator==(const Declaration& aCopy) const;
 
   static void AppendImportanceToString(PRBool aIsImportant, nsAString& aString);
   // return whether there was a value in |aValue| (i.e., it had a non-null unit)
-  PRBool   AppendValueToString(nsCSSProperty aProperty, nsAString& aResult) const;
+  PRBool AppendValueToString(nsCSSProperty aProperty, nsAString& aResult) const;
   // Helper for ToString with strange semantics regarding aValue.
-  void     AppendPropertyAndValueToString(nsCSSProperty aProperty,
-                                          nsAutoString& aValue,
-                                          nsAString& aResult) const;
-
-private:
-    //
-    // Specialized ref counting.
-    // We do not want everyone to ref count us, only the rules which hold
-    //  onto us (our well defined lifetime is when the last rule releases
-    //  us).
-    // It's worth a comment here that the main css::Declaration is
-    //  refcounted, but its |mImportant| is not refcounted, just owned
-    //  by the non-important declaration.
-    //
-    friend class ::CSSStyleRuleImpl;
-    void AddRef(void) {
-      if (mRefCnt == PR_UINT32_MAX) {
-        NS_WARNING("refcount overflow, leaking object");
-        return;
-      }
-      ++mRefCnt;
-    }
-    void Release(void) {
-      if (mRefCnt == PR_UINT32_MAX) {
-        NS_WARNING("refcount overflow, leaking object");
-        return;
-      }
-      NS_ASSERTION(0 < mRefCnt, "bad Release");
-      if (0 == --mRefCnt) {
-        delete this;
-      }
-    }
-public:
-    void RuleAbort(void) {
-      NS_ASSERTION(0 == mRefCnt, "bad RuleAbort");
-      delete this;
-    }
-private:
-  // Block everyone, except us or a derivative, from deleting us.
-  ~Declaration();
+  void AppendPropertyAndValueToString(nsCSSProperty aProperty,
+                                      nsAutoString& aValue,
+                                      nsAString& aResult) const;
 
   nsCSSProperty OrderValueAt(PRUint32 aValue) const {
     return nsCSSProperty(mOrder.ElementAt(aValue));
   }
 
-private:
-    bool IsMutable() const {
-      return ((!mData || mData->IsMutable()) &&
-              (!mImportantData || mImportantData->IsMutable()));
-    }
+  nsAutoTArray<PRUint8, 8> mOrder;
+
+  // never null, except while expanded, or before the first call to
+  // InitializeEmpty or CompressFrom.
+  nsAutoPtr<nsCSSCompressedDataBlock> mData;
 
-    nsAutoTArray<PRUint8, 8> mOrder;
-    nsAutoRefCnt mRefCnt;
+  // may be null
+  nsAutoPtr<nsCSSCompressedDataBlock> mImportantData;
 
-    // never null, except while expanded
-    nsRefPtr<nsCSSCompressedDataBlock> mData;
-
-    // may be null
-    nsRefPtr<nsCSSCompressedDataBlock> mImportantData;
+  // set by style rules when |RuleMatched| is called
+  PRPackedBool mImmutable;
 };
 
 } // namespace css
 } // namespace mozilla
 
 #endif /* mozilla_css_Declaration_h */
--- a/layout/style/nsCSSDataBlock.cpp
+++ b/layout/style/nsCSSDataBlock.cpp
@@ -418,24 +418,24 @@ nsCSSCompressedDataBlock::StorageFor(nsC
             } break;
         }
     }
     NS_ASSERTION(cursor == cursor_end, "inconsistent data");
 
     return nsnull;
 }
 
-already_AddRefed<nsCSSCompressedDataBlock>
+nsCSSCompressedDataBlock*
 nsCSSCompressedDataBlock::Clone() const
 {
     const char *cursor = Block(), *cursor_end = BlockEnd();
     char *result_cursor;
 
-    nsCSSCompressedDataBlock *result =
-        new(cursor_end - cursor) nsCSSCompressedDataBlock();
+    nsAutoPtr<nsCSSCompressedDataBlock> result
+        (new(cursor_end - cursor) nsCSSCompressedDataBlock());
     if (!result)
         return nsnull;
     result_cursor = result->Block();
 
     while (cursor < cursor_end) {
         nsCSSProperty iProp = PropertyAtCursor(cursor);
         NS_ASSERTION(0 <= iProp && iProp < eCSSProperty_COUNT_no_shorthands,
                      "out of range");
@@ -482,38 +482,36 @@ nsCSSCompressedDataBlock::Clone() const
                     case eCSSType_ValueList:
                         copy = ValueListAtCursor(cursor)->Clone();
                         break;
                     case eCSSType_ValuePairList:
                         copy = ValuePairListAtCursor(cursor)->Clone();
                         break;
                 }
                 if (!copy) {
+                    // so the destructor knows where to stop clearing
                     result->mBlockEnd = result_cursor;
-                    result->Destroy();
                     return nsnull;
                 }
                 PointerAtCursor(result_cursor) = copy;
                 cursor += CDBPointerStorage_advance;
                 result_cursor += CDBPointerStorage_advance;
             } break;
         }
     }
     NS_ASSERTION(cursor == cursor_end, "inconsistent data");
 
     result->mBlockEnd = result_cursor;
     result->mStyleBits = mStyleBits;
     NS_ASSERTION(result->DataSize() == DataSize(), "wrong size");
 
-    result->AddRef();
-    return result;
+    return result.forget();
 }
 
-void
-nsCSSCompressedDataBlock::Destroy()
+nsCSSCompressedDataBlock::~nsCSSCompressedDataBlock()
 {
     const char* cursor = Block();
     const char* cursor_end = BlockEnd();
     while (cursor < cursor_end) {
         nsCSSProperty iProp = PropertyAtCursor(cursor);
         NS_ASSERTION(0 <= iProp && iProp < eCSSProperty_COUNT_no_shorthands,
                      "out of range");
 
@@ -551,25 +549,23 @@ nsCSSCompressedDataBlock::Destroy()
                 nsCSSValuePairList* val = ValuePairListAtCursor(cursor);
                 NS_ASSERTION(val, "oops");
                 delete val;
                 cursor += CDBPointerStorage_advance;
             } break;
         }
     }
     NS_ASSERTION(cursor == cursor_end, "inconsistent data");
-    delete this;
 }
 
-/* static */ already_AddRefed<nsCSSCompressedDataBlock>
+/* static */ nsCSSCompressedDataBlock*
 nsCSSCompressedDataBlock::CreateEmptyBlock()
 {
     nsCSSCompressedDataBlock *result = new(0) nsCSSCompressedDataBlock();
     result->mBlockEnd = result->Block();
-    result->AddRef();
     return result;
 }
 
 /* static */ void
 nsCSSCompressedDataBlock::MoveValue(void *aSource, void *aDest,
                                     nsCSSProperty aPropID,
                                     PRBool* aChanged)
 {
@@ -645,41 +641,25 @@ nsCSSExpandedDataBlock::kOffsetTable[] =
     #define CSS_PROP(name_, id_, method_, flags_, datastruct_, member_, type_, \
                      kwtable_, stylestruct_, stylestructoffset_, animtype_)    \
         offsetof(nsCSSExpandedDataBlock, m##datastruct_.member_),
     #include "nsCSSPropList.h"
     #undef CSS_PROP
 };
 
 void
-nsCSSExpandedDataBlock::DoExpand(nsRefPtr<nsCSSCompressedDataBlock> *aBlock,
+nsCSSExpandedDataBlock::DoExpand(nsCSSCompressedDataBlock *aBlock,
                                  PRBool aImportant)
 {
-    NS_PRECONDITION(*aBlock, "unexpected null block");
-
-    if (!(*aBlock)->IsMutable()) {
-        // FIXME (maybe): We really don't need to clone the block
-        // itself, just all the data inside it.
-        *aBlock = (*aBlock)->Clone();
-        if (!*aBlock) {
-            // Not much we can do; just lose the properties.
-            NS_WARNING("out of memory");
-            return;
-        }
-        NS_ABORT_IF_FALSE((*aBlock)->IsMutable(), "we just cloned it");
-    }
-
     /*
      * Save needless copying and allocation by copying the memory
-     * corresponding to the stored data in the compressed block, and
-     * then, to avoid destructors, deleting the compressed block by
-     * calling |delete| instead of using its |Destroy| method.
+     * corresponding to the stored data in the compressed block.
      */
-    const char* cursor = (*aBlock)->Block();
-    const char* cursor_end = (*aBlock)->BlockEnd();
+    const char* cursor = aBlock->Block();
+    const char* cursor_end = aBlock->BlockEnd();
     while (cursor < cursor_end) {
         nsCSSProperty iProp = PropertyAtCursor(cursor);
         NS_ASSERTION(0 <= iProp && iProp < eCSSProperty_COUNT_no_shorthands,
                      "out of range");
         NS_ASSERTION(!HasPropertyBit(iProp),
                      "compressed block has property multiple times");
         SetPropertyBit(iProp);
         if (aImportant)
@@ -736,30 +716,30 @@ nsCSSExpandedDataBlock::DoExpand(nsRefPt
                 NS_ASSERTION(!*dest, "expanding into non-empty block");
                 *dest = val;
                 cursor += CDBPointerStorage_advance;
             } break;
         }
     }
     NS_ASSERTION(cursor == cursor_end, "inconsistent data");
 
-    NS_ASSERTION((*aBlock)->mRefCnt == 1, "unexpected reference count");
-    delete aBlock->forget().get();
+    // Don't destroy remnants of what we just copied
+    aBlock->mBlockEnd = aBlock->Block();
+    delete aBlock;
 }
 
 void
-nsCSSExpandedDataBlock::Expand(
-                          nsRefPtr<nsCSSCompressedDataBlock> *aNormalBlock,
-                          nsRefPtr<nsCSSCompressedDataBlock> *aImportantBlock)
+nsCSSExpandedDataBlock::Expand(nsCSSCompressedDataBlock *aNormalBlock,
+                               nsCSSCompressedDataBlock *aImportantBlock)
 {
-    NS_PRECONDITION(*aNormalBlock, "unexpected null block");
+    NS_PRECONDITION(aNormalBlock, "unexpected null block");
     AssertInitialState();
 
     DoExpand(aNormalBlock, PR_FALSE);
-    if (*aImportantBlock) {
+    if (aImportantBlock) {
         DoExpand(aImportantBlock, PR_TRUE);
     }
 }
 
 nsCSSExpandedDataBlock::ComputeSizeResult
 nsCSSExpandedDataBlock::ComputeSize()
 {
     ComputeSizeResult result = {0, 0};
@@ -822,21 +802,21 @@ nsCSSExpandedDataBlock::ComputeSize()
     }
     return result;
 }
 
 void
 nsCSSExpandedDataBlock::Compress(nsCSSCompressedDataBlock **aNormalBlock,
                                  nsCSSCompressedDataBlock **aImportantBlock)
 {
-    nsRefPtr<nsCSSCompressedDataBlock> result_normal, result_important;
+    nsAutoPtr<nsCSSCompressedDataBlock> result_normal, result_important;
     char *cursor_normal, *cursor_important;
 
     ComputeSizeResult size = ComputeSize();
-    
+
     result_normal = new(size.normal) nsCSSCompressedDataBlock();
     if (!result_normal) {
         *aNormalBlock = nsnull;
         *aImportantBlock = nsnull;
         return;
     }
     cursor_normal = result_normal->Block();
 
@@ -933,18 +913,18 @@ nsCSSExpandedDataBlock::Compress(nsCSSCo
     if (result_important) {
         result_important->mBlockEnd = cursor_important;
         NS_ASSERTION(result_important->DataSize() == ptrdiff_t(size.important),
                      "size miscalculation");
     }
 
     ClearSets();
     AssertInitialState();
-    result_normal.forget(aNormalBlock);
-    result_important.forget(aImportantBlock);
+    *aNormalBlock = result_normal.forget();
+    *aImportantBlock = result_important.forget();
 }
 
 void
 nsCSSExpandedDataBlock::Clear()
 {
     for (size_t iHigh = 0; iHigh < nsCSSPropertySet::kChunkCount; ++iHigh) {
         if (!mPropertiesSet.HasPropertyInChunk(iHigh))
             continue;
--- a/layout/style/nsCSSDataBlock.h
+++ b/layout/style/nsCSSDataBlock.h
@@ -1,8 +1,9 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
  * http://www.mozilla.org/MPL/
  *
@@ -40,42 +41,42 @@
  */
 
 #ifndef nsCSSDataBlock_h__
 #define nsCSSDataBlock_h__
 
 #include "nsCSSStruct.h"
 #include "nsCSSProps.h"
 #include "nsCSSPropertySet.h"
-#include "nsAutoPtr.h"
 
 struct nsRuleData;
 class nsCSSExpandedDataBlock;
 
 namespace mozilla {
 namespace css {
 class Declaration;
 }
 }
 
 /**
  * An |nsCSSCompressedDataBlock| holds a usually-immutable chunk of
  * property-value data for a CSS declaration block (which we misname a
  * |css::Declaration|).  Mutation is accomplished through
  * |nsCSSExpandedDataBlock| or in some cases via direct slot access.
- *
- * Mutation is forbidden when the reference count is greater than one,
- * since once a style rule has used a compressed data block, mutation of
- * that block is forbidden, and any declarations that want to mutate it
- * need to clone it first.
  */
 class nsCSSCompressedDataBlock {
-public:
+private:
     friend class nsCSSExpandedDataBlock;
-    friend class mozilla::css::Declaration;
+
+    // Only this class (via |CreateEmptyBlock|) or nsCSSExpandedDataBlock
+    // (in |Compress|) can create compressed data blocks.
+    nsCSSCompressedDataBlock() : mStyleBits(0) {}
+
+public:
+    ~nsCSSCompressedDataBlock();
 
     /**
      * Do what |nsIStyleRule::MapRuleInfoInto| needs to do for a style
      * rule using this block for storage.
      */
     void MapRuleInfoInto(nsRuleData *aRuleData) const;
 
     /**
@@ -86,16 +87,23 @@ public:
      *
      * Inefficient (by design).
      *
      * Must not be called for shorthands.
      */
     const void* StorageFor(nsCSSProperty aProperty) const;
 
     /**
+     * As above, but provides mutable access to a value slot.
+     */
+    void* SlotForValue(nsCSSProperty aProperty) {
+      return const_cast<void*>(StorageFor(aProperty));
+    }
+
+    /**
      * A set of slightly more typesafe helpers for the above.  All
      * return null if the value is not present.
      */
     const nsCSSValue* ValueStorageFor(nsCSSProperty aProperty) const {
       NS_ABORT_IF_FALSE(nsCSSProps::kTypeTable[aProperty] == eCSSType_Value,
                         "type mismatch");
       return static_cast<const nsCSSValue*>(StorageFor(aProperty));
     }
@@ -124,41 +132,22 @@ public:
                         "type mismatch");
       return static_cast<const nsCSSValuePairList*const*>(
                StorageFor(aProperty));
     }
 
     /**
      * Clone this block, or return null on out-of-memory.
      */
-    already_AddRefed<nsCSSCompressedDataBlock> Clone() const;
+    nsCSSCompressedDataBlock* Clone() const;
 
     /**
      * Create a new nsCSSCompressedDataBlock holding no declarations.
      */
-    static already_AddRefed<nsCSSCompressedDataBlock> CreateEmptyBlock();
-
-    void AddRef() {
-        NS_ASSERTION(mRefCnt == 0 || mRefCnt == 1,
-                     "unexpected reference count");
-        ++mRefCnt;
-    }
-    void Release() {
-        NS_ASSERTION(mRefCnt == 1 || mRefCnt == 2,
-                     "unexpected reference count");
-        if (--mRefCnt == 0) {
-            Destroy();
-        }
-    }
-
-    PRBool IsMutable() const {
-        NS_ASSERTION(mRefCnt == 1 || mRefCnt == 2,
-                     "unexpected reference count");
-        return mRefCnt < 2;
-    }
+    static nsCSSCompressedDataBlock* CreateEmptyBlock();
 
     /**
      * Does a fast move of aSource to aDest.  The previous value in
      * aDest is cleanly destroyed, and aSource is cleared.  *aChanged
      * is set true if, before the copy, the value at aSource compares
      * unequal to the value at aDest.
      *
      * This can only be used for non-shorthand properties.  The caller
@@ -166,53 +155,39 @@ public:
      * to the right kind of objects for the property id.
      */
     static void MoveValue(void *aSource, void *aDest, nsCSSProperty aPropID,
                           PRBool* aChanged);
 
 private:
     PRInt32 mStyleBits; // the structs for which we have data, according to
                         // |nsCachedStyleData::GetBitForSID|.
-    nsAutoRefCnt mRefCnt;
 
     enum { block_chars = 4 }; // put 4 chars in the definition of the class
                               // to ensure size not inflated by alignment
 
     void* operator new(size_t aBaseSize, size_t aDataSize) {
         // subtract off the extra size to store |mBlock_|
         return ::operator new(aBaseSize + aDataSize -
                               sizeof(char) * block_chars);
     }
 
-    nsCSSCompressedDataBlock() : mStyleBits(0) {}
-
-    // Only this class (through |Destroy|) or nsCSSExpandedDataBlock (in
-    // |Expand|) can delete compressed data blocks.
-    ~nsCSSCompressedDataBlock() { }
-
     /**
      * Delete all the data stored in this block, and the block itself.
      */
     void Destroy();
 
     char* mBlockEnd; // the byte after the last valid byte
     char mBlock_[block_chars]; // must be the last member!
 
     char* Block() { return mBlock_; }
     char* BlockEnd() { return mBlockEnd; }
     const char* Block() const { return mBlock_; }
     const char* BlockEnd() const { return mBlockEnd; }
     ptrdiff_t DataSize() const { return BlockEnd() - Block(); }
-
-    // Direct slot access to our values.  See StorageFor above.  Can
-    // return null.  Must not be called for shorthand properties.
-    void* SlotForValue(nsCSSProperty aProperty) {
-      NS_ABORT_IF_FALSE(IsMutable(), "must be mutable");
-      return const_cast<void*>(StorageFor(aProperty));
-    }
 };
 
 class nsCSSExpandedDataBlock {
 public:
     nsCSSExpandedDataBlock();
     ~nsCSSExpandedDataBlock();
     /*
      * When setting properties in an |nsCSSExpandedDataBlock|, callers
@@ -232,38 +207,37 @@ public:
     nsCSSAural mAural;
     nsCSSPage mPage;
     nsCSSBreaks mBreaks;
     nsCSSXUL mXUL;
     nsCSSSVG mSVG;
     nsCSSColumn mColumn;
 
     /**
-     * Transfer all of the state from the compressed block to this
-     * expanded block.  The state of this expanded block must be clear
+     * Transfer all of the state from a pair of compressed data blocks
+     * to this expanded block.  This expanded block must be clear
      * beforehand.
      *
-     * The compressed block passed in IS RELEASED by this method and
-     * set to null, and thus cannot be used again.  (This is necessary
-     * because ownership of sub-objects is transferred to the expanded
-     * block in many cases.)
+     * This method DELETES both of the compressed data blocks it is
+     * passed.  (This is necessary because ownership of sub-objects
+     * is transferred to the expanded block.)
      */
-    void Expand(nsRefPtr<nsCSSCompressedDataBlock> *aNormalBlock,
-                nsRefPtr<nsCSSCompressedDataBlock> *aImportantBlock);
+    void Expand(nsCSSCompressedDataBlock *aNormalBlock,
+                nsCSSCompressedDataBlock *aImportantBlock);
 
     /**
      * Allocate a new compressed block and transfer all of the state
      * from this expanded block to the new compressed block, clearing
      * the state of this expanded block.
      */
     void Compress(nsCSSCompressedDataBlock **aNormalBlock,
                   nsCSSCompressedDataBlock **aImportantBlock);
 
     /**
-     * Clear (and thus destroy) the state of this expanded block.
+     * Clear the state of this expanded block.
      */
     void Clear();
 
     /**
      * Clear the data for the given property (including the set and
      * important bits).  Can be used with shorthand properties.
      */
     void ClearProperty(nsCSSProperty aPropID);
@@ -304,18 +278,17 @@ private:
      * Compute the size that will be occupied by the result of
      * |Compress|.
      */
     struct ComputeSizeResult {
         PRUint32 normal, important;
     };
     ComputeSizeResult ComputeSize();
 
-    void DoExpand(nsRefPtr<nsCSSCompressedDataBlock> *aBlock,
-                  PRBool aImportant);
+    void DoExpand(nsCSSCompressedDataBlock *aBlock, PRBool aImportant);
 
     /**
      * Worker for TransferFromBlock; cannot be used with shorthands.
      */
     void DoTransferFromBlock(nsCSSExpandedDataBlock& aFromBlock,
                              nsCSSProperty aPropID,
                              PRBool aIsImportant,
                              PRBool aOverrideImportant,
--- a/layout/style/nsCSSStyleRule.cpp
+++ b/layout/style/nsCSSStyleRule.cpp
@@ -895,60 +895,63 @@ nsCSSSelectorList::Clone(PRBool aDeep) c
 }
 
 // -- CSSImportantRule -------------------------------
 
 class CSSStyleRuleImpl;
 
 class CSSImportantRule : public nsIStyleRule {
 public:
-  CSSImportantRule(nsCSSCompressedDataBlock *aImportantBlock);
+  CSSImportantRule(css::Declaration *aDeclaration);
 
   NS_DECL_ISUPPORTS
 
   // nsIStyleRule interface
   virtual void MapRuleInfoInto(nsRuleData* aRuleData);
 #ifdef DEBUG
   virtual void List(FILE* out = stdout, PRInt32 aIndent = 0) const;
 #endif
 
 protected:
   virtual ~CSSImportantRule(void);
 
-  nsRefPtr<nsCSSCompressedDataBlock> mImportantBlock;
+  // Not an owning reference; the CSSStyleRuleImpl that owns this
+  // CSSImportantRule also owns the mDeclaration, and any rule node
+  // pointing to this rule keeps that CSSStyleRuleImpl alive as well.
+  css::Declaration* mDeclaration;
 
   friend class CSSStyleRuleImpl;
 };
 
-CSSImportantRule::CSSImportantRule(nsCSSCompressedDataBlock* aImportantBlock)
-  : mImportantBlock(aImportantBlock)
+CSSImportantRule::CSSImportantRule(css::Declaration* aDeclaration)
+  : mDeclaration(aDeclaration)
 {
 }
 
 CSSImportantRule::~CSSImportantRule(void)
 {
 }
 
 NS_IMPL_ISUPPORTS1(CSSImportantRule, nsIStyleRule)
 
 /* virtual */ void
 CSSImportantRule::MapRuleInfoInto(nsRuleData* aRuleData)
 {
-  mImportantBlock->MapRuleInfoInto(aRuleData);
+  mDeclaration->MapImportantRuleInfoInto(aRuleData);
 }
 
 #ifdef DEBUG
 /* virtual */ void
 CSSImportantRule::List(FILE* out, PRInt32 aIndent) const
 {
   // Indent
   for (PRInt32 index = aIndent; --index >= 0; ) fputs("  ", out);
 
-  fprintf(out, "! Important rule block=%p\n",
-          static_cast<void*>(mImportantBlock.get()));
+  fprintf(out, "! Important declaration=%p\n",
+          static_cast<void*>(mDeclaration));
 }
 #endif
 
 // --------------------------------------------------------
 
 class DOMCSSStyleRuleImpl;
 
 class DOMCSSDeclarationImpl : public nsDOMCSSDeclaration
@@ -1325,45 +1328,41 @@ private:
   CSSStyleRuleImpl& operator=(const CSSStyleRuleImpl& aCopy);
 
 protected:
   virtual ~CSSStyleRuleImpl(void);
 
 protected:
   nsCSSSelectorList*      mSelector; // null for style attribute
   css::Declaration*       mDeclaration;
-  nsRefPtr<nsCSSCompressedDataBlock> mNormalBlock;
-  CSSImportantRule*       mImportantRule;
+  CSSImportantRule*       mImportantRule; // initialized by RuleMatched
   DOMCSSStyleRuleImpl*    mDOMRule;
   PRUint32                mLineNumber;
 };
 
 CSSStyleRuleImpl::CSSStyleRuleImpl(nsCSSSelectorList* aSelector,
                                    css::Declaration* aDeclaration)
   : nsCSSRule(),
     mSelector(aSelector),
     mDeclaration(aDeclaration),
     mImportantRule(nsnull),
     mDOMRule(nsnull),
     mLineNumber(0)
 {
-  mDeclaration->AddRef();
 }
 
 // for |Clone|
 CSSStyleRuleImpl::CSSStyleRuleImpl(const CSSStyleRuleImpl& aCopy)
   : nsCSSRule(aCopy),
     mSelector(aCopy.mSelector ? aCopy.mSelector->Clone() : nsnull),
-    mDeclaration(aCopy.mDeclaration->Clone()),
+    mDeclaration(new css::Declaration(*aCopy.mDeclaration)),
     mImportantRule(nsnull),
     mDOMRule(nsnull),
     mLineNumber(aCopy.mLineNumber)
 {
-  if (mDeclaration)
-    mDeclaration->AddRef();
   // rest is constructed lazily on existing data
 }
 
 // for |SetCSSDeclaration|
 CSSStyleRuleImpl::CSSStyleRuleImpl(CSSStyleRuleImpl& aCopy,
                                    css::Declaration* aDeclaration)
   : nsCSSRule(aCopy),
     mSelector(aCopy.mSelector),
@@ -1371,48 +1370,34 @@ CSSStyleRuleImpl::CSSStyleRuleImpl(CSSSt
     mImportantRule(nsnull),
     mDOMRule(aCopy.mDOMRule),
     mLineNumber(aCopy.mLineNumber)
 {
   // The DOM rule is replacing |aCopy| with |this|, so transfer
   // the reverse pointer as well (and transfer ownership).
   aCopy.mDOMRule = nsnull;
 
-  // Transfer ownership of selector and declaration:
-  // NOTE that transferring ownership of the declaration relies on the
-  // code in RuleMatched caching what we need from mDeclaration so that
-  // mDeclaration is unused in CSSStyleRuleImpl::GetImportantRule,
-  // CSSStyleRuleImpl::MapRuleInfoInto, and
-  // CSSImportantRule::MapRuleInfoInto.  This means that any style rule
-  // that has become part of the rule tree has already retrieved the
-  // necessary data blocks from its declaration, so any later
-  // MapRuleInfoInto calls (see stack in bug 209575; also needed for CSS
-  // Transitions) and GetImportantRule calls will also work.
-  if (mDeclaration != aCopy.mDeclaration) {
-    NS_ADDREF(mDeclaration);
-    NS_RELEASE(aCopy.mDeclaration);
+  // Similarly for the selector.
+  aCopy.mSelector = nsnull;
+
+  // We are probably replacing the old declaration with |aDeclaration|
+  // instead of taking ownership of the old declaration; only null out
+  // aCopy.mDeclaration if we are taking ownership.
+  if (mDeclaration == aCopy.mDeclaration) {
+    // This should only ever happen if the declaration was modifiable.
+    mDeclaration->AssertMutable();
+    aCopy.mDeclaration = nsnull;
   }
-  aCopy.mSelector = nsnull;
-  aCopy.mDeclaration = nsnull;
 }
 
 CSSStyleRuleImpl::~CSSStyleRuleImpl(void)
 {
-  if (mSelector) {
-    delete mSelector;
-    mSelector = nsnull;
-  }
-  if (nsnull != mDeclaration) {
-    mDeclaration->Release();
-    mDeclaration = nsnull;
-  }
-  if (nsnull != mImportantRule) {
-    NS_RELEASE(mImportantRule);
-    mImportantRule = nsnull;
-  }
+  delete mSelector;
+  delete mDeclaration;
+  NS_IF_RELEASE(mImportantRule);
   if (mDOMRule) {
     mDOMRule->DOMDeclaration()->DropReference();
     NS_RELEASE(mDOMRule);
   }
 }
 
 // QueryInterface implementation for CSSStyleRuleImpl
 NS_INTERFACE_MAP_BEGIN(CSSStyleRuleImpl)
@@ -1448,27 +1433,23 @@ css::Declaration* CSSStyleRuleImpl::GetD
 nsIStyleRule* CSSStyleRuleImpl::GetImportantRule(void)
 {
   return mImportantRule;
 }
 
 /* virtual */ void
 CSSStyleRuleImpl::RuleMatched()
 {
-  if (mNormalBlock) {
-    // This method has already been called.
-    return;
-  }
-  NS_ABORT_IF_FALSE(!mImportantRule, "should not have important rule either");
+  if (mDeclaration->IsMutable()) {
+    NS_ABORT_IF_FALSE(!mImportantRule, "should not have important rule yet");
 
-  mNormalBlock = mDeclaration->GetNormalBlock();
-
-  if (mDeclaration->HasImportantData()) {
-    mImportantRule = new CSSImportantRule(mDeclaration->GetImportantBlock());
-    NS_IF_ADDREF(mImportantRule);
+    mDeclaration->SetImmutable();
+    if (mDeclaration->HasImportantData()) {
+      NS_ADDREF(mImportantRule = new CSSImportantRule(mDeclaration));
+    }
   }
 }
 
 /* virtual */ already_AddRefed<nsIStyleSheet>
 CSSStyleRuleImpl::GetStyleSheet() const
 {
 // XXX What about inner, etc.
   return nsCSSRule::GetStyleSheet();
@@ -1546,19 +1527,19 @@ CSSStyleRuleImpl::DeclarationChanged(css
   }
 
   return clone;
 }
 
 /* virtual */ void
 CSSStyleRuleImpl::MapRuleInfoInto(nsRuleData* aRuleData)
 {
-  NS_ABORT_IF_FALSE(mNormalBlock,
+  NS_ABORT_IF_FALSE(!mDeclaration->IsMutable(),
                     "somebody forgot to call nsICSSStyleRule::RuleMatched");
-  mNormalBlock->MapRuleInfoInto(aRuleData);
+  mDeclaration->MapNormalRuleInfoInto(aRuleData);
 }
 
 #ifdef DEBUG
 /* virtual */ void
 CSSStyleRuleImpl::List(FILE* out, PRInt32 aIndent) const
 {
   // Indent
   for (PRInt32 index = aIndent; --index >= 0; ) fputs("  ", out);
--- a/layout/style/nsDOMCSSDeclaration.cpp
+++ b/layout/style/nsDOMCSSDeclaration.cpp
@@ -141,28 +141,27 @@ nsDOMCSSDeclaration::SetCssText(const ns
 
   // For nsDOMCSSAttributeDeclaration, SetCSSDeclaration will lead to
   // Attribute setting code, which leads in turn to BeginUpdate.  We
   // need to start the update now so that the old rule doesn't get used
   // between when we mutate the declaration and when we set the new
   // rule (see stack in bug 209575).
   mozAutoDocConditionalContentUpdateBatch autoUpdate(DocToUpdate(), PR_TRUE);
 
-  css::Declaration* decl = new css::Declaration();
+  nsAutoPtr<css::Declaration> decl(new css::Declaration());
   decl->InitializeEmpty();
   nsCSSParser cssParser(cssLoader);
   PRBool changed;
   result = cssParser.ParseDeclarations(aCssText, sheetURI, baseURI,
                                        sheetPrincipal, decl, &changed);
   if (NS_FAILED(result) || !changed) {
-    decl->RuleAbort();
     return result;
   }
 
-  return SetCSSDeclaration(decl);
+  return SetCSSDeclaration(decl.forget());
 }
 
 NS_IMETHODIMP
 nsDOMCSSDeclaration::GetLength(PRUint32* aLength)
 {
   css::Declaration* decl = GetCSSDeclaration(PR_FALSE);
 
   if (decl) {
@@ -305,17 +304,17 @@ nsDOMCSSDeclaration::ParsePropertyValue(
 
   nsCSSParser cssParser(cssLoader);
   PRBool changed;
   result = cssParser.ParseProperty(aPropID, aPropValue, sheetURI, baseURI,
                                    sheetPrincipal, decl, &changed,
                                    aIsImportant);
   if (NS_FAILED(result) || !changed) {
     if (decl != olddecl) {
-      decl->RuleAbort();
+      delete decl;
     }
     return result;
   }
 
   return SetCSSDeclaration(decl);
 }
 
 nsresult
--- a/layout/style/nsStyleAnimation.cpp
+++ b/layout/style/nsStyleAnimation.cpp
@@ -1604,17 +1604,17 @@ nsStyleAnimation::AddWeighted(nsCSSPrope
 
 already_AddRefed<nsICSSStyleRule>
 BuildStyleRule(nsCSSProperty aProperty,
                nsIContent* aTargetElement,
                const nsAString& aSpecifiedValue,
                PRBool aUseSVGMode)
 {
   // Set up an empty CSS Declaration
-  css::Declaration* declaration = new css::Declaration();
+  nsAutoPtr<css::Declaration> declaration(new css::Declaration());
   declaration->InitializeEmpty();
 
   PRBool changed; // ignored, but needed as outparam for ParseProperty
   nsIDocument* doc = aTargetElement->GetOwnerDoc();
   nsCOMPtr<nsIURI> baseURI = aTargetElement->GetBaseURI();
   nsCSSParser parser(doc->CSSLoader());
 
   if (aUseSVGMode) {
@@ -1633,21 +1633,20 @@ BuildStyleRule(nsCSSProperty aProperty,
   if (!parser ||
       NS_FAILED(parser.ParseProperty(aProperty, aSpecifiedValue,
                                      doc->GetDocumentURI(), baseURI,
                                      aTargetElement->NodePrincipal(),
                                      declaration, &changed, PR_FALSE)) ||
       // check whether property parsed without CSS parsing errors
       !declaration->HasNonImportantValueFor(propertyToCheck)) {
     NS_WARNING("failure in BuildStyleRule");
-    declaration->RuleAbort();  // deletes declaration
     return nsnull;
   }
 
-  return NS_NewCSSStyleRule(nsnull, declaration);
+  return NS_NewCSSStyleRule(nsnull, declaration.forget());
 }
 
 inline
 already_AddRefed<nsStyleContext>
 LookupStyleContext(nsIContent* aElement)
 {
   nsIDocument* doc = aElement->GetCurrentDoc();
   nsIPresShell* shell = doc->GetShell();