REGRESSION(57081): Renderer crash in WebCore::HTMLElement::isContentEditable() const

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ojan Vafai]

Created attachment 56530 [details]
test case

This is a very frequent crash see on youtube and YUI's gridbuilder (hit the "show code" button on http://developer.yahoo.com/yui/grids/builder/).

[Ojan Vafai]

The issue seems to be in HTMLElement::isContentEditable. renderer() points to something sane before document()->updateStyleIfNeeded() and to garbage after.

[James Robinson]

Stack (from a ToT WebKit debug build on Snow Leopard):


Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xfffffffe0000600d
0x0000000101a40e20 in WTF::RefPtr<WebCore::RenderStyle>::get (this=0xfffffffe0000600d) at RefPtr.h:60
60	        T* get() const { return m_ptr; }
(gdb) bt
#0  0x0000000101a40e20 in WTF::RefPtr<WebCore::RenderStyle>::get (this=0xfffffffe0000600d) at RefPtr.h:60
#1  0x0000000101051c7d in WebCore::RenderObject::style (this=0xfffffffe00006005) at RenderObject.h:598
#2  0x000000010136071e in WebCore::HTMLElement::isContentEditable (this=0x11723a960) at /usr/local/home/jamesr/WebKit/WebCore/html/HTMLElement.cpp:634
#3  0x00000001017283d4 in WebCore::Node::isContentEditable (this=0x117214520) at /usr/local/home/jamesr/WebKit/WebCore/dom/Node.cpp:643
#4  0x0000000101727ba1 in WebCore::Node::rootEditableElement (this=0x117214520) at /usr/local/home/jamesr/WebKit/WebCore/dom/Node.cpp:1520
#5  0x0000000101359423 in WebCore::editableRootForPosition (p=@0x7fff5fbfea80) at /usr/local/home/jamesr/WebKit/WebCore/editing/htmlediting.cpp:214
#6  0x0000000101a8aee5 in WebCore::VisibleSelection::rootEditableElement (this=0x10701dfd0) at /usr/local/home/jamesr/WebKit/WebCore/editing/VisibleSelection.cpp:569
#7  0x000000010127fd73 in WebCore::SelectionController::rootEditableElement (this=0x10701dfc0) at SelectionController.h:51
#8  0x00000001012c3b20 in WebCore::Frame::notifyRendererOfSelectionChange (this=0x10701da00, userTriggered=true) at /usr/local/home/jamesr/WebKit/WebCore/page/Frame.cpp:535
#9  0x0000000101278c90 in WebCore::EventHandler::handleMouseReleaseEvent (this=0x10701e140, event=@0x7fff5fbfec70) at /usr/local/home/jamesr/WebKit/WebCore/page/EventHandler.cpp:703
#10 0x00000001012790c7 in WebCore::EventHandler::handleMouseReleaseEvent (this=0x10701e140, mouseEvent=@0x7fff5fbfed60) at /usr/local/home/jamesr/WebKit/WebCore/page/EventHandler.cpp:1566
#11 0x00000001012813f5 in WebCore::EventHandler::mouseUp (this=0x10701e140, event=0x1172a1fd0) at /usr/local/home/jamesr/WebKit/WebCore/page/mac/EventHandlerMac.mm:530
#12 0x00000001003857e8 in -[WebHTMLView mouseUp:] (self=0x1172080b0, _cmd=0x7fff8743c914, event=0x1172a1fd0) at /usr/local/home/jamesr/WebKit/WebKit/mac/WebView/WebHTMLView.mm:3713
#13 0x00007fff86e36fc9 in -[NSWindow sendEvent:] ()
#14 0x0000000100048de3 in ?? ()
#15 0x00007fff86d6c676 in -[NSApplication sendEvent:] ()
#16 0x00000001000318dc in ?? ()
#17 0x00007fff86d030be in -[NSApplication run] ()
#18 0x00007fff86cfbd8c in NSApplicationMain ()
#19 0x00000001000016f4 in ?? ()
Current language:  auto; currently c++

[James Robinson]

+cc some folks with knowledge of this area.

It seems that the basic problem here is that the editing code is calling deep into a subtree that gets detached by the Document::updateStyleIfNeeded() call.  I think the fix is to move the updateStyle call further down the callstack, especially as some bits down there (like htmlediting.cpp's editableRootForPosition() queries information might be affected by style.

[James Robinson]

Created attachment 56546 [details]
Patch

[James Robinson]

Proposed patch up for review.  Do you have any idea when this might have regressed, Ojan?

[Ojan Vafai]

(In reply to comment #5)
> Proposed patch up for review.  Do you have any idea when this might have regressed, Ojan?

No idea. Haven't tried looking at nightlies.

[Simon Fraser (smfr)]

Comment on attachment 56546 [details]
Patch

Does this allow you to remove the call to updateRendering () in isContentEditable()? Can you add assertions to ensure that updateRendering() doesn't happen during layout?

[James Robinson]

Removing that call breaks some execCommand() tests. Hopefully that can be fixed easily as well but it'd be a different patch.

[Eric Seidel (no email)]

Comment on attachment 56546 [details]
Patch

OK.

[James Robinson]

Comment on attachment 56546 [details]
Patch

Gonna land by hand

[James Robinson]

Comment on attachment 56546 [details]
Patch

Clearing flags on attachment: 56546

Committed r59856: <http://trac.webkit.org/changeset/59856>

[James Robinson]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

I'm starting a bisect now to see when this regressed.

[James Robinson]

Surprise surprise, this particular test case was broken by http://trac.webkit.org/changeset/57081 since that patch eliminated a style recalc that would have papered over this issue.  However I think the editing code was broken before r57081 anyway since it poked into the render tree without making any effort to see if styles were up to date.  It is likely that there is a way to get this code to crash before r57081 (although it might be tricky).

[James Robinson]

Also note that r57081 fixed a bunch of other crashes :)

[Enrica Casucci]

<rdar://problem/8010983>