Bug 53263 – contentEditable formatBlock crashes on divs with contenteditable="false"

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Emil A Eklund]

Executing a formatBlock command on a selection containing an element with contenteditable="false" causes a crash due to a null pointer.

Downstream bug: http://code.google.com/p/chromium/issues/detail?id=70160

[Emil A Eklund]

Created attachment 80371 [details]
Patch

[Darin Adler]

Comment on attachment 80371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80371&action=review

> Source/WebCore/editing/FormatBlockCommand.cpp:72
> +    // Root is null for elements with contenteditable=false.
> +    if (!root)
> +        return;

Why don’t other commands need this kind of check? Is this really specific to FormatBlock?

[Emil A Eklund]

editableRootForPosition is only used by a handfull of commands and FormatBlock appears to be the only one where it's assumed to always return a node.

I tried quite a few commands and FormatBlock was the only one I could get to trigger a crash.

I could include more commands in the test if you'd like (and make it a generic contenteditable=false/execCommand test).

[Darin Adler]

(In reply to comment #3)
> I could include more commands in the test if you'd like (and make it a generic contenteditable=false/execCommand test).

Sure, that’d be nice long term, but not needed now.

[Darin Adler]

Comment on attachment 80371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80371&action=review

>> Source/WebCore/editing/FormatBlockCommand.cpp:72
>> +        return;
> 
> Why don’t other commands need this kind of check? Is this really specific to FormatBlock?

I figured out the answer to my own question. The simple editing commands all have code in them that checks and does nothing if the nodes involved are not editable. And the complex commands are built out of the simple commands. So in most cases, no checking is needed.

There may be some advantage to doing some higher level checking so we don’t end up with undoable commands on the undo chain that work hard to do nothing.

[Emil A Eklund]

Makes sense. Thanks for the review!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 80371 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 80371 [details]
Patch

Clearing flags on attachment: 80371

Committed r76903: <http://trac.webkit.org/changeset/76903>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.