Bug 57897 – Crash in WebCore::RenderMathMLSubSup::baselinePosition()

• Component: MathML
• Status: RESOLVED FIXED

[Beth Dakin]

<rdar://problem/8908364>

Crashing test case attached. 

  1 com.apple.WebCore              0x7fff8c671c54 WebCore::RenderMathMLSubSup::baselinePosition(WebCore::FontBaseline, bool, WebCore::LineDirectionMode, WebCore::LinePositionMode) const + 0x8c
   2 com.apple.WebCore              0x7fff8c3c00cf WebCore::InlineFlowBox::computeLogicalBoxHeights(int&, int&, int&, int&, bool&, bool&, bool, WTF::HashMap<WebCore::InlineTextBox const*, std::pair<WTF::Vector<WebCore::SimpleFontData const*, 0ul>, WebCore::GlyphOverflow>, WTF::PtrHash<WebCore::InlineTextBox const*>, WTF::HashTraits<WebCore::InlineTextBox const*>, WTF::HashTraits<std::pair<WTF::Vector<WebCore::SimpleFontData const*, 0ul>, WebCore::GlyphOverflow> > >&, WebCore::FontBaseline, WebCore::VerticalPositionCache&) + 0x71
   3 com.apple.WebCore              0x7fff8c6a52f0 WebCore::RootInlineBox::alignBoxesInBlockDirection(int, WTF::HashMap<WebCore::InlineTextBox const*, std::pair<WTF::Vector<WebCore::SimpleFontData const*, 0ul>, WebCore::GlyphOverflow>, WTF::PtrHash<WebCore::InlineTextBox const*>, WTF::HashTraits<WebCore::InlineTextBox const*>, WTF::HashTraits<std::pair<WTF::Vector<WebCore::SimpleFontData const*, 0ul>, WebCore::GlyphOverflow> > >&, WebCore::VerticalPositionCache&) + 0xc0
   4 com.apple.WebCore              0x7fff8c64ef20 WebCore::RenderBlock::computeBlockDirectionPositionsForLine(WebCore::RootInlineBox*, WebCore::BidiRun*, WTF::HashMap<WebCore::InlineTextBox const*, std::pair<WTF::Vector<WebCore::SimpleFontData const*, 0ul>, WebCore::GlyphOverflow>, WTF::PtrHash<WebCore::InlineTextBox const*>, WTF::HashTraits<WebCore::InlineTextBox const*>, WTF::HashTraits<std::pair<WTF::Vector<WebCore::SimpleFontData const*, 0ul>, WebCore::GlyphOverflow> > >&, WebCore::VerticalPositionCache&) + 0x50
   5 com.apple.WebCore              0x7fff8be927de WebCore::RenderBlock::layoutInlineChildren(bool, int&, int&) + 0x1184
   6 com.apple.WebCore              0x7fff8c645eee WebCore::RenderBlock::layoutBlock(bool, int) + 0x4dc
   7 com.apple.WebCore              0x7fff8be8eeaa WebCore::RenderBlock::layout() + 0x28
   8 com.apple.WebCore              0x7fff8c67210e WebCore::RenderMathMLSubSup::layout() + 0xaa
   9 com.apple.WebCore              0x7fff8be91a7d WebCore::RenderBlock::layoutInlineChildren(bool, int&, int&) + 0x423
  10 com.apple.WebCore              0x7fff8c645eee WebCore::RenderBlock::layoutBlock(bool, int) + 0x4dc
  11 com.apple.WebCore              0x7fff8be8eeaa WebCore::RenderBlock::layout() + 0x28
  12 com.apple.WebCore              0x7fff8c671230 WebCore::RenderMathMLRow::layout() + 0x20
  13 com.apple.WebCore              0x7fff8be91a7d WebCore::RenderBlock::layoutInlineChildren(bool, int&, int&) + 0x423
  14 com.apple.WebCore              0x7fff8c645eee WebCore::RenderBlock::layoutBlock(bool, int) + 0x4dc
  15 com.apple.WebCore              0x7fff8be8eeaa WebCore::RenderBlock::layout() + 0x28
  16 com.apple.WebCore              0x7fff8be90c67 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, int&, int&) + 0x2db
  17 com.apple.WebCore              0x7fff8be9026b WebCore::RenderBlock::layoutBlockChildren(bool, int&) + 0x2b3
  18 com.apple.WebCore              0x7fff8c645f09 WebCore::RenderBlock::layoutBlock(bool, int) + 0x4f7
  19 com.apple.WebCore              0x7fff8be8eeaa WebCore::RenderBlock::layout() + 0x28
  20 com.apple.WebCore              0x7fff8be90c67 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, int&, int&) + 0x2db
  21 com.apple.WebCore              0x7fff8be9026b WebCore::RenderBlock::layoutBlockChildren(bool, int&) + 0x2b3
  22 com.apple.WebCore              0x7fff8c645f09 WebCore::RenderBlock::layoutBlock(bool, int) + 0x4f7
  23 com.apple.WebCore              0x7fff8be8eeaa WebCore::RenderBlock::layout() + 0x28
  24 com.apple.WebCore              0x7fff8be90c67 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, int&, int&) + 0x2db
  25 com.apple.WebCore              0x7fff8be9026b WebCore::RenderBlock::layoutBlockChildren(bool, int&) + 0x2b3
  26 com.apple.WebCore              0x7fff8c645f09 WebCore::RenderBlock::layoutBlock(bool, int) + 0x4f7
  27 com.apple.WebCore              0x7fff8be8eeaa WebCore::RenderBlock::layout() + 0x28
  28 com.apple.WebCore              0x7fff8be8edc5 WebCore::RenderView::layout() + 0x21f
  29 com.apple.WebCore              0x7fff8be8dfc8 WebCore::FrameView::layout(bool) + 0x6c6
  30 com.apple.WebCore              0x7fff8be846ac WebCore::Document::implicitClose() + 0x306
  31 com.apple.WebCore              0x7fff8be8424f WebCore::FrameLoader::checkCompleted() + 0x121
  32 com.apple.WebCore              0x7fff8be83fca WebCore::FrameLoader::finishedParsing() + 0x56
  33 com.apple.WebCore              0x7fff8be81ff7 WebCore::Document::finishedParsing() + 0x10b
  34 com.apple.WebCore              0x7fff8c371795 WebCore::HTMLDocumentParser::prepareToStopParsing() + 0xa1
  35 com.apple.WebCore              0x7fff8be464c1 WebCore::DocumentWriter::endIfNotLoadingMainResource() + 0x6b
  36 com.apple.WebCore              0x7fff8bebac82 WebCore::FrameLoader::finishedLoading() + 0x48
  37 com.apple.WebCore              0x7fff8c60053d WebCore::MainResourceLoader::didFinishLoading(double) + 0x6f
  38 com.apple.Foundation           0x7fff9651a0e6 ___NSURLConnectionDidFinishLoading_block_invoke_1 + 0x7a
  39 com.apple.Foundation           0x7fff9643ce7d _NSURLConnectionDidFinishLoading + 0x51
  40 com.apple.CFNetwork            0x7fff928f8748 URLConnectionClient::_clientDidFinishLoading(URLConnectionClient::ClientConnectionEventQueue*) + 0x148
  41 com.apple.CFNetwork            0x7fff929acc37 URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) + 0x171
  42 com.apple.CFNetwork            0x7fff929ace44 URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) + 0x37e
  43 com.apple.CFNetwork            0x7fff928e936b URLConnectionClient::processEvents() + 0xc1
  44 com.apple.CFNetwork            0x7fff928e9230 MultiplexerSource::perform() + 0xd4
  45 com.apple.CoreFoundation       0x10ca32bdc __CFRunLoopDoSources0 + 0x1bc
  46 com.apple.CoreFoundation       0x10ca324e9 __CFRunLoopRun + 0x389
  47 com.apple.CoreFoundation       0x10ca31f26 CFRunLoopRunSpecific + 0xe6
  48 com.apple.HIToolbox            0x7fff9032b067 RunCurrentEventLoopInMode + 0x115
  49 com.apple.HIToolbox            0x7fff9032adb3 ReceiveNextEventCommon + 0xb5
  50 com.apple.HIToolbox            0x7fff9032acee BlockUntilNextEventMatchingListInMode + 0x3e
  51 com.apple.AppKit               0x7fff8e9fa3e5 _DPSNextEvent + 0x293
  52 com.apple.AppKit               0x7fff8e9f9cea -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 0x87
  53 com.apple.Safari.framework     0x7fff8d65e5a4 -[BrowserApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 0xab
  54 com.apple.AppKit               0x7fff8e9bebad -[NSApplication run] + 0x1c8
  55 com.apple.AppKit               0x7fff8e9b7988 NSApplicationMain + 0x35c
  56 com.apple.Safari.framework     0x7fff8d7bf8ea SafariMain + 0xc5
  57 com.apple.Safari               0x10c9def24 start + 0x0

[Beth Dakin]

Created attachment 88323 [details]
Crashes

[Kulanthaivel Palanichamy]

Created attachment 91738 [details]
Proposed patch

This patch attempts to fix this crash by making container RenderMathMLBlock objects as anonymous render objects and removing them from render tree when all of their children are removed.

This patch also fixes the following bugs
https://bugs.webkit.org/show_bug.cgi?id=57900
https://bugs.webkit.org/show_bug.cgi?id=57901
https://bugs.webkit.org/show_bug.cgi?id=57902

[Eric Seidel (no email)]

Comment on attachment 91738 [details]
Proposed patch

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

I don't feel like I have enough information to judge the correctness of this change.  More background/explaination would be helpful first.

> Source/WebCore/ChangeLog:11
> +        RenderMathMLBlock objects which are created as container objects
> +        are not removed from the render tree even after all of its
> +        children are removed due to the deletion of their corresponding
> +        DOM node.

Why aren't they removed?

> Source/WebCore/ChangeLog:15
> +        This patch creates all the container RenderMathMLBlock objects
> +        as anonymous render objects and makes sure that they are removed
> +        from the render tree when all of their children renderers are removed.

Anonymous renderers generally mean you have no associated DOM node, and are used most often for when you need many renderers to a single DOM node to hold synthetic style, or to box inline children (when you have other box children), etc. Why should RenderMathMLBlocks be anonymous?

> Source/WebCore/ChangeLog:19
> +        In connection to the changes for the issue mentioned above, I have
> +        added null check in few places to avoid potential crashes while
> +        accessing grandchild renderer objects.

Can we test those?

[Eric Seidel (no email)]

Comment on attachment 91738 [details]
Proposed patch

I need more information in the bug before I am convinced this is the right approach.  I don't doubt that it is, so much as I feel lacking in information.  Marking r- for the time being, feel free to mark r? again after answering the above questions.

[Kulanthaivel Palanichamy]

(In reply to comment #3)
> (From update of attachment 91738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91738&action=review
> 
> I don't feel like I have enough information to judge the correctness of this change.  More background/explaination would be helpful first.
> 
> > Source/WebCore/ChangeLog:11
> > +        RenderMathMLBlock objects which are created as container objects
> > +        are not removed from the render tree even after all of its
> > +        children are removed due to the deletion of their corresponding
> > +        DOM node.
> 
> Why aren't they removed?

In the existing implementation, whenever a new child render object is added to the MathML renderers (RenderMathMLSubSup, RenderMathMLUnderOver, RenderMathMLFraction, etc...) a RenderMathMLBlock is created as container block for the new child irrespective of the type of child render object. In many places, these children objects are accessed by firstChild()->firstChild() assuming that the container block should have at least one child, but when a child node of above said elements are removed by JS, then only the renderers corresponding to those elements are removed and not their containers.

> 
> > Source/WebCore/ChangeLog:15
> > +        This patch creates all the container RenderMathMLBlock objects
> > +        as anonymous render objects and makes sure that they are removed
> > +        from the render tree when all of their children renderers are removed.
> 
> Anonymous renderers generally mean you have no associated DOM node, and are used most often for when you need many renderers to a single DOM node to hold synthetic style, or to box inline children (when you have other box children), etc. Why should RenderMathMLBlocks be anonymous?
>
These RenderMathMLBlocks are created exactly for the same purpose and they shouldn't be associated with any DOM node. Currently they are associated with parent renderer's DOM node which is wrong I guess.

> > Source/WebCore/ChangeLog:19
> > +        In connection to the changes for the issue mentioned above, I have
> > +        added null check in few places to avoid potential crashes while
> > +        accessing grandchild renderer objects.
> 
> Can we test those?
Well, the newly added test cases are meant for testing those changes initially. But after introducing the anonymous block change, the container MathML blocks are always getting removed. So, they are no longer covered by the test. But I kept them to handle the any empty MathML container blocks (just in-case)

[Eric Seidel (no email)]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 91738 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=91738&action=review
> > 
> > I don't feel like I have enough information to judge the correctness of this change.  More background/explaination would be helpful first.
> > 
> > > Source/WebCore/ChangeLog:11
> > > +        RenderMathMLBlock objects which are created as container objects
> > > +        are not removed from the render tree even after all of its
> > > +        children are removed due to the deletion of their corresponding
> > > +        DOM node.
> > 
> > Why aren't they removed?
> 
> In the existing implementation, whenever a new child render object is added to the MathML renderers (RenderMathMLSubSup, RenderMathMLUnderOver, RenderMathMLFraction, etc...) a RenderMathMLBlock is created as container block for the new child irrespective of the type of child render object. In many places, these children objects are accessed by firstChild()->firstChild() assuming that the container block should have at least one child, but when a child node of above said elements are removed by JS, then only the renderers corresponding to those elements are removed and not their containers.

OK.  These sound like anonymous blocks then.  So there exists no such thing as a <mathml:block> element (or whatever DOM element would correspond to these things?)

> > 
> > > Source/WebCore/ChangeLog:15
> > > +        This patch creates all the container RenderMathMLBlock objects
> > > +        as anonymous render objects and makes sure that they are removed
> > > +        from the render tree when all of their children renderers are removed.
> > 
> > Anonymous renderers generally mean you have no associated DOM node, and are used most often for when you need many renderers to a single DOM node to hold synthetic style, or to box inline children (when you have other box children), etc. Why should RenderMathMLBlocks be anonymous?
> >
> These RenderMathMLBlocks are created exactly for the same purpose and they shouldn't be associated with any DOM node. Currently they are associated with parent renderer's DOM node which is wrong I guess.

Yes, when you create a synthetic rendering object, it's supposed to be anonymous.  There are very few cases where more than one renderer will point to the same DOM node, I don't believe this should be one. :)

[Kulanthaivel Palanichamy]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (From update of attachment 91738 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=91738&action=review
> > > 
> > > I don't feel like I have enough information to judge the correctness of this change.  More background/explaination would be helpful first.
> > > 
> > > > Source/WebCore/ChangeLog:11
> > > > +        RenderMathMLBlock objects which are created as container objects
> > > > +        are not removed from the render tree even after all of its
> > > > +        children are removed due to the deletion of their corresponding
> > > > +        DOM node.
> > > 
> > > Why aren't they removed?
> > 
> > In the existing implementation, whenever a new child render object is added to the MathML renderers (RenderMathMLSubSup, RenderMathMLUnderOver, RenderMathMLFraction, etc...) a RenderMathMLBlock is created as container block for the new child irrespective of the type of child render object. In many places, these children objects are accessed by firstChild()->firstChild() assuming that the container block should have at least one child, but when a child node of above said elements are removed by JS, then only the renderers corresponding to those elements are removed and not their containers.
> 
> OK.  These sound like anonymous blocks then.  So there exists no such thing as a <mathml:block> element (or whatever DOM element would correspond to these things?)
>
No, there is no such element exists in MathML, but some of these container
blocks can be eliminated based on the type of child object it is going to hold and this change requires lot of modifications allover MathML rendering implementation. May be, I'll try it in a separate patch.

> > > 
> > > > Source/WebCore/ChangeLog:15
> > > > +        This patch creates all the container RenderMathMLBlock objects
> > > > +        as anonymous render objects and makes sure that they are removed
> > > > +        from the render tree when all of their children renderers are removed.
> > > 
> > > Anonymous renderers generally mean you have no associated DOM node, and are used most often for when you need many renderers to a single DOM node to hold synthetic style, or to box inline children (when you have other box children), etc. Why should RenderMathMLBlocks be anonymous?
> > >
> > These RenderMathMLBlocks are created exactly for the same purpose and they shouldn't be associated with any DOM node. Currently they are associated with parent renderer's DOM node which is wrong I guess.
> 
> Yes, when you create a synthetic rendering object, it's supposed to be anonymous.  There are very few cases where more than one renderer will point to the same DOM node, I don't believe this should be one. :)

[Alex Milowski]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #3)
> > > > (From update of attachment 91738 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=91738&action=review
> > > > 
> > > > I don't feel like I have enough information to judge the correctness of this change.  More background/explaination would be helpful first.
> > > > 
> > > > > Source/WebCore/ChangeLog:11
> > > > > +        RenderMathMLBlock objects which are created as container objects
> > > > > +        are not removed from the render tree even after all of its
> > > > > +        children are removed due to the deletion of their corresponding
> > > > > +        DOM node.
> > > > 
> > > > Why aren't they removed?
> > > 
> > > In the existing implementation, whenever a new child render object is added to the MathML renderers (RenderMathMLSubSup, RenderMathMLUnderOver, RenderMathMLFraction, etc...) a RenderMathMLBlock is created as container block for the new child irrespective of the type of child render object. In many places, these children objects are accessed by firstChild()->firstChild() assuming that the container block should have at least one child, but when a child node of above said elements are removed by JS, then only the renderers corresponding to those elements are removed and not their containers.
> > 
> > OK.  These sound like anonymous blocks then.  So there exists no such thing as a <mathml:block> element (or whatever DOM element would correspond to these things?)
> >
> No, there is no such element exists in MathML, but some of these container
> blocks can be eliminated based on the type of child object it is going to hold and this change requires lot of modifications allover MathML rendering implementation. May be, I'll try it in a separate patch.

Many of these "anonymous blocks" exist to layout or stack elements, with their own style properties, that might otherwise have an inline flow.  For example, a fraction is a vertical stack of RenderMathMLBlock instances who contain the child who is either the numerator or denominator.

That is <mfrac><mn>1</mn><mi>x</mi></mfrac> uses RenderMathMLBlock instances to wrap the inline rendering objects for mn and mi--which come directly from the CSS and not from any customized code.  This allows things like:

<mfrac><input xmlns='http://www.w3.org/1999/xhtml' size='2'/><mn>3</mn></mfrac>

to work as well.

I'll look at this patch in more detail.  The anonymous blocks caused a great deal of other problems and that is why I use the node.  There is some funky problems I was never able to get a straight answer about that are made worse by anonymous rendering objects.  So, that may be historical or it may still be there.  I'll check that too.

[Alex Milowski]

To fix this crash, you just need:


Index: RenderMathMLSubSup.cpp
===================================================================
--- RenderMathMLSubSup.cpp	(revision 84804)
+++ RenderMathMLSubSup.cpp	(working copy)
@@ -109,7 +109,7 @@
     if (!base)
         return;
     
-    if (base->firstChild()->isRenderMathMLBlock()) {
+    if (base->firstChild() && base->firstChild()->isRenderMathMLBlock()) {
         RenderMathMLBlock* block = toRenderMathMLBlock(base->firstChild());
         block->stretchToHeight(static_cast<int>(gSubSupStretch * height));
         
@@ -185,7 +185,7 @@
     switch (m_kind) {
     case SubSup:
         base = base->firstChild();
-        if (m_scripts && base->isBoxModelObject()) {
+        if (m_scripts && base && base->isBoxModelObject()) {
             RenderBoxModelObject* box = toRenderBoxModelObject(base);
             
             int topAdjust = (m_scripts->offsetHeight() - box->offsetHeight()) / 2;

[Alex Milowski]

I would take this fix slightly farther.  The block wrappers from the super/subscript or base handling needs to be removed in the dom child is removed.  The rendering will be wrong if it is manipulated via the DOM API.  The addChild() method needs to be much more intelligent.

I'd rather file a separate bug for that and fix the crash.

[Alex Milowski]

Let me add that this rendering object handles msub (base + subscript), msup (base + superscript), msubsup (base + subscript + superscript).  I now think that is a mistake.  msubsup is a very different beast.

As such, I have this on a list to completely re-write.  I'll have to address scripting requirements at the same time.

[Alex Milowski]

*** Bug 57902 has been marked as a duplicate of this bug. ***

[Alex Milowski]

Created attachment 92666 [details]
A fix for the crash

This patch is focus only on fixing the crash and keeping the structure of the layout of msubsup intact.

[WebKit Review Bot]

Attachment 92666 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/math..." exit_code: 1

LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Milowski]

Created attachment 92668 [details]
Updated patch with property changelog entry.

Grrrr.  It would help if I actually put a change log entry for the tests.

[Vicki Pfau]

Created attachment 95836 [details]
Patch

[Beth Dakin]

Comment on attachment 92668 [details]
Updated patch with property changelog entry.

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

I think the bug fix here is compelling, but I think we should just fix the crash with this bug. Since Jeffrey posted a patch just to fix the crash, I think I will r+ that, and we should file a new bug for the rendering issue you are working on here.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:69
> +            position++;

Our style-guide dictates that there should be braces around this for-loop since it contains more than one line of code. Also, is it necessary to compare the nodeType() to Node::ELEMENT_NODE? Can you just ask current->isElementNode()?

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:73
> +        RenderMathMLBlock* wrapper = new (renderArena()) RenderMathMLBlock(node());

How do you know this is always position 1? Is there a way to break this?

[Beth Dakin]

Comment on attachment 95836 [details]
Patch

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

Commit-queue minus-ing since the test file contains tabs. But upload a new version and I will CQ+.

> LayoutTests/mathml/msubsup-remove-children.xhtml:5
> +		<script>

Looks like this file contains tabs instead of spaces. You should replace the tabs with spaces.

[Alex Milowski]

(In reply to comment #17)
> (From update of attachment 92668 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92668&action=review
> 
> I think the bug fix here is compelling, but I think we should just fix the crash with this bug. Since Jeffrey posted a patch just to fix the crash, I think I will r+ that, and we should file a new bug for the rendering issue you are working on here.

That will end up with the wrong rendering after the script runs.  The additional tests show that the correct result will happen for msup/msub/msubsup when a child is removed and added again.

I would really prefer this is fixed properly and not just patched to remove the crash.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:69
> > +            position++;
> 
> Our style-guide dictates that there should be braces around this for-loop since it contains more than one line of code. Also, is it necessary to compare the nodeType() to Node::ELEMENT_NODE? Can you just ask current->isElementNode()?

Yet, it passed all the style checks.  I can fix that.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:73
> > +        RenderMathMLBlock* wrapper = new (renderArena()) RenderMathMLBlock(node());
> 
> How do you know this is always position 1? Is there a way to break this?

Because position 1 is the first element child and, for these MathML elements, the first element child is always the base of the msup/msub/msubsup.

[Beth Dakin]

(In reply to comment #19)
> (In reply to comment #17)
> > (From update of attachment 92668 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=92668&action=review
> > 
> > I think the bug fix here is compelling, but I think we should just fix the crash with this bug. Since Jeffrey posted a patch just to fix the crash, I think I will r+ that, and we should file a new bug for the rendering issue you are working on here.
> 
> That will end up with the wrong rendering after the script runs.  The additional tests show that the correct result will happen for msup/msub/msubsup when a child is removed and added again.
> 
> I would really prefer this is fixed properly and not just patched to remove the crash.
> 

I really think it's better to have one bug-fix per patch. And I consider these to be two bugs.

> > 
> > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:69
> > > +            position++;
> > 
> > Our style-guide dictates that there should be braces around this for-loop since it contains more than one line of code. Also, is it necessary to compare the nodeType() to Node::ELEMENT_NODE? Can you just ask current->isElementNode()?
> 
> Yet, it passed all the style checks.  I can fix that.
> 

Yeah, the style bot should be fixed here. Check out item 3 in the "Braces" section: http://www.webkit.org/coding/coding-style.html

> > 
> > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:73
> > > +        RenderMathMLBlock* wrapper = new (renderArena()) RenderMathMLBlock(node());
> > 
> > How do you know this is always position 1? Is there a way to break this?
> 
> Because position 1 is the first element child and, for these MathML elements, the first element child is always the base of the msup/msub/msubsup.

That makes sense.

I really think you should file a new bug for the layout problem and post your layout patch there. Ping me when you do, and I will be happy to review.

[Vicki Pfau]

Created attachment 95966 [details]
Patch

[WebKit Review Bot]

Comment on attachment 95966 [details]
Patch

Clearing flags on attachment: 95966

Committed r88104: <http://trac.webkit.org/changeset/88104>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Milowski]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > (From update of attachment 92668 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=92668&action=review
> > > 
> > > I think the bug fix here is compelling, but I think we should just fix the crash with this bug. Since Jeffrey posted a patch just to fix the crash, I think I will r+ that, and we should file a new bug for the rendering issue you are working on here.
> > 
> > That will end up with the wrong rendering after the script runs.  The additional tests show that the correct result will happen for msup/msub/msubsup when a child is removed and added again.
> > 
> > I would really prefer this is fixed properly and not just patched to remove the crash.
> > 
> 
> I really think it's better to have one bug-fix per patch. And I consider these to be two bugs.
> 
> > > 
> > > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:69
> > > > +            position++;
> > > 
> > > Our style-guide dictates that there should be braces around this for-loop since it contains more than one line of code. Also, is it necessary to compare the nodeType() to Node::ELEMENT_NODE? Can you just ask current->isElementNode()?
> > 
> > Yet, it passed all the style checks.  I can fix that.
> > 
> 
> Yeah, the style bot should be fixed here. Check out item 3 in the "Braces" section: http://www.webkit.org/coding/coding-style.html
> 
> > > 
> > > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:73
> > > > +        RenderMathMLBlock* wrapper = new (renderArena()) RenderMathMLBlock(node());
> > > 
> > > How do you know this is always position 1? Is there a way to break this?
> > 
> > Because position 1 is the first element child and, for these MathML elements, the first element child is always the base of the msup/msub/msubsup.
> 
> That makes sense.
> 
> I really think you should file a new bug for the layout problem and post your layout patch there. Ping me when you do, and I will be happy to review.

It isn't a new layout problem.  It is the correct fix for the incorrect assumptions made by the original code that led to the crash.

I guess I will have to do so.

[Beth Dakin]

(In reply to comment #24)
> 
> It isn't a new layout problem.  It is the correct fix for the incorrect assumptions made by the original code that led to the crash.
> 
> I guess I will have to do so.

I described it as "new" since previously this test case crashed. So any layout that it actually achieves when not crashing is indeed new.