Bug 56372

Summary: Crash in ReplaceSelectionCommand::doApply when inserting a node under a document node
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, ojan, rniwa, tkent, tony
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 56421    
Attachments:
Description Flags
Repro
none
fixes the crash none

Berend-Jan Wever
Reported 2011-03-15 06:48:57 PDT
Created attachment 85800 [details] Repro Chromium: http://code.google.com/p/chromium/issues/detail?id=76183 It is possible to make any HTMLElement a child of the HTMLDocument. In this case HTMLElement::parentElement returns NULL because a HTMLDocument is not an HTMLElement. It is also possible to execute CompositeEditCommand::insertNodeAfter to insert another HTMLElement after such an HTMLElement. CompositeEditCommand::insertNodeAfter calls HTMLElement::parentElement and expects it to return an HTMLElement. Combined, this can cause NULL pointer exceptions. I'm not entirely sure where the problem is; it could be that CompositeEditCommand::insertNodeAfter should work on Nodes rather than HTMLElements. It could also be that the code should never get this far and throw a JavaScript exception if it detects this situation. I suspect the former, it makes sense considering the name of the function. Repro: <body onload="go()"><script>function go() { var sElement = ['iframe', 'img', 'embed', 'textarea'][0]; // Select which element you want to test. document.open(); var oElement1 = document.appendChild(document.createElement(sElement)); var oElement2 = oElement1.appendChild(document.createElement(sElement)); document.designMode = "on"; oElement1.focus(); document.execCommand("InsertHorizontalRule"); }</script></body> id: chrome.dll!WebCore::CompositeEditCommand::insertNodeAfter ReadAV@NULL (cf23d1b576db7f88c78bb2e323691274) description: Attempt to read from unallocated NULL pointer+0x2C in chrome.dll!WebCore::CompositeEditCommand::insertNodeAfter application: Chromium 12.0.705.0 stack: chrome.dll!WebCore::CompositeEditCommand::insertNodeAfter chrome.dll!WebCore::CompositeEditCommand::insertNodeAt chrome.dll!WebCore::ReplaceSelectionCommand::insertNodeAtAndUpdateNodesInserted chrome.dll!WebCore::ReplaceSelectionCommand::doApply chrome.dll!WebCore::EditCommand::apply chrome.dll!WebCore::applyCommand chrome.dll!WebCore::executeInsertFragment chrome.dll!WebCore::executeInsertNode chrome.dll!WebCore::executeInsertHorizontalRule chrome.dll!WebCore::Editor::Command::execute chrome.dll!WebCore::Document::execCommand ...
Attachments
Repro (430 bytes, text/html)
2011-03-15 06:48 PDT, Berend-Jan Wever 		no flags
Details
fixes the crash (8.41 KB, patch)
2011-03-15 12:49 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-03-15 12:49:37 PDT
Created attachment 85844 [details] fixes the crash
Darin Adler
Comment 2 2011-03-15 14:55:22 PDT
Comment on attachment 85844 [details] fixes the crash View in context: https://bugs.webkit.org/attachment.cgi?id=85844&action=review > Source/WebCore/dom/Document.cpp:4134 > +bool Document::isContentEditable() const > +{ > + if (document()->inDesignMode()) > + return true; > + > + return renderer() && (renderer()->style()->userModify() == READ_WRITE || renderer()->style()->userModify() == READ_WRITE_PLAINTEXT_ONLY); > +} > + > +bool Document::isContentRichlyEditable() const > +{ > + if (document()->inDesignMode()) > + return true; > + > + return renderer() && renderer()->style()->userModify() == READ_WRITE; > +} Is there a way we can share code with the HTMLElement functions? It’s not good to have a near exact copy of those functions in the document class. Also, it’s strange to call document() in a function on Document. > Source/WebCore/dom/Document.h:898 > + virtual bool isContentEditable() const; > + virtual bool isContentRichlyEditable() const; We often make this sort of function private, because if it’s going to be called in performance-critical code and you have a Document*, then it would be better to have a non-virtual version of the function. I think that makes sense in this case. Nobody needs to call isContentEditable on a Document* yet. If they did we could provide something slightly faster.
Darin Adler
Comment 3 2011-03-15 14:55:52 PDT
Comment on attachment 85844 [details] fixes the crash The review tool changed this back to review? But I don’t want to change it to review+ because then it will look like I reviewed it. Can someone help me fix this?
Darin Adler
Comment 4 2011-03-15 14:56:34 PDT
(In reply to comment #3) > The review tool changed this back to review? But I don’t want to change it to review+ because then it will look like I reviewed it. Can someone help me fix this? I didn’t intentionally remove the review flag.
Darin Adler
Comment 5 2011-03-15 14:58:24 PDT
Tony, you could resolve this by setting review+ again.
Ryosuke Niwa
Comment 6 2011-03-15 15:04:31 PDT
Thanks for the review, Tony. (In reply to comment #2) > Is there a way we can share code with the HTMLElement functions? It’s not good to have a near exact copy of those functions in the document class. I thought about it too but isContentEditable is in a really hot path, and I can't increase any latency there. > Also, it’s strange to call document() in a function on Document. Oops, will fix. > We often make this sort of function private, because if it’s going to be called in performance-critical code and you have a Document*, then it would be better to have a non-virtual version of the function. I think that makes sense in this case. Nobody needs to call isContentEditable on a Document* yet. If they did we could provide something slightly faster. We could make them non-virtual function of Node.
Ryosuke Niwa
Comment 7 2011-03-15 15:06:59 PDT
Okay, let's land this patch as is (Reviewed by Tony) and then refactor these functions in a separate patch.
Ryosuke Niwa
Comment 8 2011-03-15 15:38:03 PDT
Committed r81185: <http://trac.webkit.org/changeset/81185>
David Levin
Comment 9 2011-03-15 22:16:16 PDT
Comment on attachment 85844 [details] fixes the crash Clearing r? from landed patch. (It was given a r+ also.)
Note You need to log in before you can comment on or make changes to this bug.