738190 - "ASSERTION: style context has old rule node" with document.dir, MathML

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: style context has old rule node: 'n == mRuleTree', file layout/style/nsStyleSet.cpp, line 234

###!!! ASSERTION: old rule tree still referenced: 'Not Reached', file layout/style/nsStyleSet.cpp, line 1361

###!!! ASSERTION: must be in the same rule tree as parent: 'r1 == r2', file layout/style/nsStyleContext.cpp, line 95

(Probably not a recent regression. The fuzzer was ignoring the first two assertions due to bug 618428 until it was fixed this week.)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  So I see two style contexts in mRoots, both pointing rulenodes that chain up to mOldRuleTrees[0]...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Alright, so we post a rebuild all style data event from nsMathMLElement::BindToTree:

102           // Rebuild style data for the presshell, because style system
103           // optimizations may have taken place assuming MathML was disabled.
104           // (See nsRuleNode::CheckSpecifiedProperties.)
105           nsCOMPtr<nsIPresShell> shell = aDocument->GetShell();
106           if (shell) {
107             shell->GetPresContext()->PostRebuildAllStyleDataEvent(nsChangeHint(0));
108           }

which is why the mathml element is relevant.

Then we don't actually compute any new style contexts for anything, and end up with style contexts referencing the old ruletree...

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, so what happens here is that we go to reresolve style land in CaptureChange for the viewport, get a reframe change hint due to the document direction change, and then:

1021      if (NS_UpdateHint(aMinChange, ourChange)) {
1022        if (!(ourChange & nsChangeHint_ReconstructFrame) || aContent) {
1023          aChangeList->AppendChange(aFrame, aContent, ourChange);
1024        }
1025      }
1026      return aMinChange;

so we update aMinChange, do nothing else (hint is reconstruct, aContent is null for the viewport), return a reconstruct hint, assume that we don't need to do anything with any descendants of the viewport because the reframe will handle it, and bail out of ReResolveStyleContext.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Looks like the default value of mDirection is the one from the document.  And of course mDirection changes reframe...

I assume that mDirection on the viewport is in fact something we want to set based on the document direction?  If so, it sounds like we need to make it possible to reframe the viewport or something.

Comment 5

[(no longer active)]

I thought we should use NS_STYLE_HINT_FRAMECHANGE for direction change, but I tried it and it doesn't make any difference.  Is it not possible to reflow the viewport frame?!

Comment 6

[Boris Zbarsky [:bzbarsky]]

I don't follow comment 5....

Comment 7

[(no longer active)]

I made direction change return NS_STYLE_HINT_FRAMECHANGE which includes a reflow hint in addition to a reframe hint, and I expected that would reflow the viewport frame, but it doesn't, and I don't know why.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, because we assume that a reframe hint means that we don't need to do anything else, as an optimization, but we don't actually apply reframe hints on the root due to the code in comment 3.

So one option would be to change the code in comment 3 to handle the !aContent case by subtracting framechange from ourChange and then appending the resulting change.  Let me think about it for a bit.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I think we should do that.  The only !aContent case is the viewport, and this will only affect style changes on the viewport.

Comment 10

[Jesse Ruderman]

WFM on trunk