53263 2011-01-27 15:35:50 -0800 contentEditable formatBlock crashes on divs with contenteditable="false" 2011-01-27 18:41:52 -0800 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 eae webkit-unassigned commit-queue darin oldest_to_newest 341086 0 eae 2011-01-27 15:35:50 -0800 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 341089 1 80371 eae 2011-01-27 15:50:50 -0800 Created attachment 80371 Patch 341092 2 80371 darin 2011-01-27 15:59:54 -0800 Comment on attachment 80371 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? 341098 3 eae 2011-01-27 16:10:57 -0800 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). 341108 4 darin 2011-01-27 16:15:12 -0800 (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. 341109 5 80371 darin 2011-01-27 16:16:36 -0800 Comment on attachment 80371 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. 341112 6 eae 2011-01-27 16:19:27 -0800 Makes sense. Thanks for the review! 341234 7 commit-queue 2011-01-27 18:39:15 -0800 The commit-queue encountered the following flaky tests while processing attachment 80371: http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org) The commit-queue is continuing to process your patch. 341237 8 80371 commit-queue 2011-01-27 18:41:49 -0800 Comment on attachment 80371 Patch Clearing flags on attachment: 80371 Committed r76903: <http://trac.webkit.org/changeset/76903> 341238 9 commit-queue 2011-01-27 18:41:52 -0800 All reviewed patches have been landed. Closing bug. 80371 2011-01-27 15:50:50 -0800 2011-01-27 18:41:49 -0800 Patch 53263.patch text/plain 4359 eae SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDc2ODU4KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDEtMjcgIEVtaWwgQSBF a2x1bmQgIDxlYWVAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIGNvbnRlbnRFZGl0YWJsZSBmb3JtYXRCbG9jayBjcmFzaGVzIG9u IGRpdnMgd2l0aCBjb250ZW50ZWRpdGFibGU9ImZhbHNlIgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTMyNjMKKworICAgICAgICBDaGVjayBpZiBlZGl0 YWJsZVJvb3RGb3JQb3NpdGlvbiByZXR1cm5zIG51bGwgZm9yIHBvc2l0aW9uLgorCisgICAgICAg IFRlc3Q6IGVkaXRpbmcvZXhlY0NvbW1hbmQvZm9ybWF0LWJsb2NrLWNvbnRlbnRlZGl0YWJsZS1m YWxzZS5odG1sCisKKyAgICAgICAgKiBlZGl0aW5nL0Zvcm1hdEJsb2NrQ29tbWFuZC5jcHA6Cisg ICAgICAgIChXZWJDb3JlOjpGb3JtYXRCbG9ja0NvbW1hbmQ6OmZvcm1hdFJhbmdlKToKKwogMjAx MS0wMS0yNyAgQWRhbSBSb2JlbiAgPGFyb2JlbkBhcHBsZS5jb20+CiAKICAgICAgICAgQWRkIFdL Q0FDRlZpZXdMYXllclRyZWVIb3N0CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9lZGl0aW5nL0Zvcm1h dEJsb2NrQ29tbWFuZC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvZWRpdGluZy9G b3JtYXRCbG9ja0NvbW1hbmQuY3BwCShyZXZpc2lvbiA3NjcyOCkKKysrIFNvdXJjZS9XZWJDb3Jl L2VkaXRpbmcvRm9ybWF0QmxvY2tDb21tYW5kLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjcsNiAr NjcsOSBAQCB2b2lkIEZvcm1hdEJsb2NrQ29tbWFuZDo6Zm9ybWF0UmFuZ2UoY29uCiAgICAgUmVm UHRyPFJhbmdlPiByYW5nZSA9IFJhbmdlOjpjcmVhdGUoZG9jdW1lbnQoKSwgc3RhcnQsIGVuZE9m U2VsZWN0aW9uKTsKICAgICBFbGVtZW50KiByZWZOb2RlID0gZW5jbG9zaW5nQmxvY2tGbG93RWxl bWVudChlbmQpOwogICAgIEVsZW1lbnQqIHJvb3QgPSBlZGl0YWJsZVJvb3RGb3JQb3NpdGlvbihz dGFydCk7CisgICAgLy8gUm9vdCBpcyBudWxsIGZvciBlbGVtZW50cyB3aXRoIGNvbnRlbnRlZGl0 YWJsZT1mYWxzZS4KKyAgICBpZiAoIXJvb3QpCisgICAgICAgIHJldHVybjsKICAgICBpZiAoaXNF bGVtZW50Rm9yRm9ybWF0QmxvY2socmVmTm9kZS0+dGFnUU5hbWUoKSkgJiYgc3RhcnQgPT0gc3Rh cnRPZkJsb2NrKHN0YXJ0KQogICAgICAgICAmJiAoZW5kID09IGVuZE9mQmxvY2soZW5kKSB8fCBp c05vZGVWaXNpYmx5Q29udGFpbmVkV2l0aGluKHJlZk5vZGUsIHJhbmdlLmdldCgpKSkKICAgICAg ICAgJiYgcmVmTm9kZSAhPSByb290ICYmICFyb290LT5pc0Rlc2NlbmRhbnRPZihyZWZOb2RlKSkg ewpJbmRleDogTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3Rz L0NoYW5nZUxvZwkocmV2aXNpb24gNzY4NTgpCisrKyBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHdv cmtpbmcgY29weSkKQEAgLTEsMyArMSwxNiBAQAorMjAxMS0wMS0yNyAgRW1pbCBBIEVrbHVuZCAg PGVhZUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgY29udGVudEVkaXRhYmxlIGZvcm1hdEJsb2NrIGNyYXNoZXMgb24gZGl2cyB3 aXRoIGNvbnRlbnRlZGl0YWJsZT0iZmFsc2UiCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD01MzI2MworCisgICAgICAgIEFkZCB0ZXN0IGZvciBleGVjdXRp bmcgYSBmb3JtYXRCbG9jayBjb21tYW5kIG9uIGEgc2VsZWN0aW9uIGNvbnRhaW5pbmcgYW4KKyAg ICAgICAgZWxlbWVudCB3aXRoIGNvbnRlbnRlZGl0YWJsZT1mYWxzZS4KKworICAgICAgICAqIGVk aXRpbmcvZXhlY0NvbW1hbmQvZm9ybWF0LWJsb2NrLWNvbnRlbnRlZGl0YWJsZS1mYWxzZS1leHBl Y3RlZC50eHQ6IEFkZGVkLgorICAgICAgICAqIGVkaXRpbmcvZXhlY0NvbW1hbmQvZm9ybWF0LWJs b2NrLWNvbnRlbnRlZGl0YWJsZS1mYWxzZS5odG1sOiBBZGRlZC4KKwogMjAxMS0wMS0yNyAgRGly ayBTY2h1bHplICA8a3JpdEB3ZWJraXQub3JnPgogCiAgICAgICAgIFVucmV2aWV3ZWQgcmViYXNl bGluZS4KSW5kZXg6IExheW91dFRlc3RzL2VkaXRpbmcvZXhlY0NvbW1hbmQvZm9ybWF0LWJsb2Nr LWNvbnRlbnRlZGl0YWJsZS1mYWxzZS1leHBlY3RlZC50eHQKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0 VGVzdHMvZWRpdGluZy9leGVjQ29tbWFuZC9mb3JtYXQtYmxvY2stY29udGVudGVkaXRhYmxlLWZh bHNlLWV4cGVjdGVkLnR4dAkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2VkaXRpbmcvZXhl Y0NvbW1hbmQvZm9ybWF0LWJsb2NrLWNvbnRlbnRlZGl0YWJsZS1mYWxzZS1leHBlY3RlZC50eHQJ KHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMTYgQEAKK2Zvcm1hdGJsb2NrIG9uIHNlbGVjdGlvbiB3 aXRoIGEgY29udGVudGVkaXRhYmxlPSJmYWxzZSIgY2hpbGQuCit8IDxoMT4KK3wgICA8aT4KK3wg ICAgICIKKyAgICAgICAgPCNzZWxlY3Rpb24tYW5jaG9yPldpbGwgc2VsZWN0IGZyb20gaGVyZQor ICAgICAgICAiCit8ICAgPGJyPgorfCAgIDxpPgorfCAgICAgIgorICAgICAgICB1bnRpbCBoZXJl Ljwjc2VsZWN0aW9uLWZvY3VzPgorICAgICIKK3wgPGRpdj4KK3wgICBjb250ZW50ZWRpdGFibGU9 ImZhbHNlIgorfCAgICIKKyAgICAgICAgICAgIG92ZXIgdGhpcyBjb250ZW50ZWRpdGFibGU9ZmFs c2UgZGl2CisgICAgICAgICIKSW5kZXg6IExheW91dFRlc3RzL2VkaXRpbmcvZXhlY0NvbW1hbmQv Zm9ybWF0LWJsb2NrLWNvbnRlbnRlZGl0YWJsZS1mYWxzZS5odG1sCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExh eW91dFRlc3RzL2VkaXRpbmcvZXhlY0NvbW1hbmQvZm9ybWF0LWJsb2NrLWNvbnRlbnRlZGl0YWJs ZS1mYWxzZS5odG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvZWRpdGluZy9leGVjQ29t bWFuZC9mb3JtYXQtYmxvY2stY29udGVudGVkaXRhYmxlLWZhbHNlLmh0bWwJKHJldmlzaW9uIDAp CkBAIC0wLDAgKzEsMzIgQEAKKzwhRE9DVFlQRSBodG1sPgorPGh0bWw+Cis8aGVhZD4KKyAgICA8 c2NyaXB0IHNyYz0iLi4vLi4vcmVzb3VyY2VzL2R1bXAtYXMtbWFya3VwLmpzIj48L3NjcmlwdD4K KzwvaGVhZD4KKzxib2R5PgorICAgIDxkaXYgaWQ9ImVkaXRvcmNvbnRhaW5lciIgY29udGVudGVk aXRhYmxlPSJ0cnVlIj4KKyAgICAgICAgV2lsbCBzZWxlY3QgZnJvbSBoZXJlCisgICAgICAgIDxk aXYgY29udGVudGVkaXRhYmxlPSJmYWxzZSI+CisgICAgICAgICAgICBvdmVyIHRoaXMgY29udGVu dGVkaXRhYmxlPWZhbHNlIGRpdgorICAgICAgICA8L2Rpdj4KKyAgICAgICAgdW50aWwgaGVyZS4K KyAgICA8L2Rpdj4KKzwvYm9keT4KKzxzY3JpcHQ+CisgICAgTWFya3VwLmRlc2NyaXB0aW9uKCdm b3JtYXRibG9jayBvbiBzZWxlY3Rpb24gd2l0aCBhIGNvbnRlbnRlZGl0YWJsZT0iZmFsc2UiIGNo aWxkLicpOworICAgIAorICAgIHZhciBzZWxlY3Rpb24gPSB3aW5kb3cuZ2V0U2VsZWN0aW9uKCk7 CisgICAgc2VsZWN0aW9uLnJlbW92ZUFsbFJhbmdlcygpOworCisgICAgdmFyIGNvbnRhaW5lciA9 IGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKCdlZGl0b3Jjb250YWluZXInKTsKKyAgICB2YXIgcmFu Z2UgPSBkb2N1bWVudC5jcmVhdGVSYW5nZSgpCisgICAgcmFuZ2Uuc2V0U3RhcnRCZWZvcmUoY29u dGFpbmVyLmZpcnN0Q2hpbGQpOworICAgIHJhbmdlLnNldEVuZEFmdGVyKGNvbnRhaW5lci5sYXN0 Q2hpbGQpOworICAgIHNlbGVjdGlvbi5hZGRSYW5nZShyYW5nZSk7CisKKyAgICBkb2N1bWVudC5l eGVjQ29tbWFuZCgnaXRhbGljJywgZmFsc2UsICIiKTsKKyAgICBkb2N1bWVudC5leGVjQ29tbWFu ZCgnZm9ybWF0YmxvY2snLCBmYWxzZSwgIjxoMT4iKTsKKworICAgIE1hcmt1cC5kdW1wKGNvbnRh aW5lcik7Cis8L3NjcmlwdD4KKzwvaHRtbD4K