[Webkit-unassigned] [Bug 57897] Crash in WebCore::RenderMathMLSubSup::baselinePosition()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=57897





--- Comment #24 from Alex Milowski <alex at milowski.com>  2011-06-04 08:50:56 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.