Bug 386640: ClearStyleDataAndReflow is fundamentally broken. r+sr=bz

layout/base/nsPresContext.cpp

@@ -77,6 +77,8 @@
 #include "nsFrameManager.h"
 #include "nsLayoutUtils.h"
 #include "nsIViewManager.h"
+#include "nsCSSFrameConstructor.h"
+#include "nsStyleChangeList.h"
 
 #ifdef IBMBIDI
 #include "nsBidiPresUtils.h"
@@ -642,15 +644,28 @@ nsPresContext::GetUserPreferences()
 void
 nsPresContext::ClearStyleDataAndReflow()
 {
-  if (mShell) {
-    // Clear out all our style data.
-    mShell->StyleSet()->ClearStyleData(this);
-
-    // Force a reflow of the root frame
-    // XXX We really should only do a reflow if a preference that affects
-    // formatting changed, e.g., a font change. If it's just a color change
-    // then we only need to repaint...
-    mShell->StyleChangeReflow();
+  // This method is used to recompute the style data when some change happens
+  // outside of any style rules, like a color preference change or a change
+  // in a system font size
+  if (mShell && mShell->GetRootFrame()) {
+    // Tell the style set to get the old rule tree out of the way
+    // so we can recalculate while maintaining rule tree immutability
+    nsresult rv = mShell->StyleSet()->BeginReconstruct();
+    if (NS_FAILED(rv))
+      return;
+    // Recalculate all of the style contexts for the document
+    // Note that we can ignore the return value of ComputeStyleChangeFor
+    // because we never need to reframe the root frame
+    // XXX This could be made faster by not rerunning rule matching
+    // (but note that nsPresShell::SetPreferenceStyleRules currently depends
+    // on us re-running rule matching here
+    nsStyleChangeList changeList;
+    mShell->FrameManager()->ComputeStyleChangeFor(mShell->GetRootFrame(),
+                                                  &changeList, nsChangeHint(0));
+    // Tell the style set it's safe to destroy the old rule tree
+    mShell->StyleSet()->EndReconstruct();
+    // Tell the frame constructor to process the required changes
+    mShell->FrameConstructor()->ProcessRestyledFrames(changeList);
   }
 }
 

layout/base/nsPresShell.cpp

@@ -1794,11 +1794,10 @@ PresShell::SetPreferenceStyleRules(PRBool aForceReflow)
     }
 #ifdef DEBUG_attinasi
     printf( "Preference Style Rules set: error=%ld\n", (long)result);
-#endif    
+#endif
 
-    if (aForceReflow){
-      mPresContext->ClearStyleDataAndReflow();
-    }
+    // Note that this method never needs to force any calculation; the caller
+    // will recalculate style if needed
 
     return result;
   }

layout/style/nsRuleNode.cpp

@@ -557,35 +557,6 @@ nsRuleNode::ConvertChildrenToHash()
   SetChildrenHash(hash);
 }
 
-PR_STATIC_CALLBACK(PLDHashOperator)
-ClearStyleDataHelper(PLDHashTable *table, PLDHashEntryHdr *hdr,
-                               PRUint32 number, void *arg)
-{
-  ChildrenHashEntry *entry = static_cast<ChildrenHashEntry*>(hdr);
-  entry->mRuleNode->ClearStyleData();
-  return PL_DHASH_NEXT;
-}
-
-nsresult
-nsRuleNode::ClearStyleData()
-{
-  // Blow away all data stored at this node.
-  if (mStyleData.mResetData || mStyleData.mInheritedData)
-    mStyleData.Destroy(0, mPresContext);
-
-  mNoneBits &= ~NS_STYLE_INHERIT_MASK;
-  mDependentBits &= ~NS_STYLE_INHERIT_MASK;
-
-  if (ChildrenAreHashed())
-    PL_DHashTableEnumerate(ChildrenHash(),
-                           ClearStyleDataHelper, nsnull);
-  else
-    for (nsRuleList* curr = ChildrenList(); curr; curr = curr->mNext)
-      curr->mRuleNode->ClearStyleData();
-
-  return NS_OK;
-}
-
 inline void
 nsRuleNode::PropagateNoneBit(PRUint32 aBit, nsRuleNode* aHighestNode)
 {

layout/style/nsRuleNode.h

@@ -693,7 +693,6 @@ public:
   // NOTE: Does not |AddRef|.
   nsPresContext* GetPresContext() const { return mPresContext; }
 
-  NS_HIDDEN_(nsresult) ClearStyleData();
   NS_HIDDEN_(const nsStyleStruct*) GetStyleData(nsStyleStructID aSID, 
                                                 nsStyleContext* aContext,
                                                 PRBool aComputeData);

layout/style/nsStyleContext.cpp

@@ -384,34 +384,6 @@ nsStyleContext::ApplyStyleFixups(nsPresContext* aPresContext)
   GetStyleUserInterface();
 }
 
-void
-nsStyleContext::ClearStyleData(nsPresContext* aPresContext)
-{
-  // First we need to clear out all of our style data.
-  if (mCachedStyleData.mResetData || mCachedStyleData.mInheritedData)
-    mCachedStyleData.Destroy(mBits, aPresContext);
-
-  mBits = 0; // Clear all bits.
-
-  ApplyStyleFixups(aPresContext);
-
-  if (mChild) {
-    nsStyleContext* child = mChild;
-    do {
-      child->ClearStyleData(aPresContext);
-      child = child->mNextSibling;
-    } while (mChild != child);
-  }
-  
-  if (mEmptyChild) {
-    nsStyleContext* child = mEmptyChild;
-    do {
-      child->ClearStyleData(aPresContext);
-      child = child->mNextSibling;
-    } while (mEmptyChild != child);
-  }
-}
-
 nsChangeHint
 nsStyleContext::CalcStyleDifference(nsStyleContext* aOther)
 {

layout/style/nsStyleContext.h

@@ -158,8 +158,6 @@ public:
 
   NS_HIDDEN_(nsStyleStruct*) GetUniqueStyleData(const nsStyleStructID& aSID);
 
-  NS_HIDDEN_(void) ClearStyleData(nsPresContext* aPresContext);
-
   NS_HIDDEN_(nsChangeHint) CalcStyleDifference(nsStyleContext* aOther);
 
 #ifdef DEBUG

layout/style/nsStyleSet.cpp

@@ -63,6 +63,7 @@ nsStyleSet::nsStyleSet()
     mRuleWalker(nsnull),
     mDestroyedCount(0),
     mBatching(0),
+    mOldRuleTree(nsnull),
     mInShutdown(PR_FALSE),
     mAuthorStyleDisabled(PR_FALSE),
     mDirty(0)
@@ -99,6 +100,48 @@ nsStyleSet::Init(nsPresContext *aPresContext)
   return NS_OK;
 }
 
+nsresult
+nsStyleSet::BeginReconstruct()
+{
+  NS_ASSERTION(!mOldRuleTree, "Unmatched begin/end?");
+  NS_ASSERTION(mRuleTree, "Reconstructing before first construction?");
+
+  // Create a new rule tree root
+  nsRuleNode* newTree =
+    nsRuleNode::CreateRootNode(mRuleTree->GetPresContext());
+  if (!newTree)
+    return NS_ERROR_OUT_OF_MEMORY;
+  nsRuleWalker* ruleWalker = new nsRuleWalker(newTree);
+  if (!ruleWalker) {
+    newTree->Destroy();
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+
+  // Save the old rule tree so we can destroy it later
+  mOldRuleTree = mRuleTree;
+  // Delete mRuleWalker because it holds a reference to the rule tree root
+  delete mRuleWalker;
+  // Clear out the old style contexts; we don't need them anymore
+  mRoots.Clear();
+
+  mRuleTree = newTree;
+  mRuleWalker = ruleWalker;
+
+  return NS_OK;
+}
+
+void
+nsStyleSet::EndReconstruct()
+{
+  NS_ASSERTION(mOldRuleTree, "Unmatched begin/end?");
+  // Reset the destroyed count; it's no longer valid
+  mDestroyedCount = 0;
+  // Destroy the old rule tree (all the associated style contexts should have
+  // been destroyed by the caller beforehand)
+  mOldRuleTree->Destroy();
+  mOldRuleTree = nsnull;
+}
+
 nsresult
 nsStyleSet::GatherRuleProcessors(sheetType aType)
 {
@@ -785,6 +828,9 @@ nsStyleSet::NotifyStyleContextDestroyed(nsPresContext* aPresContext,
     return;
 
   NS_ASSERTION(mRuleWalker->AtRoot(), "Rule walker should be at root");
+  
+  if (mOldRuleTree)
+    return;
 
   if (!aStyleContext->GetParent()) {
     mRoots.RemoveElement(aStyleContext);
@@ -797,8 +843,8 @@ nsStyleSet::NotifyStyleContextDestroyed(nsPresContext* aPresContext,
     // all descendants.  This will reach style contexts in the
     // undisplayed map and "additional style contexts" since they are
     // descendants of the root.
-    for (PRInt32 i = mRoots.Count() - 1; i >= 0; --i) {
-      static_cast<nsStyleContext*>(mRoots[i])->Mark();
+    for (PRInt32 i = mRoots.Length() - 1; i >= 0; --i) {
+      mRoots[i]->Mark();
     }
 
     // Sweep the rule tree.
@@ -811,16 +857,6 @@ nsStyleSet::NotifyStyleContextDestroyed(nsPresContext* aPresContext,
   }
 }
 
-void
-nsStyleSet::ClearStyleData(nsPresContext* aPresContext)
-{
-  mRuleTree->ClearStyleData();
-
-  for (PRInt32 i = mRoots.Count() - 1; i >= 0; --i) {
-    static_cast<nsStyleContext*>(mRoots[i])->ClearStyleData(aPresContext);
-  }
-}
-
 already_AddRefed<nsStyleContext>
 nsStyleSet::ReParentStyleContext(nsPresContext* aPresContext,
                                  nsStyleContext* aStyleContext, 

layout/style/nsStyleSet.h

@@ -48,9 +48,10 @@
 
 #include "nsIStyleRuleProcessor.h"
 #include "nsICSSStyleSheet.h"
-#include "nsVoidArray.h"
 #include "nsBindingManager.h"
 #include "nsRuleNode.h"
+#include "nsTArray.h"
+#include "nsCOMArray.h"
 
 class nsIURI;
 
@@ -72,9 +73,6 @@ class nsStyleSet
   // To be used only by nsRuleNode.
   nsCachedStyleData* DefaultStyleData() { return &mDefaultStyleData; }
 
-  // clear out all of the computed style data
-  void ClearStyleData(nsPresContext *aPresContext);
-
   // enable / disable the Quirk style sheet
   void EnableQuirkStyleSheet(PRBool aEnable);
 
@@ -192,6 +190,13 @@ class nsStyleSet
   void     BeginUpdate();
   nsresult EndUpdate();
 
+  // Methods for reconstructing the tree; BeginReconstruct basically moves the
+  // old rule tree root and style context roots out of the way,
+  // and EndReconstruct destroys the old rule tree when we're done
+  nsresult BeginReconstruct();
+  // Note: EndReconstruct should not be called if BeginReconstruct fails
+  void EndReconstruct();
+
  private:
   // Not to be implemented
   nsStyleSet(const nsStyleSet& aCopy);
@@ -265,13 +270,17 @@ class nsStyleSet
                              // be used to navigate through our tree.
 
   PRInt32 mDestroyedCount; // used to batch style context GC
-  nsVoidArray mRoots; // style contexts with no parent
+  nsTArray<nsStyleContext*> mRoots; // style contexts with no parent
 
   PRUint16 mBatching;
 
+  nsRuleNode* mOldRuleTree; // Old rule tree; used during tree reconstruction
+                            // (See BeginReconstruct and EndReconstruct)
+
   unsigned mInShutdown : 1;
   unsigned mAuthorStyleDisabled: 1;
   unsigned mDirty : 7;  // one dirty bit is used per sheet type
+
 };
 
 #endif

layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

@@ -4093,6 +4093,20 @@ nsTreeBodyFrame::ClearStyleAndImageCaches()
   return NS_OK;
 }
 
+NS_IMETHODIMP
+nsTreeBodyFrame::DidSetStyleContext()
+{
+  // Clear the style cache; the pointers are no longer even valid
+  mStyleCache.Clear();
+  // XXX The following is hacky, but it's not incorrect,
+  // and appears to fix a few bugs with style changes, like text zoom and
+  // dpi changes
+  mIndentation = GetIndentation();
+  mRowHeight = GetRowHeight();
+  mStringWidth = -1;
+  return NS_OK;
+}
+
 PRBool 
 nsTreeBodyFrame::OffsetForHorzScroll(nsRect& rect, PRBool clip)
 {

layout/xul/base/src/tree/src/nsTreeBodyFrame.h

@@ -120,6 +120,8 @@ public:
                               const nsRect&           aDirtyRect,
                               const nsDisplayListSet& aLists);
 
+  NS_IMETHOD DidSetStyleContext();
+
   friend nsIFrame* NS_NewTreeBodyFrame(nsIPresShell* aPresShell);
 
   struct ScrollParts {