Bug 56372 - Crash in ReplaceSelectionCommand::doApply when inserting a node under a document node
Summary: Crash in ReplaceSelectionCommand::doApply when inserting a node under a docum...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 56421
  Show dependency treegraph
 
Reported: 2011-03-15 06:48 PDT by Berend-Jan Wever
Modified: 2011-03-15 22:16 PDT (History)
6 users (show)

See Also:


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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 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
                ...
Comment 1 Ryosuke Niwa 2011-03-15 12:49:37 PDT
Created attachment 85844 [details]
fixes the crash
Comment 2 Darin Adler 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.
Comment 3 Darin Adler 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?
Comment 4 Darin Adler 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.
Comment 5 Darin Adler 2011-03-15 14:58:24 PDT
Tony, you could resolve this by setting review+ again.
Comment 6 Ryosuke Niwa 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.
Comment 7 Ryosuke Niwa 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.
Comment 8 Ryosuke Niwa 2011-03-15 15:38:03 PDT
Committed r81185: <http://trac.webkit.org/changeset/81185>
Comment 9 David Levin 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.)