<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
Created attachment 88323 [details] Crashes
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
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?
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.
(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)
(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. :)
(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. :)
(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.
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;
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.
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.
*** Bug 57902 has been marked as a duplicate of this bug. ***
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.
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.
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.
Created attachment 95836 [details] Patch
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?
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.
(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.
(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.
Created attachment 95966 [details] Patch
Comment on attachment 95966 [details] Patch Clearing flags on attachment: 95966 Committed r88104: <http://trac.webkit.org/changeset/88104>
All reviewed patches have been landed. Closing bug.
(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.
(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.