Closed Bug 386640 Opened 12 years ago Closed 12 years ago

ClearStyleDataAndReflow is fundamentally broken

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: sharparrow1)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

ClearStyleDataAndReflow and the ClearStyleData methods on nsStyleContext and nsRuleNode are fundamentally broken.  They have the following problems:

* they can cause style changes without calling DidSetStyleContext (see bug 382422)

* they can cause the PeekStyleData optimizations in CalcStyleDifference to optimize away necessary notifications because:
  + the following reflow and potential repaint doesn't get structs that may have been gotten only 
  + people make dynamic changes between the ClearStyleData and the reflow/repaint that is now asynchronous


We should instead do what I describe in bug 382422 comment 4.
Blocks: 382422
Attached patch WIP (obsolete) — Splinter Review
I need to add comments and stuff, but is there anything wrong with this approach?
Assignee: nobody → sharparrow1
Status: NEW → ASSIGNED
Seems like the right idea.  You might need to do something with the result of ComputeStyleChangeFor.  And you should probably add a comment that we could optimize with a faster version of ComputeStyleChangeFor that doesn't rerun rule matching.  (The existing ReParentStyleContext codepath might even work, if we trust it.)
Attached patch Patch v1Splinter Review
Okay, here's the completed version; probably better to get another set of eyes on it.
Attachment #270650 - Attachment is obsolete: true
Attachment #270679 - Flags: review?(bzbarsky)
Comment on attachment 270679 [details] [diff] [review]
Patch v1

>Index: base/nsPresContext.cpp
>Index: base/nsPresShell.cpp
>-    if (aForceReflow){
>-      mPresContext->ClearStyleDataAndReflow();
>-    }

So the aForceReflow arg can go away, right?  Do that or file a followup?

>Index: style/nsStyleSet.cpp
>+    // XXX What should I do for OOM?

Make this method a no-op on OOM.  That is, first allocate the new ruletree and rulewalker, then if that succeeded go ahead and tear down the old stuff.  In the failure case we'll render wrong, but not crash, right?

>+nsStyleSet::EndReconstruct() {
>+  NS_ASSERTION(mOldRuleTree, "Unmatched begin/end?");

And then here mOldRuleTree might be null if BeginReconstruct() failed.  But we could document that if BeginReconstruct() returns an error the caller must not call EndReconstruct().

>+    for (PRInt32 i = mRoots.Length() - 1; i >= 0; --i) 

This doesn't cause warnings from assigning a PRUint32 into a PRInt32?

If it does, you could really iterate this forwards, I think... I assume it was done backwards to avoid calling Length() more than once, and you could store the length in a local.  Check, though?

>Index: style/nsStyleSet.h
>+#include "nsCOMArray.h"

I assume this change is not really for this bug?

>Index: style/nsStyleStruct.h

And same for the changes here?  I'm fine with this landing, but perhaps a separate checkin with a comment like "removing unused code, rs=bzbarsky, no bug" or something?

r=bzbarsky on the patch itself, modulo above nits.
Attachment #270679 - Flags: review?(bzbarsky) → review+
Blocks: 388607
(In reply to comment #4)
> (From update of attachment 270679 [details] [diff] [review])
> >Index: base/nsPresContext.cpp
> >Index: base/nsPresShell.cpp
> >-    if (aForceReflow){
> >-      mPresContext->ClearStyleDataAndReflow();
> >-    }
> 
> So the aForceReflow arg can go away, right?  Do that or file a followup?

Followup filed.

> >Index: style/nsStyleSet.cpp
> >+    // XXX What should I do for OOM?
> 
> Make this method a no-op on OOM.  That is, first allocate the new ruletree and
> rulewalker, then if that succeeded go ahead and tear down the old stuff.  In
> the failure case we'll render wrong, but not crash, right?

Right; done.

> >+nsStyleSet::EndReconstruct() {
> >+  NS_ASSERTION(mOldRuleTree, "Unmatched begin/end?");
> 
> And then here mOldRuleTree might be null if BeginReconstruct() failed.  But we
> could document that if BeginReconstruct() returns an error the caller must not
> call EndReconstruct().

Done.

> >+    for (PRInt32 i = mRoots.Length() - 1; i >= 0; --i) 
> 
> This doesn't cause warnings from assigning a PRUint32 into a PRInt32?

No, at least on Windows.

> >Index: style/nsStyleSet.h
> >+#include "nsCOMArray.h"
> 
> I assume this change is not really for this bug?

Not really, but it's correct in any case.

> >Index: style/nsStyleStruct.h
> 
> And same for the changes here?  I'm fine with this landing, but perhaps a
> separate checkin with a comment like "removing unused code, rs=bzbarsky, no
> bug" or something?

Okay, done.
No longer blocks: 388607
Blocks: 388607
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #272821 - Flags: superreview?(roc)
Comment on attachment 272821 [details] [diff] [review]
Patch v2

Cancelling superreview; there's a couple of typos in this patch, and I found an issue in nsTreeStyleCache.
Attachment #272821 - Flags: superreview?(roc)
Blocks: 378930
Attached patch Patch v3Splinter Review
This patch has a couple of additional changes to nsTreeBodyFrame to fix some issues there (primarily a crash from style contexts with invalid rule node pointers).
Attachment #272821 - Attachment is obsolete: true
Attachment #272997 - Flags: superreview?(roc)
Attachment #272997 - Flags: review?(roc)
Do the optimizations in the tree style cache still work with those changes?  I suspect they may not -- I remember writing a patch much like that once before.  (I think there may have been issues with things like hovering over a tree causing a border on the tree forcing everything inside the tree to be restyled.)
No serious issues with the optimization; we trigger a DidSetStyleContext which appears unnecessary in response to user actions sometimes (maybe in response to focus changes?), but it never triggers during painting, which I think is the most important part of the optimization.
I'm not a style system peer and I probably shouldn't review this.
Attachment #272997 - Flags: superreview?(roc)
Attachment #272997 - Flags: superreview?(bzbarsky)
Attachment #272997 - Flags: review?(roc)
Attachment #272997 - Flags: review?(bzbarsky)
Comment on attachment 272997 [details] [diff] [review]
Patch v3

>Index: style/nsCSSValue.cpp

The changes to this file don't seem to be related to this patch.

>Index: style/nsStyleSet.cpp
>+    mOldRuleTree(nsnull)

This doesn't match the member order in the header; that will cause compile warnings.  Put mOldRuleTree before mInShutdown here, please.

>Index: xul/base/src/tree/src/nsTreeBodyFrame.cpp
>+  // XXX The following is hacky, but it's not incorrect,
>+  // and appears to fix some bugs

Say which bugs please, so people who try to clean it up later know what to test, ok?

r+sr=bzbarsky with those nits.
Attachment #272997 - Flags: superreview?(bzbarsky)
Attachment #272997 - Flags: superreview+
Attachment #272997 - Flags: review?(bzbarsky)
Attachment #272997 - Flags: review+
Attached patch Final patchSplinter Review
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 392335
Depends on: 394057
Flags: in-testsuite?
Depends on: 443629
Blocks: 149203
Blocks: 153817
You need to log in before you can comment on or make changes to this bug.