Bug 155018 - Refactor RenderMathMLOperator and RenderMathMLToken to avoid using anonymous renderers
Summary: Refactor RenderMathMLOperator and RenderMathMLToken to avoid using anonymous ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 152244 153987 157519 157521 157568
Blocks: 104185 108778 125597 130245 132267 134021 139582 153991 155434 155435 159090 159091 159114 159205
  Show dependency treegraph
 
Reported: 2016-03-04 07:49 PST by Frédéric Wang (:fredw)
Modified: 2016-07-14 14:34 PDT (History)
10 users (show)

See Also:


Attachments
Patch (494.75 KB, patch)
2016-03-08 05:03 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (522.49 KB, patch)
2016-03-09 02:25 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (526.04 KB, patch)
2016-03-11 01:26 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (525.46 KB, patch)
2016-03-31 08:19 PDT, Frédéric Wang (:fredw)
alex: review-
Details | Formatted Diff | Diff
Patch (525.24 KB, patch)
2016-04-26 08:59 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Testing a small subset in EWS (824.53 KB, patch)
2016-05-11 08:05 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (358.13 KB, patch)
2016-05-11 10:03 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS testing (866.69 KB, patch)
2016-05-12 00:18 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.08 MB, application/zip)
2016-05-12 00:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (712.99 KB, application/zip)
2016-05-12 01:03 PDT, Build Bot
no flags Details
Patch for EWS testing (868.38 KB, patch)
2016-05-12 01:10 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (516.01 KB, patch)
2016-05-12 01:47 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (821.52 KB, application/zip)
2016-05-12 02:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.38 MB, application/zip)
2016-05-12 02:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (579.81 KB, application/zip)
2016-05-12 02:18 PDT, Build Bot
no flags Details
Patch (518.90 KB, patch)
2016-05-12 02:29 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (518.84 KB, patch)
2016-05-17 06:24 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (522.12 KB, patch)
2016-06-21 05:10 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (525.37 KB, patch)
2016-06-21 07:26 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (531.88 KB, patch)
2016-06-21 23:12 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (523.06 KB, patch)
2016-06-22 02:26 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (523.11 KB, patch)
2016-06-22 10:07 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (523.05 KB, patch)
2016-06-24 06:28 PDT, Frédéric Wang (:fredw)
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2016-03-04 07:49:59 PST
This is another commit from our MathMLLayout branch:

1) Thanks to bug 152244, we can use StretchyOperator for RenderMathMLOperator to avoid (re)creating anonymous text nodes. 
2) As suggested in bug 108778 comment 10.
Comment 1 Frédéric Wang (:fredw) 2016-03-08 05:03:20 PST
Created attachment 273290 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2016-03-09 02:25:06 PST
Created attachment 273418 [details]
Patch

A slightly better version where I implement layoutBlock and remove computeLogicalHeight so that bug 153991 will be easier. I also had forgotten to layout children when we do "special" painting ; fixing that changes some text expectations.
Comment 3 Frédéric Wang (:fredw) 2016-03-09 04:36:09 PST
Comment on attachment 273418 [details]
Patch

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

> Source/WebCore/css/mathml.css:43
>  

Note: it also makes sense to remove the following rule in this patch. It does not seem to have any effect (see bug 151592).

mtext {
    line-height: 1.0;
}
Comment 4 Frédéric Wang (:fredw) 2016-03-11 01:26:58 PST
Created attachment 273706 [details]
Patch

I'm uploading updated version of the MathML refactoring patches to solve conflicts after bug 154388 and bug 155005 ; and to do other minor changes.
Comment 5 Frédéric Wang (:fredw) 2016-03-31 08:19:58 PDT
Created attachment 275289 [details]
Patch
Comment 6 Joanmarie Diggs (irc: joanie) 2016-04-01 08:04:09 PDT
In terms of the accessibility-related changes (tests only), the one-pixel changes reported for size and position are fine. And it looks like we're now exposing the operator text in the same way we expose the other element text. Arguably, that's a bug fix. :) So a11y-wize, lgtm.
Comment 7 Alejandro G. Castro 2016-04-08 10:06:28 PDT
Comment on attachment 275289 [details]
Patch

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

The cleaning looks great, I'll let other reviewer to check it.

> LayoutTests/ChangeLog:10
> +        * platform/mac/TestExpectations: Disable some tests for now. Some just fail because of small rendering differences. The accessibility test for mfenced must be updated to take into account changes in the render tree.

Why don't we update the ones that fail because of small rendering differences?

> LayoutTests/platform/mac/TestExpectations:818
> +# FIXME: Small differences.
> +mathml/presentation/attributes-mathvariant.html [ ImageOnlyFailure ]
> +mathml/presentation/fenced-mi.html [ ImageOnlyFailure ]
> +mathml/presentation/fenced.html [ ImageOnlyFailure ]
> +mathml/presentation/tokenElements-mathvariant.html [ ImageOnlyFailure ]

Shall we update the images? Or you think we are adding a bug? In that case we should add a bug for it and add it here with the explanation.

> Source/WebCore/ChangeLog:11
> +        For anonymous RenderMathMLOperator renderers created by RenderMathMLFenced, we have to paint the character by ourselves ;

Nit, there is a space before the ;

> Source/WebCore/ChangeLog:29
> +        (WebCore::RenderMathMLOperator::specialPainting): New function to determine what will be painted using StretchyOperator.

specialPainting means stretched? Not sure if the name is very clear.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:279
> +        for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox())
> +            child->layoutIfNeeded();

What children an Operator can have when the anonymous are there, the text? Is it just one child?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:287
> +        // We first do the normal layout without spacing.
> +        recomputeLogicalWidth();
> +        setLogicalWidth(width() - m_leadingSpace - m_trailingSpace);
> +        RenderMathMLToken::layoutBlock(relayoutChildren, pageLogicalHeight);
> +        recomputeLogicalWidth();

This does not look correct, what are you trying to do calling recomputeLogicalWidth twice, it actually returns a bool to check if the width was actually recomputed. Also setting the logicalwidth and later calling the layoutBlock of the parent looks weird to me.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:64
> -    void updateTokenContent() final;
> +    void updateTokenContent() override;

Do you need to override in this patch?

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:159
> +    ASSERT(needsLayout());

Why we do not have this in the first line of the function?

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:165
> +    for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox())
> +        child->layoutIfNeeded();

Is this too general, in what situations a token could have more than one child?

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:175
> +    RenderMathMLBlock::paint(info, paintOffset);

Is this required in case isValid and we paint the glyph?
Comment 8 Frédéric Wang (:fredw) 2016-04-26 08:59:34 PDT
Created attachment 277376 [details]
Patch
Comment 9 Frédéric Wang (:fredw) 2016-05-11 08:05:33 PDT
Created attachment 278626 [details]
Testing a small subset in EWS
Comment 10 Frédéric Wang (:fredw) 2016-05-11 09:59:07 PDT
Comment on attachment 275289 [details]
Patch

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

>> LayoutTests/ChangeLog:10
>> +        * platform/mac/TestExpectations: Disable some tests for now. Some just fail because of small rendering differences. The accessibility test for mfenced must be updated to take into account changes in the render tree.
> 
> Why don't we update the ones that fail because of small rendering differences?

See below.

>> LayoutTests/platform/mac/TestExpectations:818
>> +mathml/presentation/tokenElements-mathvariant.html [ ImageOnlyFailure ]
> 
> Shall we update the images? Or you think we are adding a bug? In that case we should add a bug for it and add it here with the explanation.

These are reftests not pixel tests, so that's not easy to just update. The problem is the following: for some math text (like anonymous operator created by mfenced or italic mi and more generally mathvariant char) we will for use our own layout instead of the standard text layout because we need special painting. This works well on GTK but on other platforms you have small shifts. The problem is probably the mix between int and LayoutUnit (see bug 155879). I've been able to fixed the case of mfenced operators by correct int rounding of firstBaseLine. I'll see if I can do something for the non-mo token elements tomorrow.

>> Source/WebCore/ChangeLog:29
>> +        (WebCore::RenderMathMLOperator::specialPainting): New function to determine what will be painted using StretchyOperator.
> 
> specialPainting means stretched? Not sure if the name is very clear.

I renamed it to useMathOperator and moved that to bug 157519.

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:279
>> +            child->layoutIfNeeded();
> 
> What children an Operator can have when the anonymous are there, the text? Is it just one child?

I think only one anonymous block created by the CSS rules (maybe 0 too), but it does not hurt to have a general loop I guess.

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:287
>> +        recomputeLogicalWidth();
> 
> This does not look correct, what are you trying to do calling recomputeLogicalWidth twice, it actually returns a bool to check if the width was actually recomputed. Also setting the logicalwidth and later calling the layoutBlock of the parent looks weird to me.

I need to check that and see if I can make this less hacky.

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:64
>> +    void updateTokenContent() override;
> 
> Do you need to override in this patch?

I removed that. Probably I just mixed things up when trying to update all the MathML patches to move to the new override/final/virtual conventions.

>> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:159
>> +    ASSERT(needsLayout());
> 
> Why we do not have this in the first line of the function?

I moved that to the top for consistency with other functions. (This assert is probably in RenderMathMLBlock::layoutBlock too.)

>> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:165
>> +        child->layoutIfNeeded();
> 
> Is this too general, in what situations a token could have more than one child?

Same here, I think it's fine to have a loop even it there is probably only one anonymous block container.

>> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:175
>> +    RenderMathMLBlock::paint(info, paintOffset);
> 
> Is this required in case isValid and we paint the glyph?

I think RenderMathMLBlock has some compile flag to paint the MathML frame (not sure if that's really needed though).
Comment 11 Frédéric Wang (:fredw) 2016-05-11 10:03:14 PDT
Created attachment 278630 [details]
Patch

Here is an updated version. Some parts have been moved to bug 157519, bug 157521 and bug 157568, so the code change will be smaller here and hopefully easier to review. I still need to check again Alex's comments and the tests on iOS/Mac.
Comment 12 Frédéric Wang (:fredw) 2016-05-12 00:18:06 PDT
Created attachment 278706 [details]
Patch for EWS testing
Comment 13 Build Bot 2016-05-12 00:56:14 PDT
Comment on attachment 278706 [details]
Patch for EWS testing

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

New failing tests:
accessibility/math-text.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readwrite-pseudoclass-editable.html
fast/css/readonly-pseudoclass-common-element.html
accessibility/mac/mathml-elements.html
Comment 14 Build Bot 2016-05-12 00:56:18 PDT
Created attachment 278709 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 15 Build Bot 2016-05-12 01:03:21 PDT
Comment on attachment 278706 [details]
Patch for EWS testing

Attachment 278706 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1308134

New failing tests:
accessibility/ios-simulator/math.html
fast/css/readwrite-pseudoclass-editable.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readonly-pseudoclass-common-element.html
Comment 16 Build Bot 2016-05-12 01:03:25 PDT
Created attachment 278710 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 17 Frédéric Wang (:fredw) 2016-05-12 01:10:03 PDT
Created attachment 278711 [details]
Patch for EWS testing
Comment 18 Frédéric Wang (:fredw) 2016-05-12 01:47:16 PDT
Created attachment 278715 [details]
Patch
Comment 19 Build Bot 2016-05-12 02:01:31 PDT
Comment on attachment 278711 [details]
Patch for EWS testing

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

New failing tests:
fast/css/readwrite-pseudoclass-editable.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readonly-pseudoclass-common-element.html
Comment 20 Build Bot 2016-05-12 02:01:35 PDT
Created attachment 278717 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 21 Build Bot 2016-05-12 02:02:47 PDT
Comment on attachment 278711 [details]
Patch for EWS testing

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

New failing tests:
fast/css/readwrite-pseudoclass-editable.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readonly-pseudoclass-common-element.html
Comment 22 Build Bot 2016-05-12 02:02:51 PDT
Created attachment 278718 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 23 Build Bot 2016-05-12 02:18:45 PDT
Comment on attachment 278711 [details]
Patch for EWS testing

Attachment 278711 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1308418

New failing tests:
fast/css/readwrite-pseudoclass-editable.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readonly-pseudoclass-common-element.html
Comment 24 Build Bot 2016-05-12 02:18:49 PDT
Created attachment 278720 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 25 Frédéric Wang (:fredw) 2016-05-12 02:29:24 PDT
Created attachment 278721 [details]
Patch
Comment 26 Frédéric Wang (:fredw) 2016-05-17 05:49:11 PDT
Comment on attachment 278721 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:187
> +    IntPoint adjustedPaintOffset = roundedIntPoint(paintOffset + location() + LayoutPoint(0, glyphAscent));

So after more testing in bug 108778 on Mac/iOS, it seems that we should only round the vertical coordinate (glyphAscent) not the horizontal one or otherwise one mathvariant reftest does not work.
Comment 27 Frédéric Wang (:fredw) 2016-05-17 06:24:58 PDT
Created attachment 279116 [details]
Patch
Comment 28 Frédéric Wang (:fredw) 2016-06-21 05:10:47 PDT
Created attachment 281732 [details]
Patch
Comment 29 Frédéric Wang (:fredw) 2016-06-21 07:26:57 PDT
Created attachment 281743 [details]
Patch
Comment 30 Frédéric Wang (:fredw) 2016-06-21 23:12:38 PDT
Created attachment 281821 [details]
Patch

Here is an updated patch to run EWS, but I'll try and move some fenced accessibility code into a separate patch.
Comment 31 Frédéric Wang (:fredw) 2016-06-22 02:26:48 PDT
Created attachment 281828 [details]
Patch

This version applies on top of the patch for bug 139582.
Comment 32 Frédéric Wang (:fredw) 2016-06-22 10:07:01 PDT
Created attachment 281847 [details]
Patch

After discussion with joanie, it probably makes more sense to make the accessibility changes independent from this layout refactoring. So I'm uploading a new patch that goes back to the idea of temporarily disabling the fenced accessibility test on mac. The code to make that test work again and to fix a similar bug on ATK/Linux is essentially ready on bug 139582, but the way to organize the patches and review has to be sorted out with accessibility people.
Comment 33 Frédéric Wang (:fredw) 2016-06-24 06:28:12 PDT
Created attachment 281972 [details]
Patch
Comment 34 Martin Robinson 2016-06-24 07:15:06 PDT
Comment on attachment 281972 [details]
Patch

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

> LayoutTests/accessibility/mac/mathml-elements.html:114
> +   /*
> +   FIXME: AccessibilityRenderObject should be updated to properly expose the text of mfenced operators.
> +   See https://bugs.webkit.org/show_bug.cgi?id=139582.
>     var fenceValues = new Array("{", "2", ",", "a", ",", "e", "}");

Instead of commenting out this code you should remove it.

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:115
> +    // Early return if the token element contains RendererElement's.

Do you mean RenderElements here?
Comment 35 Frédéric Wang (:fredw) 2016-06-24 07:40:22 PDT
Committed r202420: <http://trac.webkit.org/changeset/202420>
Comment 36 Chris Dumez 2016-07-14 12:44:30 PDT
Comment on attachment 281972 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:167
> +    setLogicalWidth(m_mathVariantGlyph.font->widthForGlyph(m_mathVariantGlyph.glyph));

What guarantees m_mathVariantGlyph.font is non-null and alive? I suspect it could be because we see crashes like these in the wild:
Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Subtype: KERN_INVALID_ADDRESS at 0x000000000000074f
Triggered by Thread:  0

Filtered syslog:
None found
Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed ↩:
0   WebCore                       	0x0000000190d67970 WTF::HashTableAddResult<WTF::HashTableIterator<int, WTF::KeyValuePair<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > > >, WTF::IntHash<unsigned int>, WTF::HashMap<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> >, WTF::IntHash<unsigned int>, WTF::HashTraits<int>, WTF::HashTraits<std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > > >::KeyValuePairTraits, WTF::HashTraits<int> > > WTF::HashMap<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> >, WTF::IntHash<unsigned int>, WTF::HashTraits<int>, WTF::HashTraits<std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > > >::add<std::nullptr_t>(int&&, std::nullptr_t&&) + 28 (HashTable.h:869)
1   WebCore                       	0x0000000190970634 WebCore::GlyphMetricsMap<float>::locatePageSlowCase(unsigned int) + 176 (GlyphMetricsMap.h:120)
2   WebCore                       	0x0000000190970634 WebCore::GlyphMetricsMap<float>::locatePageSlowCase(unsigned int) + 176 (GlyphMetricsMap.h:120)
3   WebCore                       	0x00000001914cfb3c WebCore::RenderMathMLToken::layoutBlock(bool, WebCore::LayoutUnit) + 240 (GlyphMetricsMap.h:87)
4   WebCore                       	0x00000001908de104 WebCore::RenderBlock::layout() + 72 (RenderBlock.cpp:1075)
5   WebCore                       	0x00000001914cc320 WebCore::RenderMathMLRow::computeLineVerticalStretch(WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 152 (RenderElement.h:133)
6   WebCore                       	0x00000001914ccca0 WebCore::RenderMathMLRow::layoutBlock(bool, WebCore::LayoutUnit) + 64 (RenderMathMLRow.cpp:190)
7   WebCore                       	0x00000001908de104 WebCore::RenderBlock::layout() + 72 (RenderBlock.cpp:1075)
Comment 37 Frédéric Wang (:fredw) 2016-07-14 12:53:42 PDT
(In reply to comment #36)
> Comment on attachment 281972 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281972&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:167
> > +    setLogicalWidth(m_mathVariantGlyph.font->widthForGlyph(m_mathVariantGlyph.glyph));
> 
> What guarantees m_mathVariantGlyph.font is non-null and alive? I suspect it

There are isValid() calls everywhere in the MathML code. However, they do "glyph || font" rather than "glyph && font" as I expected...
Comment 38 Frédéric Wang (:fredw) 2016-07-14 13:03:58 PDT
I see in bug 150171 comment 5 that the initial intent was GlyphData::isEmpty(). For me GlyphData::isValid() means having a glyph id and a font.

@Myles, Carlos: is there any reason to call a GlyphData "valid"  without a font?
Comment 39 Frédéric Wang (:fredw) 2016-07-14 14:34:11 PDT
I opened bug 159783 and will upload a patch tomorrow.