Bug 110686 – [CSS Regions] Crash when MathML used in CSS Regions

• Component: CSS
• Status: RESOLVED FIXED

[A George]

Created attachment 189913 [details]
Test Page - Regions MathML

Browser crashes when MathML is included in CSS Regions.
Attached is the test page.

Tested on Chrome Version 24.0.1312.57 m by enabling CSS Regions through Webkit Experimental Flags

[Mihnea Ovidenie]

Taking a look.

[Andrei Bucur]

Created attachment 190959 [details]
Patch

[Mihai Maerean]

Comment on attachment 190959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190959&action=review

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:80
> +    RenderView* renderView = view();

You could move this new code in the constructor of a local class called FragmentationDisabler and also move the sibling code below in the destructor of the same local class.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:95
>      setNeedsLayout(true, MarkOnlyThis);

Previously, setNeedsLayout was called before pushLayoutState, not after.

[Dave Barton]

Ojan posted this excerpt from chat in a comment on his blog, which I assume relates to this bug:

8:58 AM <dhyatt> minPreferredLogicalWidth() is not supposed to depend on layout(), otherwise you can get into cycles
8:58 AM <abucur> ok. when we compute the preferredwidths for RenderMathMLRow
8:58 AM <dhyatt> which is what you are running into
8:58 AM <abucur> well...
8:58 AM <abucur> void RenderMathMLRow::computePreferredLogicalWidths()
8:58 AM <abucur> {
8:58 AM <abucur>     ASSERT(preferredLogicalWidthsDirty() && needsLayout());
8:58 AM <abucur> #ifndef NDEBUG
8:58 AM <abucur>     // FIXME: Remove this once mathml stops modifying the render tree here.
8:58 AM <abucur>     SetLayoutNeededForbiddenScope layoutForbiddenScope(this, false);
8:58 AM <abucur> #endif
8:58 AM <abucur>     computeChildrenPreferredLogicalHeights();
8:58 AM <dhyatt> .....
8:58 AM <abucur> the last line is doing a children layout
8:58 AM <dhyatt> this should never have made it past review.

I wrote RenderMathMLRow::computePreferredLogicalWidths() and computeChildrenPreferredLogicalHeights(). They haven't gotten into cycles up to now because they only call layout recursively on descendents (after temporarily setting a very large container width, and later caching the results). This was done because MathML preferred widths depend on preferred heights of certain descendents, not just their widths, e.g. because of tall parentheses and brackets. (This also seems plausible to me for general layout. When you ask something complicated for its preferred width, it ought to be able to look at its children's preferred sizes - though not final layout sizes - and visual features in order to answer. I could imagine Adobe or someone proposing a new CSS layout feature someday that would need this, for instance. Certainly MathML would like it, if possible.) See bug 107353 for more discussion. In bug 107353 comment 35 Tony proposed a change to avoid the descendent layout calls, though Ojan has since expressed concern that even it would be too complicated to maintain.

Could someone say more about why my approach should never have made it past review, i.e. why it must cause circularity? I don't believe it has caused any bugs up to now. I do understand that it's different than HTML layout code so far, hence unfamiliar, but not why it can't work.

I do think this issue is difficult. I would be happy to talk about it in a video chat (e.g. Google Hangout), unless there's a simple compelling bug or reason why the best approach is clear, that can be summarized in a bug page comment. For instance, is Tony's approach either necessary or sufficient?

Again, I'm not saying I'm right. I'm just saying that I wish someone would talk to me about the issue. I did ask for design advice before implementing this code, and got very little.

P.S. Andrei's patch was submitted as I was typing this. I am grateful that he didn't remove my code, but if my code is causing problems, I would still like to know if it should be removed or modified. Thanks and sorry for the inconvenience.

[Andrei Bucur]

Comment on attachment 190959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190959&action=review

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:80
>> +    RenderView* renderView = view();
> 
> You could move this new code in the constructor of a local class called FragmentationDisabler and also move the sibling code below in the destructor of the same local class.

Good suggestion.

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:95
>>      setNeedsLayout(true, MarkOnlyThis);
> 
> Previously, setNeedsLayout was called before pushLayoutState, not after.

They shouldn't be related. I think this structure is easier to read.

[Dave Barton]

P.S. I guess by "someone", I mean dhyatt. I know Ojan thinks my approach is complicated or confusing, but I'm not sure why dhyatt stated he thinks it shouldn't have made it past review, or what he'd recommend instead.

I do like the idea of this patch, and the comment to improve it.

[Andrei Bucur]

(In reply to comment #6)
> P.S. I guess by "someone", I mean dhyatt. I know Ojan thinks my approach is complicated or confusing, but I'm not sure why dhyatt stated he thinks it shouldn't have made it past review, or what he'd recommend instead.
> 
> I do like the idea of this patch, and the comment to improve it.

I think we shouldn't emit conclusions from some IRC logs. Best thing is to catch David and talk a bit about why he thinks this is wrong. It could as well be a misunderstanding; maybe I've explained something wrong during the discussion.

The idea of the patch still stands in my opinion. Even without regions, you don't want to layout the content and be concerned about the content being fragmented. It's possible you get the wrong results for the preferred height (lines can be shifted to accomodate for page breaks).

[Dave Barton]

(In reply to comment #7)
> (In reply to comment #6)
> > P.S. I guess by "someone", I mean dhyatt. I know Ojan thinks my approach is complicated or confusing, but I'm not sure why dhyatt stated he thinks it shouldn't have made it past review, or what he'd recommend instead.
> > 
> > I do like the idea of this patch, and the comment to improve it.
> 
> I think we shouldn't emit conclusions from some IRC logs. Best thing is to catch David and talk a bit about why he thinks this is wrong. It could as well be a misunderstanding; maybe I've explained something wrong during the discussion.
> 
> The idea of the patch still stands in my opinion. Even without regions, you don't want to layout the content and be concerned about the content being fragmented. It's possible you get the wrong results for the preferred height (lines can be shifted to accomodate for page breaks).

Thanks, I agree with all this. In particular, any MathML idea of preferred height doesn't want to include page breaks, IMO.

[Dave Hyatt]

Comment on attachment 190959 [details]
Patch

Broken by my change to turn the bit into a state, so you'll need to re-post.

[Andrei Bucur]

Created attachment 191210 [details]
Patch

[Dave Hyatt]

Comment on attachment 191210 [details]
Patch

r=me

[Dave Hyatt]

(In reply to comment #6)
> P.S. I guess by "someone", I mean dhyatt. I know Ojan thinks my approach is complicated or confusing, but I'm not sure why dhyatt stated he thinks it shouldn't have made it past review, or what he'd recommend instead.
> 
> I do like the idea of this patch, and the comment to improve it.

Layout is supposed to depend on preferred logical widths, but the reverse cannot be true or you risk getting into cycles. Essentially it should be possible to run the preferred logical widths algorithm independently without ever invoking layout.

[Elliott Sprehn]

Comment on attachment 191210 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191210&action=review

> Source/WebCore/rendering/RenderView.cpp:1150
> +FragmentationDisabler::FragmentationDisabler(RenderObject* root)

It would be nice if this was all be hidden behind the MATHML feature ifdef since not all ports ship mathml. :)

[Dave Barton]

(In reply to comment #12)
> Layout is supposed to depend on preferred logical widths, but the reverse cannot be true or you risk getting into cycles. Essentially it should be possible to run the preferred logical widths algorithm independently without ever invoking layout.

Thanks very much for the reply. Isn't "cannot" too strong a word here? Because MathML preferred logical widths only depend on the preferred logical heights of *descendents*, circularity is avoided, as I explained in comment 4. It's just a bottom-up (non-circular) algorithm inside RenderMathMLRow::computePreferredLogicalWidths() and computeChildrenPreferredLogicalHeights().

[Andrei Bucur]

(In reply to comment #13)
> (From update of attachment 191210 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191210&action=review
> 
> > Source/WebCore/rendering/RenderView.cpp:1150
> > +FragmentationDisabler::FragmentationDisabler(RenderObject* root)
> 
> It would be nice if this was all be hidden behind the MATHML feature ifdef since not all ports ship mathml. :)

I can move this to RenderMathMLBlock. It was added to RenderView because I thought it may be needed in other circumstances.
I suppose we could just move it out when it will be the case.

[Dave Hyatt]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 191210 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191210&action=review
> > 
> > > Source/WebCore/rendering/RenderView.cpp:1150
> > > +FragmentationDisabler::FragmentationDisabler(RenderObject* root)
> > 
> > It would be nice if this was all be hidden behind the MATHML feature ifdef since not all ports ship mathml. :)
> 
> I can move this to RenderMathMLBlock. It was added to RenderView because I thought it may be needed in other circumstances.
> I suppose we could just move it out when it will be the case.

I actually would like it to stay in RenderView. I suspect we will have use for it beyond MathML.

[Dave Hyatt]

(In reply to comment #14)
> (In reply to comment #12)
> > Layout is supposed to depend on preferred logical widths, but the reverse cannot be true or you risk getting into cycles. Essentially it should be possible to run the preferred logical widths algorithm independently without ever invoking layout.
> 
> Thanks very much for the reply. Isn't "cannot" too strong a word here? Because MathML preferred logical widths only depend on the preferred logical heights of *descendents*, circularity is avoided, as I explained in comment 4. It's just a bottom-up (non-circular) algorithm inside RenderMathMLRow::computePreferredLogicalWidths() and computeChildrenPreferredLogicalHeights().

Well, in general it would be nice for the one to not depend on the other, since originally it was even done as a separate pass before layout. Is it really the case that the layout of these descendants would never ask for the width of the container? 

If that width is the very intrinsic width you're computing, then the answer you give to the children will be wrong. I don't think it will cause a cycle, because we will simply assume the correct width got stored in the renderer, but since it didn't, you'll just use the wrong value.

That's my main point, that there is circularity. You may be getting lucky and not experiencing the circularity because of how the code happens to be set up, in which case you avoid the cycle by rendering wrong.

[Dave Barton]

(In reply to comment #17)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > Layout is supposed to depend on preferred logical widths, but the reverse cannot be true or you risk getting into cycles. Essentially it should be possible to run the preferred logical widths algorithm independently without ever invoking layout.
> > 
> > Thanks very much for the reply. Isn't "cannot" too strong a word here? Because MathML preferred logical widths only depend on the preferred logical heights of *descendents*, circularity is avoided, as I explained in comment 4. It's just a bottom-up (non-circular) algorithm inside RenderMathMLRow::computePreferredLogicalWidths() and computeChildrenPreferredLogicalHeights().
> 
> Well, in general it would be nice for the one to not depend on the other, since originally it was even done as a separate pass before layout. Is it really the case that the layout of these descendants would never ask for the width of the container? 

The final *layout* of the descendents might depend on the container width, for line wrapping, as in the HTML case. However, the *preferred* width and height of the descendents is defined to *not* depend on wrapping, just like the HTML preferred (logical) maximum width.

> If that width is the very intrinsic width you're computing, then the answer you give to the children will be wrong. I don't think it will cause a cycle, because we will simply assume the correct width got stored in the renderer, but since it didn't, you'll just use the wrong value.
> 
> That's my main point, that there is circularity. You may be getting lucky and not experiencing the circularity because of how the code happens to be set up, in which case you avoid the cycle by rendering wrong.

I think the answer isn't "wrong", it's just preferred vs. final height, like for width.

The preferred heights of descendents are used to stretch adjacent parentheses and other brackets, e.g. around matrices. The height of a stretched parenthesis affects its width, even its preferred width, so it needs to know the preferred heights of its (unstretched) siblings. See <http://www.w3.org/TR/MathML3/chapter3.html#presm.op.stretch>, e.g. <http://www.w3.org/TR/MathML3/chapter3.html#id.3.2.5.8.2>. In other words, I agree you're right that you can't compute the final layout height of renderers at preferred logical widths time, because line wrapping could conceivably occur later. But stretching to match unwrapped heights is almost always good enough, and I would argue what you really want anyway. So preferred logical heights are good enough, and final layout heights are unnecessary, at preferred logical width time.

I find this totally analogous to layout logical widths vs. preferred logical widths, and the use cases for the latter.

[WebKit Review Bot]

Comment on attachment 191210 [details]
Patch

Clearing flags on attachment: 191210

Committed r144744: <http://trac.webkit.org/changeset/144744>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dave Barton]

Created attachment 192385 [details]
example of preferred height & operator stretching, with line breaking

This attached image example is from Neil Soiffer of the w3c MathML committee, to show what should happen with preferred heights, operator stretching, and line breaking. The committee discussed this in their monthly conference call on Thursday, and will add a clarification to the spec. Basically they agree with what I said in comment 18 for the common case of an <mrow>. Stretching in <mtable>s however is typically used for arrows in a commutative diagram, which would ideally be handled differently, though this is rarer and something WebKit probably won't get to for a while. They also said this is all a clarification of what's already in the MathML spec sections I mentioned, not a change.