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 ...
Created attachment 85844 [details] fixes the crash
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.
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?
(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.
Tony, you could resolve this by setting review+ again.
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.
Okay, let's land this patch as is (Reviewed by Tony) and then refactor these functions in a separate patch.
Committed r81185: <http://trac.webkit.org/changeset/81185>
Comment on attachment 85844 [details] fixes the crash Clearing r? from landed patch. (It was given a r+ also.)