Bug 65362

Summary:

Search field in designMode causes a crash

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

HTML Editing

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Major

CC:

adele, ap, cshu, darin, dglazkov, emerick, enrica, hyatt, leviw, noel.gordon, rniwa, simon.fraser, tonikitoo

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

data:text/html,<html> <body> <input type="search"> <script>document.designMode = 'on';</script> </body> </html>

Attachments:

Description	Flags
fixes the bug	none
fixes the bug with much less hack	dglazkov: review+

Kent Tamura

Reported 2011-07-29 00:40:19 PDT

http://code.google.com/p/chromium/issues/detail?id=90306 1. Open the URL 2. Focus the search field 3. Type the Del key (Backspace in Windows) 4. Crash! This is reproducible with Safari 5.0.5 - Today's WebKit nightly. In the nightly, this is a null pointer dereference.

Attachments
fixes the bug (4.98 KB, patch) 2011-08-01 12:33 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
fixes the bug with much less hack (5.49 KB, patch) 2011-08-01 13:40 PDT, Ryosuke Niwa	dglazkov: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

noel gordon

Comment 1 2011-07-29 01:21:40 PDT

Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000 0x00000001022f7f1c in WebCore::RenderTextControl::computeLogicalHeight (this=0x12b3b23a8) at WebKit/Source/WebCore/rendering/RenderTextControl.cpp:288 288 innerTextRenderBox->marginTop() + innerTextRenderBox->marginBottom()); (gdb) backtrace #0 WebCore::RenderTextControl::computeLogicalHeight (this=0x12b3b23a8) at WebKit/Source/WebCore/rendering/RenderTextControl.cpp:288 #1 WebCore::RenderTextControlSingleLine::layout (this=0x12b3b23a8) at WebKit/Source/WebCore/rendering/RenderTextControlSingleLine.cpp:223 #2 WebCore::FrameView::layout (this=0x12b3ab190, allowSubtree=true) at WebKit/Source/WebCore/page/FrameView.cpp:1016 #3 WebCore::Document::updateLayout (this=0x12c07ae00) at WebKit/Source/WebCore/dom/Document.cpp:1620 #4 WebCore::Document::updateLayoutIgnorePendingStylesheets (this=0x12c07ae00) at WebKit/Source/WebCore/dom/Document.cpp:1651 #5 WebCore::EditCommand::updateLayout (this=0x12b21b490) at WebKit/Source/WebCore/editing/EditCommand.cpp:208 #6 WebCore::DeleteSelectionCommand::fixupWhitespace (this=0x12b21b490) at WebKit/Source/WebCore/editing/DeleteSelectionCommand.cpp:558 #7 WebCore::DeleteSelectionCommand::doApply (this=0x12b21b490) at WebKit/Source/WebCore/editing/DeleteSelectionCommand.cpp:832 #8 WebCore::EditCommand::apply (this=0x12b21b490) at WebKit/Source/WebCore/editing/EditCommand.cpp:92 #9 WebCore::CompositeEditCommand::applyCommandToComposite (this=0x12b21b010, cmd=@0x7fff5fbfe0e0) at WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:102 #10 WebCore::CompositeEditCommand::deleteSelection (this=0x12b21b010, selection=@0x7fff5fbfe2e0, smartDelete=false, mergeBlocksAfterDelete=true, replace=false, expandForSpecialElements=true) at WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:409 #11 WebCore::TypingCommand::deleteKeyPressed (this=0x12b21b010, granularity=WebCore::CharacterGranularity, killRing=false) at WebKit/Source/WebCore/editing/TypingCommand.cpp:548 #12 WebCore::TypingCommand::doApply (this=0x12b21b010) at WebKit/Source/WebCore/editing/TypingCommand.cpp:292 #13 WebCore::EditCommand::apply (this=0x12b21b010) at WebKit/Source/WebCore/editing/EditCommand.cpp:92 #14 WebCore::TypingCommand::deleteKeyPressed (document=0x12c07ae00, options=0, granularity=WebCore::CharacterGranularity) at WebKit/Source/WebCore/editing/TypingCommand.cpp:117 #15 WebCore::Editor::deleteWithDirection (this=0x109015a38, direction=WebCore::DirectionBackward, granularity=WebCore::CharacterGranularity, killRing=false, isTypingAction=true) at WebKit/Source/WebCore/editing/Editor.cpp:315 #16 WebCore::executeDeleteBackward (frame=0x109015400) at WebKit/Source/WebCore/editing/EditorCommand.cpp:330 #17 WebCore::Editor::Command::execute (this=0x7fff5fbfe700, parameter=@0x7fff5fbfe690, triggeringEvent=0x12b2195c0) at WebKit/Source/WebCore/editing/EditorCommand.cpp:1648 #18 WebCore::Editor::Command::execute (this=0x7fff5fbfe700, triggeringEvent=0x12b2195c0) at WebKit/Source/WebCore/editing/EditorCommand.cpp:1653 #19 -[WebHTMLView(WebNSTextInputSupport) doCommandBySelector:] (self=0x12b3a7470, _cmd=0x7fff8295951c, selector=0x7fff8298f4cf) at WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:5805 #20 -[WebHTMLView(WebInternal) _executeSavedKeypressCommands] (self=0x12b3a7470, _cmd=0x101218398) at WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:5274 #21 -[WebHTMLView(WebInternal) _interpretKeyEvent:savingCommands:] (self=0x12b3a7470, _cmd=0x1011ff8c2, event=0x12b2195c0, savingCommands=0 '\0') at WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:5333 #22 WebEditorClient::handleKeyboardEvent (this=0x1089258a0, event=0x12b2195c0) at WebKit/Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:593 #23 WebCore::Editor::handleKeyboardEvent (this=0x109015a38, event=0x12b2195c0) at WebKit/Source/WebCore/editing/Editor.cpp:144 #24 WebCore::EventHandler::defaultKeyboardEventHandler (this=0x109015be8, event=0x12b2195c0) at WebKit/Source/WebCore/page/EventHandler.cpp:2658 #25 WebCore::Node::defaultEventHandler (this=0x12b3b1b10, event=0x12b2195c0) at WebKit/Source/WebCore/dom/Node.cpp:2811 #26 WebCore::EventDispatcher::dispatchEvent (this=0x7fff5fbfeb40, event=@0x7fff5fbfeb00) at WebKit/Source/WebCore/dom/EventDispatcher.cpp:346 #27 WebCore::EventDispatchMediator::dispatchEvent (this=0x7fff5fbfebb0, dispatcher=0x7fff5fbfeb40) at WebKit/Source/WebCore/dom/Event.cpp:303 #28 WebCore::EventDispatcher::dispatchEvent (node=0x12b3b1b10, mediator=@0x7fff5fbfebb0) at WebKit/Source/WebCore/dom/EventDispatcher.cpp:54 #29 WebCore::Node::dispatchEvent (this=0x12b3b1b10, event=@0x7fff5fbfec10) at WebKit/Source/WebCore/dom/Node.cpp:2717 #30 WebCore::EventTarget::dispatchEvent (this=0x12b3b1b10, event=@0x7fff5fbfed40, ec=@0x7fff5fbfedcc) at WebKit/Source/WebCore/dom/EventTarget.cpp:320 #31 WebCore::EventHandler::keyEvent (this=0x109015be8, initialKeyEvent=@0x7fff5fbfee30) at WebKit/Source/WebCore/page/EventHandler.cpp:2562 #32 WebCore::EventHandler::keyEvent (this=0x109015be8, event=0x12b2110a0) at WebKit/Source/WebCore/page/mac/EventHandlerMac.mm:129 #33 -[WebHTMLView keyDown:] (self=0x12b3a7470, _cmd=0x7fff82966400, event=0x12b2110a0) at WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:4044

Alexey Proskuryakov

Comment 2 2011-07-29 10:47:39 PDT

> This is reproducible with Safari 5.0.5 - Today's WebKit nightly. To clarify, this is not a regression from Safari/WebKit 5.0.5.

Ryosuke Niwa

Comment 3 2011-07-29 12:06:41 PDT

The problem is that search & cancel buttons and innerTextElement become editable in design mode and DeleteSelectionCommand liberally remove them all :( We should modify rendererIsEditable so that we don't treat these elements as editable.

Ryosuke Niwa

Comment 4 2011-07-29 14:12:02 PDT

I talked with Dimitri and Levi and we agreed that the correct approach here is not to propagate designMode=true into the shadow DOM. So this crash can be fixed by one line change: --- Source/WebCore/dom/Node.cpp (revision 92006) +++ Source/WebCore/dom/Node.cpp (working copy) @@ -781,7 +781,7 @@ bool Node::rendererIsEditable(EditableLevel editableLevel) const { - if (document()->frame() && document()->frame()->page() && document()->frame()->page()->isEditable()) + if (document()->frame() && document()->frame()->page() && document()->frame()->page()->isEditable() && !shadowTreeRootNode()) return true;

Ryosuke Niwa

Comment 5 2011-07-29 15:07:50 PDT

Oops, forget about my last comment. That's not true at all because designMode is now set via CSS style. But RenderTextControlSingleLine::createInnerBlockStyle sets -webkit-user-modify: readonly so I don't know why nodes are editable in the shadow DOM. Need more investigation here. Does shadow DOM inherit document's style?

Ryosuke Niwa

Comment 6 2011-08-01 12:33:21 PDT

Created attachment 102544 [details] fixes the bug

Dimitri Glazkov (Google)

Comment 7 2011-08-01 12:37:39 PDT

Comment on attachment 102544 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=102544&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1940 > + if (isAtShadowBoundary(e)) I wonder if you could just test for e->isShadowRoot(). I think that would make the fix more correct in the sense you mention in the ChangeLog.

Ryosuke Niwa

Comment 8 2011-08-01 13:35:34 PDT

Comment on attachment 102544 [details] fixes the bug Found a much better solution.

Ryosuke Niwa

Comment 9 2011-08-01 13:40:23 PDT

Created attachment 102549 [details] fixes the bug with much less hack

Ryosuke Niwa

Comment 10 2011-08-01 13:42:21 PDT

Comment on attachment 102549 [details] fixes the bug with much less hack View in context: https://bugs.webkit.org/attachment.cgi?id=102549&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1365 > + // Don't propagate user-modify into shadow DOM I guess this comment just repeats what the code says. Will remove it before landing the patch if r+ed.

Darin Adler

Comment 11 2011-08-01 13:54:09 PDT

Comment on attachment 102549 [details] fixes the bug with much less hack View in context: https://bugs.webkit.org/attachment.cgi?id=102549&action=review >> Source/WebCore/css/CSSStyleSelector.cpp:1365 >> + // Don't propagate user-modify into shadow DOM > > I guess this comment just repeats what the code says. Will remove it before landing the patch if r+ed. Instead of removing it you could put a why comment in. Something like “Even if surrounding content is user-editable, shadow DOM should act as a single unit, and not necessarily be editable.”

Ryosuke Niwa

Comment 12 2011-08-01 14:08:22 PDT

Committed r92139: <http://trac.webkit.org/changeset/92139>

Ryosuke Niwa

Comment 13 2011-08-01 14:09:19 PDT

(In reply to comment #11) > Instead of removing it you could put a why comment in. Something like “Even if surrounding content is user-editable, shadow DOM should act as a single unit, and not necessarily be editable.” Will do now.

Ryosuke Niwa

Comment 14 2011-08-01 14:11:35 PDT

Huh... I somehow landed the patch with the comment in it. Anyway, updated to what Darin suggested in http://trac.webkit.org/changeset/92140.

Note You need to log in before you can comment on or make changes to this bug.