Bug 56372

Summary:

Crash in ReplaceSelectionCommand::doApply when inserting a node under a document node

Product:

WebKit

Reporter:

Berend-Jan Wever <skylined>

Component:

HTML Editing

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric, ojan, rniwa, tkent, tony

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

56421

Attachments:

Description	Flags
Repro	none
fixes the crash	none

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.)