Bug 162824 – [MathML] Render tree should be all clean by the end of FrameView::layout().

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[zalan]

We fail to clean all the renderers while running the following tests:
 
LayoutTests/mathml/arbitrary-markup.html	
LayoutTests/mathml/invalid-scripts-crash.html	
LayoutTests/mathml/mn-as-list-item-assert.html	
LayoutTests/mathml/msubsup-fuzz.html	
LayoutTests/mathml/msubsup-no-grandchild.xhtml	
LayoutTests/mathml/roots-addChild.html	
LayoutTests/mathml/roots-removeChild.html	
LayoutTests/mathml/scripts-addChild.html	
LayoutTests/mathml/scripts-removeChild.html	
LayoutTests/mathml/wbr-in-mroot-crash.html

[zalan]

LayoutTests/accessibility/mac/mathml-multiscript.html

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

I see that there are several cases in bug 162834. Can you elaborate on what is meant by "clean the renderer" here? Is it a new concept recently introduced?

[zalan]

(In reply to comment #2)
> I see that there are several cases in bug 162834. Can you elaborate on what
> is meant by "clean the renderer" here? Is it a new concept recently
> introduced?
It's the good old needsLayout flag. See https://trac.webkit.org/changeset/206743

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

(In reply to comment #3)
> It's the good old needsLayout flag. See
> https://trac.webkit.org/changeset/206743

Thanks. So from a quick look into this, the following pattern can be found in several places:

https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp#L182

i.e. when the markup is invalid, set the needs layout flag to false and do an early return. I guess we should however still call layoutIfNeeded on children. The failing tests mentioned in comment 0 seem to produce some invalid markup, so that's consistent with this idea.

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

Created attachment 292389 [details]
Patch

@Brent: Can you please give a try to this patch? (I don't have a debug build to check the tests right now)

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

Comment on attachment 292389 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:252
> +{

Maybe we want to add a reference to https://bugs.webkit.org/show_bug.cgi?id=123348 here.

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

Created attachment 294020 [details]
Patch

[Darin Adler]

Comment on attachment 294020 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:251
> +void RenderMathMLBlock::layoutInvalidMarkup()

This continues our unfortunate tradition of treating the noun "layout" as a verb. But, hey, until we rename layoutBlock, layoutIfNeeded and all the many other similarly named functions, I suppose I shouldn’t complain about adding one more.

I still am not entirely sure, though, about the name. We are saying here that we need to "lay out invalid markup", but I think that’s not quite right. I think I might name this something more like layOutBlockContainingInvalidContent. Or maybe this could have a short name, but I would like to understand what the best short name would be.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:257
> +    for (auto child = firstChildBox(); child; child = child->nextSiblingBox())
> +        child->layoutIfNeeded();

If this is going to render as an empty box, as the comment above claims, then what are these children? Should we be destroying and removing them instead of calling layoutIfNeeded?

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:236
> +    setPreferredLogicalWidthsDirty(false);

One way to accomplish this without calling setPreferredLogicalWidthsDirty(false) twice would be to add another level of function nesting. The outer function could assert, call the function that does the work, and then set the flag to false.

Another way would be to clear the dirty flag at the start of this function. It’s not clear to me if there is a requirement or benefit to clearing it only at the end.

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

(In reply to comment #8)
> This continues our unfortunate tradition of treating the noun "layout" as a
> verb. But, hey, until we rename layoutBlock, layoutIfNeeded and all the many
> other similarly named functions, I suppose I shouldn’t complain about adding
> one more.
> 
> I still am not entirely sure, though, about the name. We are saying here
> that we need to "lay out invalid markup", but I think that’s not quite
> right. I think I might name this something more like
> layOutBlockContainingInvalidContent. Or maybe this could have a short name,
> but I would like to understand what the best short name would be.

For example

<mfrac><mtext>numerator</mtext></mfrac>

is invalid markup because the fraction misses a denominator but <mtext>numerator</mtext> by itself is valid. So the whole renderer (fraction) is invalid, not its content.

Maybe layOutBlockForInvalidContent?

> > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:257
> > +    for (auto child = firstChildBox(); child; child = child->nextSiblingBox())
> > +        child->layoutIfNeeded();
> 
> If this is going to render as an empty box, as the comment above claims,
> then what are these children? Should we be destroying and removing them
> instead of calling layoutIfNeeded?

Again, see above. We can have an <mfrac> element with an arbitrary number of (valid) MathML subtrees as children but the <mfrac> element itself is valid iff it has exactly two children.

So I don't think we should destroy or remove them. At the moment, we basically just make the render tree match the DOM tree, and the renderer can become valid or invalid when children are added or removed (either during normal construction/destruction or when the user modifies the DOM tree).

[Simon Fraser (smfr)]

(In reply to comment #9)

> <mfrac><mtext>numerator</mtext></mfrac>
> 
> is invalid markup because the fraction misses a denominator but
> <mtext>numerator</mtext> by itself is valid. So the whole renderer
> (fraction) is invalid, not its content.

Why don't we fix this kind of thing up at parsing time?

For HTML we create anonymous boxes if necessary, but we don't ever lay out "invalid" markup.

Simon

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

(In reply to comment #10)
> Why don't we fix this kind of thing up at parsing time?
> 
> For HTML we create anonymous boxes if necessary, but we don't ever lay out
> "invalid" markup.

Sure, we can certainly do that and actually that would probably be the proper way to fix bug 123348 if we ever want to address it. It's not clear to me whether it's a good idea to modify the render tree when passing between valid/invalid states, though. Anyway, I believe this is out of the scope of the present bug for which we just want to call layoutIfNeeded on the whole render tree up, without modifying it.

[Build Bot]

Comment on attachment 294020 [details]
Patch

Attachment 294020 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2468921

New failing tests:
accessibility/mac/mathml-multiscript.html

[Build Bot]

Created attachment 294077 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 294020 [details]
Patch

Attachment 294020 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2469071

New failing tests:
accessibility/mac/mathml-multiscript.html

[Build Bot]

Created attachment 294082 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 294020 [details]
Patch

Attachment 294020 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2469303

New failing tests:
accessibility/mac/mathml-multiscript.html

[Build Bot]

Created attachment 294083 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

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

Created attachment 294142 [details]
Patch

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

Created attachment 294358 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 294358 [details]
Patch

Rejecting attachment 294358 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 294358, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2503181

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

Created attachment 294612 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 294612 [details]
Patch

Clearing flags on attachment: 294612

Committed r208648: <http://trac.webkit.org/changeset/208648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.