Bug 44815 – crash: 0x101dad3ae in WebCore::RenderBox::positionForPoint at RenderBox.cpp:2817

• Status: NEW
• Component: Layout and Rendering

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Eric Seidel (no email)]

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?

[Ryosuke Niwa]

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.

[Darin Adler]

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.

[W. James MacLean]

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.

[Eric Seidel (no email)]

(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.

[Ryosuke Niwa]

(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.

[Eric Seidel (no email)]

Created attachment 66083 [details]
crash report

[Eric Seidel (no email)]

Comment on attachment 66083 [details]
crash report

Actually, that's from Safari 5.  I guess it crashes differently on trunk.

[Eric Seidel (no email)]

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.

[W. James MacLean]

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).

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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 ""

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Ha!  Now looking at the reduction, this has nothing to do with SVG.  I'll make a better reduction shortly.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Yup.  This can be triggered in plain old HTML too:

cat test.html:
<style>body { display: table-column-group; }</style>

Triggers the bug.

[Eric Seidel (no email)]

This bug is a bunch of bugs. :)  I'm reminded that firstDeepEditingPositionForNode is deprecated code anyway.  So whatever is calling it shouldn't be...