Bug 53263

Summary: contentEditable formatBlock crashes on divs with contenteditable="false"
Product: WebKit Reporter: Emil A Eklund <eae>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch none

Description Emil A Eklund 2011-01-27 15:35:50 PST
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
Comment 1 Emil A Eklund 2011-01-27 15:50:50 PST
Created attachment 80371 [details]
Patch
Comment 2 Darin Adler 2011-01-27 15:59:54 PST
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?
Comment 3 Emil A Eklund 2011-01-27 16:10:57 PST
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).
Comment 4 Darin Adler 2011-01-27 16:15:12 PST
(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.
Comment 5 Darin Adler 2011-01-27 16:16:36 PST
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.
Comment 6 Emil A Eklund 2011-01-27 16:19:27 PST
Makes sense. Thanks for the review!
Comment 7 WebKit Commit Bot 2011-01-27 18:39:15 PST
The commit-queue encountered the following flaky tests while processing attachment 80371 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2011-01-27 18:41:49 PST
Comment on attachment 80371 [details]
Patch

Clearing flags on attachment: 80371

Committed r76903: <http://trac.webkit.org/changeset/76903>
Comment 9 WebKit Commit Bot 2011-01-27 18:41:52 PST
All reviewed patches have been landed.  Closing bug.