Bug 107613 – Assert that computePreferredLogicalWidths never calls setNeedsLayout

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Assert that computePreferredLogicalWidths never calls setNeedsLayout

[Ojan Vafai]

Created attachment 184106 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 184106 [details]
Patch

Odd that this is a RenderBox method.  I had assumed the perferred widths where something from RenderObject.

[Eric Seidel (no email)]

Eventually I think the MathML will end up with a separate (opaque) rendering tree, similar to how SVG started.  If MathML truly has a different layout model than HTML, then it will just need to have its own rendering tree and not depend on RenderBox.

[Dave Barton]

(In reply to comment #3)
> Eventually I think the MathML will end up with a separate (opaque) rendering tree, similar to how SVG started.  If MathML truly has a different layout model than HTML, then it will just need to have its own rendering tree and not depend on RenderBox.

I of course disagree completely with this patch, and even more with this comment. I just want to register my opinion here, for anyone who might work on improving MathML's rendering in the future.

[Ojan Vafai]

(In reply to comment #2)
> (From update of attachment 184106 [details])
> Odd that this is a RenderBox method.  I had assumed the perferred widths where something from RenderObject.

Nope. It's on RenderBox. FWIW, RenderText also has a method by the same name that takes arguments (e.g. leadWidth).

(In reply to comment #4)
> (In reply to comment #3)
> > Eventually I think the MathML will end up with a separate (opaque) rendering tree, similar to how SVG started.  If MathML truly has a different layout model than HTML, then it will just need to have its own rendering tree and not depend on RenderBox.
> 
> I of course disagree completely with this patch, and even more with this comment. I just want to register my opinion here, for anyone who might work on improving MathML's rendering in the future.

I don't see what's controversial in this patch. Even if we decide we want to keep the current mathml behavior, we would still want this assert for the rest of the code. The only thing that would change is that we'd replace the FIXMEs with explanatory comments.

[WebKit Review Bot]

Comment on attachment 184106 [details]
Patch

Clearing flags on attachment: 184106

Committed r140554: <http://trac.webkit.org/changeset/140554>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dave Barton]

(In reply to comment #5)
> I don't see what's controversial in this patch. Even if we decide we want to keep the current mathml behavior, we would still want this assert for the rest of the code. The only thing that would change is that we'd replace the FIXMEs with explanatory comments.

I think I can state my position plainly. Standard mathematical convention (and common sense, frankly) say that large operators are wider than small ones. In general, an object's intrinsic width obviously can depend on its contents. For the simplest case, HTML declares block elements like <p> to be as wide as their parents, even if the paragraph is only a fraction of a line. This is counter-intuitive and surprises everyone at first, but is simple and workable. Shrink-to-fit elements like tables and inline-block need a more sophisticated measure of preferred widths, so one does calculations involving the preferred widths of sub-elements. This is more complicated but obviously can be made to work. In general, layout and preferred widths of complex objects can depend on the overall sizes, including heights, of sub-elements, not just their widths. This is more complicated, but can also be made to work. If there's really a problem in recursively calling layout to calculate these sizes, then separate routines will have to be written. However, it seems to me like this is exactly what LayoutStateDisabler is for, so I researched it and used it in MathML layout. You are not used to this complexity, so you'd like to outlaw it by fiat, and assert it never happens. My position is that if someone notices a bug it leads to, or gives some other concrete reason we shouldn't do it, then you are right. But I think you are now putting a higher priority on the possibility of bugs that haven't appeared (and may not exist), than on decent MathML layout in the future. I am a bit frustrated because I (and Alex M. before me) repeatedly asked for guidance on implementing MathML layout from rendering experts, and got very little. After not objecting at the time, you are now asserting that what I did is a bad thing, when I think it's a necessary small complication that has led to no problems.

The main issue is that Alex M. followed virtually all mathematical layout engines before him and stretched operators by stitching together partial glyphs. The easiest way to do this was by mutating the render tree during layout. This has led to a bad bug. We could try to fix that bug, but expert judgment (yours, Julien's, etc.) is that there would probably be other bugs if we continue to mutate the render tree during layout, so it's better to just scale glyphs for now. I accept that judgment. I don't yet accept the claim that it's bad to layout subexpressions during layout (e.g. during computePreferredLogicalWidths). I call this bottom-up layout, as opposed to top-down layout, but see no reason it has to be bad. It is different, but it's used in virtually all other mathematical layout systems, so obviously it can work. I'm just asking for a bug it causes, or a definite reason it's bad, other than "that's not what we do now so it violates an invariant" before we start asserting it can't happen. If there's an assert in the code, people will think there's a reason and it's necessary to comply. I am registering my opinion that this is false.

In summary, I think we both agree that you are saying the added complexity of having computePreferredLogicalWidths do what is needed for good MathML layout is not worth it. I think this is because you put very little value on good MathML layout, whereas I think hundreds of millions of users will need it (or switch). Also, I think you exaggerate the added complexity, because of a bug involving a different issue (mutating the render tree during layout). That was your original rationale for this change, until I pointed out that fixing it was independent of computePreferredLogicalWidths.

[David Carlisle]

(In reply to comment #8)

> I think I can state my position plainly.

I'd also like to note on the record that this seems a very unfortunate stance to take.

I'm not familiar with the webkit codebase but I have been involved with mathematical typesetting for 25 years and I was asked to review mathml related bugs as they were handled. It's been a pleasure to watch the webkit implementation mature over the last year and finally make it to a stable Chrome release.  To see this effort so severely compromised with apparently no weight at all given to the most basic norms of mathematical layout so shortly after it has been released is a real disappointment.

[WebKit Review Bot]

Re-opened since this is blocked by bug 107913

[Levi Weintraub]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > I think I can state my position plainly.
> 
> I'd also like to note on the record that this seems a very unfortunate stance to take.
> 
> I'm not familiar with the webkit codebase but I have been involved with mathematical typesetting for 25 years and I was asked to review mathml related bugs as they were handled. It's been a pleasure to watch the webkit implementation mature over the last year and finally make it to a stable Chrome release.  To see this effort so severely compromised with apparently no weight at all given to the most basic norms of mathematical layout so shortly after it has been released is a real disappointment.

Hi Dave and David,

Just to try and explain, one of our highest priorities as a rendering engine is security, and these patches are intended to fix a known source of security issues. That MathML requires a layout paradigm that HTML/CSS does not is unfortunate, as in its current implementation, it's built on top of WebKit's HTML/CSS rendering primitives. Our goal is not to make MathML rendering worse, but to make everyone safe.

The reason Chromium didn't have MathML turned on initially was that it had security issues. If we can't address them in a way we're comfortable with, we'll be left with the unfortunate option of turning it off again. I, personally, would very much rather that not happen.

I appreciate your interest and passion in getting our renderings to be right. I, too, want us to get there. Hopefully we can find a way to address all of our concerns.

[Dave Barton]

Levi, thanks for the comment. But believe me, David C. and I are intimately aware of Google's excellent attitude toward security, and its role in deciding whether to ship MathML in Chrome. See bug 107353 comment 44.

[David Carlisle]

(In reply to comment #11)

> Hi Dave and David,
> 
> Just to try and explain, one of our highest priorities as a rendering engine is security, and these patches are intended to fix a known source of security issues.

As I (and Dave Barton) have consistently stated we both recognise that security is a prime concern. I have in fact defended Chrome on several occasions in the last year as end users complained that the MathML that was in webkit base and in Safari was not turned on in Chrome. If code does not pass your security review you have every right not to include it.

That does not appear to be the case here. Dave Barton has repeatedly asked what bug or security failure this change is fixing and the answer appears to be that it is not fixing a bug. The only reasons given are that there is a code invariant that large characters should be the same width as small ones, and that the code would be simpler without the complication of dealing with character widths.

This is like saying that you have a code invariant that text should be set left right and that not having the Unicode bidi algorithm would make things simpler. It probably would be simpler but that doesn't mean it should be removed. You don't have to review the code or the result to see that just common sense dictates that characters get wider as they get bigger.
It's one thing to say that an implementation has not yet implemented mathematical layout, it is another to put in place asserts that say that mathematical layout is by design not possible.
 
> That MathML requires a layout paradigm that HTML/CSS does not is unfortunate, as in its current implementation, it's built on top of WebKit's HTML/CSS rendering primitives. Our goal is not to make MathML rendering worse, but to make everyone safe.

I can not comment on the code (that is not why I was asked to review every MathML component bug) but as I note above Dave Barton's repeated requests to be told what security failure this code triggers have gone un-answered.

> 
> The reason Chromium didn't have MathML turned on initially was that it had security issues. If we can't address them in a way we're comfortable with, we'll be left with the unfortunate option of turning it off again. I, personally, would very much rather that not happen.


Judging by discussion in this bug and in bug 107353 turning off won't be necessary. If you can't be convinced that the existing code is safe then the change suggested by Tony Chang in comment 35 would apparently  get to the same end result (at some possible run time costs). Or, if it comes to it, I assume I have no right of veto here so you can just overrule comments that the patch is wrong and make the change anyway. Any of those outcomes would be better than removing support completely.

> 
> I appreciate your interest and passion in getting our renderings to be right. I, too, want us to get there. Hopefully we can find a way to address all of our concerns.

I'm sure we all agree with the aims of having a secure browser that does the right thing (and also in fact that having a secure browser that does the wrong thing is better than having an insecure browser.)

[Ojan Vafai]

Trying again in bug 108539.

[Frédéric Wang (:fredw)]

I don't think the MathML code calls setNeedsLayout in computePreferredLogicalWidths anymore. However, I'm making this depends on bug 153991 since all the MathML layout functions will have been properly rewritten at that point.

[Frédéric Wang (:fredw)]

This was merged in r141517 and the hacks for MathML have been removed for a while. MathML's computePreferredLogicalWidths functions really only set m_minPreferredLogicalWidth and m_maxPreferredLogicalWidth and don't have any other side effect. So I'm closing this bug.