Bug 39389 - REGRESSION(57081): Renderer crash in WebCore::HTMLElement::isContentEditable() const
Summary: REGRESSION(57081): Renderer crash in WebCore::HTMLElement::isContentEditable(...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P1 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2010-05-19 16:40 PDT by Ojan Vafai
Modified: 2010-05-20 15:55 PDT (History)
6 users (show)

See Also:

test case (158 bytes, text/html)
2010-05-19 16:40 PDT, Ojan Vafai
no flags Details
Patch (4.17 KB, patch)
2010-05-19 18:39 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-05-19 16:40:25 PDT
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/).
Comment 1 Ojan Vafai 2010-05-19 16:43:15 PDT
The issue seems to be in HTMLElement::isContentEditable. renderer() points to something sane before document()->updateStyleIfNeeded() and to garbage after.
Comment 2 James Robinson 2010-05-19 16:46:56 PDT
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++
Comment 3 James Robinson 2010-05-19 17:04:11 PDT
+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.
Comment 4 James Robinson 2010-05-19 18:39:34 PDT
Created attachment 56546 [details]
Comment 5 James Robinson 2010-05-19 18:40:23 PDT
Proposed patch up for review.  Do you have any idea when this might have regressed, Ojan?
Comment 6 Ojan Vafai 2010-05-19 18:46:51 PDT
(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.
Comment 7 Simon Fraser (smfr) 2010-05-19 21:08:01 PDT
Comment on attachment 56546 [details]

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?
Comment 8 James Robinson 2010-05-19 21:17:51 PDT
Removing that call breaks some execCommand() tests. Hopefully that can be fixed easily as well but it'd be a different patch.
Comment 9 Eric Seidel (no email) 2010-05-20 00:33:27 PDT
Comment on attachment 56546 [details]

Comment 10 James Robinson 2010-05-20 12:06:31 PDT
Comment on attachment 56546 [details]

Gonna land by hand
Comment 11 James Robinson 2010-05-20 12:18:46 PDT
Comment on attachment 56546 [details]

Clearing flags on attachment: 56546

Committed r59856: <http://trac.webkit.org/changeset/59856>
Comment 12 James Robinson 2010-05-20 12:18:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 James Robinson 2010-05-20 13:22:56 PDT
I'm starting a bisect now to see when this regressed.
Comment 14 James Robinson 2010-05-20 15:50:41 PDT
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).
Comment 15 James Robinson 2010-05-20 15:51:11 PDT
Also note that r57081 fixed a bunch of other crashes :)
Comment 16 Enrica Casucci 2010-05-20 15:55:24 PDT