Bug 57755

Summary: REGRESSION(r81328): Null pointer crash in canAppendNewLineFeed when selection isn't inside an editable element
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, honten, isherman, rniwa, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Attachments:
Description Flags
Repro
none
Patch
none
Patch
none
Patch none

Berend-Jan Wever
Reported 2011-04-04 07:57:10 PDT
Created attachment 88058 [details] Repro Chromium: http://code.google.com/p/chromium/issues/detail?id=78320 "selection.rootEditableElement()" can return NULL, something that "canAppendNewLineFeed" does not handle: static bool canAppendNewLineFeed(const VisibleSelection& selection) { ExceptionCode ec = 0; RefPtr<BeforeTextInsertedEvent> event = BeforeTextInsertedEvent::create(String("\n")); selection.rootEditableElement()->dispatchEvent(event, ec); // CRASH return event->text().length(); } Repro: <body onload="go()"> <script> function go() { document.designMode="on"; document.write("</"); document.getSelection().addRange(document.createRange()); document.execCommand("InsertLineBreak"); } </script> id: chrome.dll!WebCore::EventTarget::dispatchEvent ReadAV@NULL (6c815dec64b94a3dde8974a25da7f213) description: Attempt to read from unallocated NULL pointer in chrome.dll!WebCore::EventTarget::dispatchEvent application: Chromium 12.0.725.0 stack: chrome.dll!WebCore::EventTarget::dispatchEvent chrome.dll!WebCore::canAppendNewLineFeed chrome.dll!WebCore::TypingCommand::insertLineBreak chrome.dll!WebCore::EditCommand::apply chrome.dll!WebCore::applyCommand chrome.dll!WebCore::TypingCommand::insertLineBreak chrome.dll!WebCore::executeInsertLineBreak chrome.dll!WebCore::Editor::Command::execute chrome.dll!WebCore::Document::execCommand chrome.dll!WebCore::DocumentInternal::execCommandCallback chrome.dll!v8::internal::HandleApiCallHelper<...> chrome.dll!v8::internal::Builtin_HandleApiCall chrome.dll!v8::internal::Invoke chrome.dll!v8::internal::Execution::Call ...
Attachments
Repro (224 bytes, text/html)
2011-04-04 07:57 PDT, Berend-Jan Wever 		no flags
Details
Patch (4.02 KB, patch)
2011-04-05 19:44 PDT, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Patch (3.63 KB, patch)
2011-04-05 23:44 PDT, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Patch (3.66 KB, patch)
2011-04-05 23:50 PDT, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-04-05 05:02:28 PDT
Naoki, could you fix this crash?
Naoki Takano
Comment 2 2011-04-05 09:03:13 PDT
Ok, I'll look into it.
Naoki Takano
Comment 3 2011-04-05 18:27:23 PDT
I can reproduce ToT.
Naoki Takano
Comment 4 2011-04-05 19:44:05 PDT
Created attachment 88353 [details] Patch
Naoki Takano
Comment 5 2011-04-05 19:44:26 PDT
Comment on attachment 88353 [details] Patch Please review.
Ryosuke Niwa
Comment 6 2011-04-05 23:22:21 PDT
Comment on attachment 88353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88353&action=review > LayoutTests/editing/execCommand/insert-line-break-onload.html:1 > +<body onload="go()"> Missing DOCTYPE. > LayoutTests/editing/execCommand/insert-line-break-onload.html:2 > +Test InsertLineBreak is called correctly without any exception. Mn... I'd expect that adding this line would stop reproducing the crash. Did you make sure this test case cause a crash? > LayoutTests/editing/execCommand/insert-line-break-onload.html:10 > + var result = "Calling InsertLineBreak succeeded without any exception."; I'd just print 'PASS' with the test description because that's what other tests do. > LayoutTests/editing/execCommand/insert-line-break-onload.html:15 > + } else > + alert(result); Why do you need an alert here? I'd expect for document.write to work here as well. > Source/WebCore/editing/TypingCommand.cpp:52 > + if (Node* node = selection.rootEditableElement()) { We normally do an early exit in these cases.
Naoki Takano
Comment 7 2011-04-05 23:33:25 PDT
Thank you for your review. (In reply to comment #6) > > LayoutTests/editing/execCommand/insert-line-break-onload.html:2 > > +Test InsertLineBreak is called correctly without any exception. > > Mn... I'd expect that adding this line would stop reproducing the crash. Did you make sure this test case cause a crash? Yes, it crashes. > > LayoutTests/editing/execCommand/insert-line-break-onload.html:15 > > + } else > > + alert(result); > > Why do you need an alert here? I'd expect for document.write to work here as well. Because I referred to insert-line-break-no-scroll.html Anyway I'll change it to use document.write().
Naoki Takano
Comment 8 2011-04-05 23:44:56 PDT
Created attachment 88376 [details] Patch
Naoki Takano
Comment 9 2011-04-05 23:45:16 PDT
Comment on attachment 88376 [details] Patch Please review again.
Naoki Takano
Comment 10 2011-04-05 23:47:09 PDT
Comment on attachment 88376 [details] Patch Oops, I forgot to change one more thing...
Naoki Takano
Comment 11 2011-04-05 23:50:36 PDT
Created attachment 88377 [details] Patch
Naoki Takano
Comment 12 2011-04-05 23:50:53 PDT
Comment on attachment 88377 [details] Patch Ok, please review again.
WebKit Commit Bot
Comment 13 2011-04-06 00:22:47 PDT
Comment on attachment 88377 [details] Patch Clearing flags on attachment: 88377 Committed r83026: <http://trac.webkit.org/changeset/83026>
WebKit Commit Bot
Comment 14 2011-04-06 00:22:53 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 15 2011-04-06 01:52:57 PDT
http://trac.webkit.org/changeset/83026 might have broken GTK Linux 32-bit Debug
Note You need to log in before you can comment on or make changes to this bug.