RESOLVED FIXED 66288
Selecting all and inserting text into a page with a frameset leads to a NULL ptr 
https://bugs.webkit.org/show_bug.cgi?id=66288
Summary Selecting all and inserting text into a page with a frameset leads to a NULL ptr
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
DetailsFormatted DiffDiff
Patch (4.49 KB, patch)
2011-08-17 10:07 PDT, Wyatt Carss 		no flags
DetailsFormatted DiffDiff
improved comment / fixed test nit (4.94 KB, patch)
2011-08-17 14:16 PDT, Wyatt Carss 		no flags
DetailsFormatted DiffDiff
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.