Bug 43071

Summary: ASSERTION FAILURE in AppendNodeCommand::AppendNodeCommand when indenting HTML
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WORKSFORME    
Severity: Normal CC: ap, darin, dbates, enrica, eric, justin.garcia, levin, rniwa
Priority: P1 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://code.google.com/p/chromium/issues/detail?id=50404
Attachments:
Description Flags
Repro - WebCore::AppendNodeCommand::AppendNodeCommand ReadAV@NULL (ae8a44eb1a4d3115a1236246b62cf2d1)
none
Crash report using Mac nightly r64156
none
nastier crash none

Berend-Jan Wever
Reported 2010-07-27 13:03:57 PDT
Created attachment 62732 [details] Repro - WebCore::AppendNodeCommand::AppendNodeCommand ReadAV@NULL (ae8a44eb1a4d3115a1236246b62cf2d1) <html> <head> <script> document.designMode="on"; document.writeln('x<isIndex/><x><object>x</object>'); selection=window.getSelection(); selection.selectAllChildren(document); document.execCommand("indent"); </script> </head> <body onload="go()"> </body> </html> The above causes a NULL pointer crash: id: WebCore::AppendNodeCommand::AppendNodeCommand ReadAV@NULL (ae8a44eb1a4d3115a1236246b62cf2d1) description: Attempt to read from NULL pointer (+0x14) in WebCore::AppendNodeCommand::AppendNodeCommand stack: WebCore::AppendNodeCommand::AppendNodeCommand WebCore::AppendNodeCommand::create WebCore::CompositeEditCommand::appendNode WebCore::CompositeEditCommand::cloneParagraphUnderNewElement WebCore::CompositeEditCommand::moveParagraphWithClones WebCore::IndentOutdentCommand::indentIntoBlockquote WebCore::IndentOutdentCommand::indentRegion WebCore::IndentOutdentCommand::doApply WebCore::EditCommand::apply WebCore::applyCommand WebCore::executeIndent WebCore::Editor::Command::execute WebCore::Document::execCommand ...
Attachments
Repro - WebCore::AppendNodeCommand::AppendNodeCommand ReadAV@NULL (ae8a44eb1a4d3115a1236246b62cf2d1) (308 bytes, text/html)
2010-07-27 13:03 PDT, Berend-Jan Wever 		no flags
Details
Crash report using Mac nightly r64156 (39.12 KB, text/plain)
2010-07-27 23:48 PDT, Daniel Bates 		no flags
Details
nastier crash (664 bytes, text/html)
2010-09-28 17:43 PDT, Ryosuke Niwa 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 2010-07-27 23:48:47 PDT
Created attachment 62796 [details] Crash report using Mac nightly r64156
Daniel Bates
Comment 2 2010-07-28 00:02:37 PDT
The assertion on line 39 of AppendNodeCommand.cpp <http://trac.webkit.org/browser/trunk/WebCore/editing/AppendNodeCommand.cpp?rev=51645#L39> fails: ASSERT(m_node)
Ryosuke Niwa
Comment 3 2010-08-02 12:10:37 PDT
This bug seems to be caused by indentIntoBlockquote calling moveParagraphWithClones with a wrong outerBlock (i.e. outerBlock doesn't contain startOfParagraphToMove). As a result, cloneParagraphUnderNewElement tries to copy everything up to Document node and fails on appendChild.
Ryosuke Niwa
Comment 4 2010-09-28 17:01:10 PDT
I spent some time on this bug and I think the problem is that when there are nodes between nodeToSplitTo and start.node(), splitTreeToNode may shift the start of paragraph to inside of the nodes being duplicated. For example, when we are on the following node in the attached reduction: BODY 0x106263a00 BLOCKQUOTE 0x1062367d0 CLASS=webkit-indent-blockquote STYLE=margin: 0 0 0 40px; border: none; padding: 0px; #text 0x108280ee0 "x" FORM 0x108279320 HR 0x108291aa0 FORM 0x106287c80 LABEL 0x106287380 #text 0x11a802d80 "This is a searchable index. Enter search keywords: " INPUT 0x11a802f40 FORM 0x106284e90 HR 0x106285150 X 0x1062856e0 OBJECT 0x106285940 * #text 0x106285ab0 "x" #text 0x106285ec0 "\n" We end up copying X and Object to get: BODY 0x106263a00 BLOCKQUOTE 0x1062367d0 CLASS=webkit-indent-blockquote STYLE=margin: 0 0 0 40px; border: none; padding: 0px; #text 0x108280ee0 "x" FORM 0x108279320 HR 0x108291aa0 FORM 0x106287c80 LABEL 0x106287380 #text 0x11a802d80 "This is a searchable index. Enter search keywords: " INPUT 0x11a802f40 FORM 0x106284e90 HR 0x106285150 X 0x106284e10 @ OBJECT 0x106270fa0 $ X 0x1062856e0 OBJECT 0x106285940 * #text 0x106285ab0 "x" #text 0x106285ec0 "\n" At which point the start of paragraph is shifted to @ even though the outerBlock is still at $.
Ryosuke Niwa
Comment 5 2010-09-28 17:38:04 PDT
Replacing RefPtr<Node> outerBlock = (start.node() == nodeToSplitTo) ? start.node() : splitTreeToNode(start.node(), nodeToSplitTo); by + RefPtr<Node> outerBlock = start.node(); + bool different = false; + for (Node* n = start.node(); n && n != nodeToSplitTo; n = n->parentNode()) + if (visiblePositionBeforeNode(n) != visiblePositionBeforeNode(start.node())) + different = true; + if (start.node() != nodeToSplitTo && different) + outerBlock = splitTreeToNode(start.node(), nodeToSplitTo); seems to work. The idea is to avoid splitting the tree when nodes between start.node() and nodeToSplitTo belong to the current paragraph.
Ryosuke Niwa
Comment 6 2010-09-28 17:43:27 PDT
Created attachment 69143 [details] nastier crash This is a reduced test cause of the crash report sent by David Levin.
Berend-Jan Wever
Comment 7 2010-09-29 09:26:00 PDT
Could you explain why that crash is nastier? It's also a NULL pointer, so security wise it doesn't seem to matter much.
Berend-Jan Wever
Comment 8 2010-09-29 09:26:40 PDT
Actually, it triggers the exact same NULL pointer crash for me as the original repro.
Ryosuke Niwa
Comment 9 2010-09-29 09:30:22 PDT
(In reply to comment #7) > Could you explain why that crash is nastier? It's also a NULL pointer, so security wise it doesn't seem to matter much. (In reply to comment #8) > Actually, it triggers the exact same NULL pointer crash for me as the original repro. Yes, it does trigger the same crash but this one cannot be fully fixed by my suggested fix. Even though the crash ceases I still get an unexpected value.
Berend-Jan Wever
Comment 10 2010-12-03 01:57:48 PST
This no longer reproduces in latest Chromium.
Note You need to log in before you can comment on or make changes to this bug.