Bug 66288

Summary:

Selecting all and inserting text into a page with a frameset leads to a NULL ptr

Product:

WebKit

Reporter:

Berend-Jan Wever <skylined>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, eric, rniwa, tony, wcarss, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows Vista

Attachments:

Description	Flags
Repro	none
Patch	none
Patch	none
improved comment / fixed test nit	none

Berend-Jan Wever

Reported 2011-08-16 03:34:52 PDT

Created attachment 104020 [details] Repro Repro: <html> <script> setTimeout(function() { document.designMode="on"; document.execCommand("selectall"); document.execCommand("InsertText",false); }, 100); </script> <frameset><frame></frame></frameset> </html> id: chrome.dll!WebCore::Node::nodeIndex ReadAV@NULL (2478f9a3629a4d08efa42d9180043895) description: Attempt to read from unallocated NULL pointer+0x18 in chrome.dll!WebCore::Node::nodeIndex stack: chrome.dll!WebCore::Node::nodeIndex chrome.dll!WebCore::positionInParentBeforeNode chrome.dll!WebCore::InsertTextCommand::doApply chrome.dll!WebCore::EditCommand::apply chrome.dll!WebCore::CompositeEditCommand::applyCommandToComposite chrome.dll!WebCore::TypingCommand::insertTextRunWithoutNewlines chrome.dll!WebCore::TypingCommand::insertText chrome.dll!WebCore::TypingCommand::doApply chrome.dll!WebCore::EditCommand::apply chrome.dll!WebCore::applyCommand chrome.dll!WebCore::TypingCommand::insertText chrome.dll!WebCore::TypingCommand::insertText chrome.dll!WebCore::executeInsertText chrome.dll!WebCore::Editor::Command::execute chrome.dll!WebCore::Document::execCommand chrome.dll!WebCore::DocumentInternal::execCommandCallback ... This causes a selection which has no anchor node, something the code does not handle: void InsertTextCommand::doApply() { <<<snip>>> // It is possible for the node that contains startPosition to contain only unrendered whitespace, // and so deleteInsignificantText could remove it. Save the position before the node in case that happens. Position positionBeforeStartNode(positionInParentBeforeNode(startPosition.containerNode())); <<<snip>>> "startPosition.containerNode()" return NULL, which causes a NULL pointer in "positionInParentBeforeNode": inline Position positionInParentBeforeNode(const Node* node) { // FIXME: This should ASSERT(node->parentNode()) // At least one caller currently hits this ASSERT though, which indicates // that the caller is trying to make a position relative to a disconnected node (which is likely an error) // Specifically, editing/deleting/delete-ligature-001.html crashes with ASSERT(node->parentNode()) return Position(node->nonShadowBoundaryParentNode(), node->nodeIndex(), Position::PositionIsOffsetInAnchor); }

Attachments
Repro (239 bytes, text/html) 2011-08-16 03:34 PDT, Berend-Jan Wever	no flags	Details
Patch (4.33 KB, patch) 2011-08-16 19:34 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (4.49 KB, patch) 2011-08-17 10:07 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
improved comment / fixed test nit (4.94 KB, patch) 2011-08-17 14:16 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Berend-Jan Wever

Comment 1 2011-08-16 03:35:41 PDT

Chromium: https://code.google.com/p/chromium/issues/detail?id=93030

Wyatt Carss

Comment 2 2011-08-16 19:34:17 PDT

Created attachment 104140 [details] Patch

Eric Seidel (no email)

Comment 3 2011-08-16 23:46:54 PDT

Comment on attachment 104140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104140&action=review > Source/WebCore/editing/InsertTextCommand.cpp:112 > + if (endingSelection().isNone()) > + return; Should we have a comment here?

Wyatt Carss

Comment 4 2011-08-17 10:07:41 PDT

Created attachment 104187 [details] Patch

Tony Chang

Comment 5 2011-08-17 11:01:55 PDT

Comment on attachment 104187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104187&action=review > LayoutTests/editing/inserting/insert-text-into-empty-frameset-crash.html:1 > +<html> Nit: <DOCTYPE html> > Source/WebCore/editing/InsertTextCommand.cpp:110 > deleteSelection(false, true, true, false); Wow, this function should take enums rather than 4 bools. (Unrelated to your patch, but would be a good place for a refactoring patch.)

Eric Seidel (no email)

Comment 6 2011-08-17 11:04:16 PDT

Comment on attachment 104187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104187&action=review > Source/WebCore/editing/InsertTextCommand.cpp:111 > + // deleteSelection may clobber the endingSelection due to some validation wackiness in VisibleSelection. Can you be more descriptive than "wackiness"? Maybe link to a bug about said wackiness? I appreciate the extra comment, btw. The goal here would be to understand why this code is here, and under what conditions we should remove it. If there are no conditions, then we just need to understand the "why" which you gave, but in somewhat imprecise terms. You certainly can link to this bug for more info if you'd rather too. This is a total trivial nit, but I'm unfamiliar with this code and your comment is helping me better understand what you're doing. :) Thank you!

Wyatt Carss

Comment 7 2011-08-17 14:16:28 PDT

Created attachment 104241 [details] improved comment / fixed test nit

Wyatt Carss

Comment 8 2011-08-17 14:32:52 PDT

(In reply to comment #7) > Created an attachment (id=104241) [details] > improved comment / fixed test nit Thanks, Tony. :)

WebKit Review Bot

Comment 9 2011-08-18 01:44:16 PDT

Comment on attachment 104241 [details] improved comment / fixed test nit Clearing flags on attachment: 104241 Committed r93290: <http://trac.webkit.org/changeset/93290>

WebKit Review Bot

Comment 10 2011-08-18 01:44:20 PDT

All reviewed patches have been landed. Closing bug.

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