The default bug view has changed. See this FAQ.

ClearStyleDataAndReflow is fundamentally broken

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: dbaron, Assigned: Eli Friedman)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Updated

10 years ago
Blocks: 382422
(Assignee)

Comment 1

10 years ago
Created attachment 270650 [details] [diff] [review]
WIP

I need to add comments and stuff, but is there anything wrong with this approach?
Assignee: nobody → sharparrow1
Status: NEW → ASSIGNED
(Reporter)

Comment 2

10 years ago
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.)
(Assignee)

Comment 3

10 years ago
Created attachment 270679 [details] [diff] [review]
Patch v1

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+
(Assignee)

Updated

10 years ago
Blocks: 388607
(Assignee)

Comment 5

10 years ago
(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
(Assignee)

Updated

10 years ago
Blocks: 388607
(Assignee)

Comment 6

10 years ago
Created attachment 272821 [details] [diff] [review]
Patch v2
Attachment #272821 - Flags: superreview?(roc)
(Assignee)

Comment 7

10 years ago
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)
(Assignee)

Updated

10 years ago
Blocks: 378930
(Assignee)

Comment 8

10 years ago
Created attachment 272997 [details] [diff] [review]
Patch v3

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)
(Reporter)

Comment 9

10 years ago
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.)
(Assignee)

Comment 10

10 years ago
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.
(Assignee)

Updated

10 years ago
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+
(Assignee)

Comment 13

10 years ago
Created attachment 273799 [details] [diff] [review]
Final patch
(Assignee)

Comment 14

10 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Depends on: 389663
(Reporter)

Updated

10 years ago
Depends on: 392335
(Reporter)

Updated

9 years ago
Depends on: 394057
Blocks: 214393
Flags: in-testsuite?

Updated

9 years ago
Depends on: 443629
(Reporter)

Updated

8 years ago
Blocks: 149203
(Reporter)

Updated

8 years ago
Blocks: 153817
You need to log in before you can comment on or make changes to this bug.