386640 - ClearStyleDataAndReflow is fundamentally broken

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[Eli Friedman]

Attachment: WIP

I need to add comments and stuff, but is there anything wrong with this approach?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 3

[Eli Friedman]

Attachment: Patch v1

Okay, here's the completed version; probably better to get another set of eyes on it.

Comment 4

[Boris Zbarsky [: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.

Comment 5

[Eli Friedman]

(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.

Comment 6

[Eli Friedman]

Attachment: Patch v2

Comment 7

[Eli Friedman]

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.

Comment 8

[Eli Friedman]

Attachment: 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).

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 10

[Eli Friedman]

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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not a style system peer and I probably shouldn't review this.

Comment 12

[Boris Zbarsky [: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.

Comment 13

[Eli Friedman]

Attachment: Final patch

Comment 14

[Eli Friedman]

Checked in.