Created attachment 65812 [details] crash mode (click somewhere after you open) Reproduction steps: 1. Open the attached file 2. Click anywhere http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderBox.cpp?rev=66247#L2823 return createVisiblePosition(firstDeepEditingPositionForNode(node())); On this line, node() is 0 because m_anonymous is true. http://crbug.com/48691.
Can RenderBox be anonymous? If that's the case, the calling node() there seems to be wrong. If RenderBox cannot be anonymous, we might have a bigger problem here.
RenderBlocks are often anonymous. <div><span></span><div></div></div> the span gets wrapped in an anonymous block. RenderBlock is a subclass of RenderBox. Does that answer your question?
Thanks for the info, Eric. (In reply to comment #2) > RenderBlocks are often anonymous. <div><span></span><div></div></div> > the span gets wrapped in an anonymous block. RenderBlock is a subclass of RenderBox. > > Does that answer your question? Yes, in that case, we shouldn't be calling node() in this function without first checking that RenderBox is not anonymous. It seems like there are many such dungeons calls to node() right now. +darin & adele since http://trac.webkit.org/changeset/41928 is the most recent changeset which touched this code. The problem seems to date back to http://trac.webkit.org/changeset/40871, which replaced call to element() by node(). The changeset claims that caller of node() handles the cases where it doesn't expect node() to be 0. It wasn't a problem here because it just instantiates VisiblePosition and VisiblePosition can be null. But http://trac.webkit.org/changeset/41863 did: - return VisiblePosition(node(), 0, DOWNSTREAM); + return firstDeepEditingPositionForNode(node()); And some later changeset added an assertion that node isn't 0 in firstDeepEditingPositionForNode. We can either check nullness of node() in RenderBox, call element() or some other function that always returns non-zero value in RenderBox, or allow firstDeepEditingPositionForNode to handle node being 0. Any thoughts or opinions on this? +justin for 41863, and +weinig for 40871.
Yes, r40871 eliminated the old "safe" node() function that was guaranteed to always return a node. It would return the document for anonymous renderers. So any call site that assumes node() can never be 0 but can process an anonymous renderer will now have to handle the anonymous case somehow. Previously, such code probably did crazy things, because it's unlikely the caller of node() expected to get the document node and it's hihgly likely it would do something undesirable in such cases. But note that the old element() function had exactly the same behavior, so when you point out that r40871 replaced a call to element() with a call to node(), that means you're on the wrong track. The new node() function does exactly the same thing that the old element() function did. There is no element() function any more. Checking for 0 or checking if the node is anonymous is probably the right thing, but then do what? We need to answer the higher level question of what we want to do. Skip forward or backward to the first non-anonymous content perhaps? At some level of the code.
Is it possible to have something like inline Position firstDeepEditingPositionForNode(Node* anchorNode) { if (anchorNode) return Position(anchorNode, 0); else return Position(); } I've tried this and it runs on the example in question, but I don't know what else it might cause problems with.
(In reply to comment #5) > Is it possible to have something like > > inline Position firstDeepEditingPositionForNode(Node* anchorNode) > { > if (anchorNode) > return Position(anchorNode, 0); > else > return Position(); > } > > I've tried this and it runs on the example in question, but I don't know what else it might cause problems with. That's wrong. If callers want this, they should check for !node themselves.
(In reply to comment #5) > Is it possible to have something like > > inline Position firstDeepEditingPositionForNode(Node* anchorNode) > { > if (anchorNode) > return Position(anchorNode, 0); > else > return Position(); > } I don't think handling null anchorNode fits into the semantics of this particular function. Furthermore, as I commented, the crash is caused by our calling node() in anonymous node. And anonymous node isn't a detached node that can be treated as null. It usually contains or is a descendent of some DOM node, which can be used to call firstDeepEditingPositionForNode. > I've tried this and it runs on the example in question, but I don't know what else it might cause problems with. I'm sure this will prevent the crash but I'm afraid that isn't the right fix here.
Created attachment 66083 [details] crash report
Comment on attachment 66083 [details] crash report Actually, that's from Safari 5. I guess it crashes differently on trunk.
I think the reduction could stil be smaller. Looks like there is a bunch of crazy comments and cdata stuff we probably don't need.
Created attachment 66089 [details] Reduced SVG input image This is a (slightly) reduced version of the problematic input. It removes the CDATA and comments, but still contains the unbalanced </altGlyph> tag (which seems to be necessary for the bug to occur ... on Linux at least).
Here is the stack: #0 0x10145259e in WebCore::firstDeepEditingPositionForNode at htmlediting.h:128 #1 0x101d6c61e in WebCore::RenderBox::positionForPoint at RenderBox.cpp:2823 #2 0x101d33142 in WebCore::RenderBlock::positionForPoint at RenderBlock.cpp:4033 #3 0x101dde750 in WebCore::RenderObject::positionForCoordinates at RenderObject.cpp:2213 #4 0x101d6c5fa in WebCore::RenderBox::positionForPoint at RenderBox.cpp:2821 #5 0x101d333d3 in WebCore::RenderBlock::positionForPoint at RenderBlock.cpp:4064 #6 0x10179642c in WebCore::EventHandler::handleMousePressEventSingleClick at EventHandler.cpp:367 #7 0x101796ca4 in WebCore::EventHandler::handleMousePressEvent at EventHandler.cpp:473 #8 0x10179762d in WebCore::EventHandler::handleMousePressEvent at EventHandler.cpp:1350 #9 0x10179d0cc in WebCore::EventHandler::mouseDown at EventHandlerMac.mm:494 #10 0x100f2ae0c in -[WebHTMLView mouseDown:] at WebHTMLView.mm:3578
It appears the anonymous block is wrapping the body? That seems very odd. (closestRenderer is the anonymous block which we're trying to hit test and is giving us trouble. To figure out what node it's wrapping I looked at its first child, which looks to be the body?) p closestRenderer->firstChild()->showTreeForThis() *body 0x151450dc0 parsererror 0x15143db30 STYLE=display: block; white-space: pre; border: 2px solid #c77; padding: 0 1em 0 1em; margin: 1em; background-color: #fdd; color: black h3 0x15143c030 #text 0x15143e230 "This page contains the following errors:" div 0x15143bb80 STYLE=font-family:monospace;font-size:12px #text 0x1066f6760 "error on line 9 at column 12: Opening and ending tag mismatch: svg line 0 and altGlyph\n" h3 0x15143ac10 #text 0x1066f2140 "Below is a rendering of the page up to the first error." svg 0x1066c4360 #text 0x15143f590 "\n" style 0x1066f8140 #text 0x15143efc0 "\nbody{\n display: table-column-group;\n}\n" #text 0x106697b80 "\n" body 0x15143d6e0 #text 0x1066ebe80 "\n" #text 0x15143bd40 ""
Here is what DRT has to say about the rendering tree: layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x0 RenderBlock {html} at (0,0) size 800x0 RenderTable at (0,0) size 0x0 RenderTableCol {body} at (0,0) size 0x0 That doesn't seem to match what we see in the DOM dump from the debugger.
Ha! Now looking at the reduction, this has nothing to do with SVG. I'll make a better reduction shortly.
OK, it does have to do with SVG, sorta. Mostly because SVG doesn't end up with any HTML content until the XML parse error code runs. Full test case: <svg xmlns="http://www.w3.org/2000/svg"> <style>body { display: table-column-group; }</style> We end up trying to attach a body which is styled as a table-column-group, which ends up creating an anonymous RenderTable to wrap the body.
There are probably multiple bugs here. I suspect we could trigger this in other ways. It might be possible to tigger this in plain old HTML as well. One bug is probably found in the XML error handling path for SVG: http://trac.webkit.org/browser/trunk/WebCore/dom/XMLDocumentParser.cpp#L281
Yup. This can be triggered in plain old HTML too: cat test.html: <style>body { display: table-column-group; }</style> Triggers the bug.
This bug is a bunch of bugs. :) I'm reminded that firstDeepEditingPositionForNode is deprecated code anyway. So whatever is calling it shouldn't be...