97763 2012-09-27 00:31:48 -0700 REGRESSION(r129186): Pressing enter at the end of a line deletes the line 2012-09-27 16:24:55 -0700 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P1 Normal --- 1 jochen leviw ap enrica eric inferno leviw rniwa webkit.review.bot oldest_to_newest 729279 0 jochen 2012-09-27 00:31:48 -0700 Steps to reproduce: 1. I reply to an HTML email in Gmail 2. I put the cursor at the end of a quoted paragraph 3. I press enter expected result: - new line after the paragraph is started observed: - new line is started, but the text of the paragraph is gone I checked that rolling out this revision fixes the issue 729405 1 inferno 2012-09-27 04:12:58 -0700 Check out comment in chromium bug and please add a link here. 729412 2 jochen 2012-09-27 04:23:07 -0700 I figured that I shouldn't just roll out a security fix, yes :) Here's the chromium bug: http://code.google.com/p/chromium/issues/detail?id=152629 It's a bit sad that this didn't break any test 729730 3 leviw 2012-09-27 11:30:23 -0700 I was able to reproduce this with the canary. I'm not as surprised this doesn't break any tests, as there's something specific to the gmail case that's breaking this. The same operation in yahoo mail or in an editor such as rniwa's simple-rte (I copied the source of the email I was repro-ing with into his editor) doesn't reproduce. 729808 4 leviw 2012-09-27 13:25:22 -0700 Managed to get a stacktrace on the bad access. Added that top function for debugging purposes ;) This is an issue we've seen before, where Layout has occurred, but something went wrong in Layout where things were dirtied during layout (I've seen this) or not everything in the tree was laid out (I haven't seen this). Since I can only get this to repro in GMail, a reduction is going to be really difficult. #0 WebCore::InlineTextBox::OMGPROBLEM (this=0x7f121431cd98) at ../../third_party/WebKit/Source/WebCore/rendering/InlineTextBox.cpp:177 #1 0x0000000001bb6c5e in WebCore::InlineTextBox::start (this=0x7f121431cd98) at ../../third_party/WebKit/Source/WebCore/rendering/InlineTextBox.h:72 #2 0x0000000002d8272e in WebCore::Position::upstream (this=0x7fff12a61660, rule=WebCore::CannotCrossEditingBoundary) at ../../third_party/WebKit/Source/WebCore/dom/Position.cpp:675 #3 0x00000000023c8e2b in WebCore::VisiblePosition::canonicalPosition (this=0x7fff12a61770, passedPosition=...) at ../../third_party/WebKit/Source/WebCore/editing/VisiblePosition.cpp:523 #4 0x00000000023c6efb in WebCore::VisiblePosition::init (this=0x7fff12a61770, position=..., affinity=WebCore::DOWNSTREAM) at ../../third_party/WebKit/Source/WebCore/editing/VisiblePosition.cpp:58 #5 0x00000000023c6ec1 in VisiblePosition (this=0x7fff12a61770, pos=..., affinity=WebCore::DOWNSTREAM) at ../../third_party/WebKit/Source/WebCore/editing/VisiblePosition.cpp:51 #6 0x000000000239724e in WebCore::FrameSelection::localCaretRect (this=0x7f1213e1b7d0) at ../../third_party/WebKit/Source/WebCore/editing/FrameSelection.cpp:1251 #7 0x00000000023975f6 in WebCore::FrameSelection::recomputeCaretRect (this=0x7f1213e1b7d0) at ../../third_party/WebKit/Source/WebCore/editing/FrameSelection.cpp:1304 #8 0x0000000002399664 in WebCore::FrameSelection::updateAppearance (this=0x7f1213e1b7d0) at ../../third_party/WebKit/Source/WebCore/editing/FrameSelection.cpp:1701 #9 0x00000000025919e2 in WebCore::FrameView::performPostLayoutTasks (this=0x7f121376f380) at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:2435 #10 0x000000000258d099 in WebCore::FrameView::layout (this=0x7f121376f380, allowSubtree=true) at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:1235 #11 0x0000000002cdb484 in WebCore::Document::updateLayout (this=0x7f12134ad000) at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:1928 #12 0x0000000002cdb555 in WebCore::Document::updateLayoutIgnorePendingStylesheets (this=0x7f12134ad000) at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:1960 #13 0x00000000023c8e03 in WebCore::VisiblePosition::canonicalPosition (this=0x7fff12a61e20, passedPosition=...) at ../../third_party/WebKit/Source/WebCore/editing/VisiblePosition.cpp:519 #14 0x00000000023c6efb in WebCore::VisiblePosition::init (this=0x7fff12a61e20, position=..., affinity=WebCore::DOWNSTREAM) at ../../third_party/WebKit/Source/WebCore/editing/VisiblePosition.cpp:58 #15 0x00000000023c6ec1 in VisiblePosition (this=0x7fff12a61e20, pos=..., affinity=WebCore::DOWNSTREAM) at ../../third_party/WebKit/Source/WebCore/editing/VisiblePosition.cpp:51 #16 0x00000000039ba2d7 in WebCore::DOMSelection::collapse (this=0x7f1215099b90, node=0x7f1213a60380, offset=0, ec=@0x7fff12a61f0c) at ../../third_party/WebKit/Source/WebCore/page/DOMSelection.cpp:210 #17 0x00000000032c6a80 in collapseCallback (args=...) at gen/webcore/bindings/V8DOMSelection.cpp:131 #18 0x000000000149c3ea in HandleApiCallHelper<false> (args=..., isolate=0x7f1223c42000) at ../../v8/src/builtins.cc:1146 #19 0x0000000001497049 in Builtin_Impl_HandleApiCall (args=..., isolate=0x7f1223c42000) at ../../v8/src/builtins.cc:1164 #20 0x000000000149701a in Builtin_HandleApiCall (args=..., isolate=0x7f1223c42000) at ../../v8/src/builtins.cc:1163 729831 5 leviw 2012-09-27 13:51:03 -0700 Some more info: the renderers are *not* dirty. Only the lines are. 729857 6 leviw 2012-09-27 14:20:42 -0700 Okay... I've got the root cause. It's an optimization in RenderText. RenderText::setTextWithOffset gets called by editing code to replace the line you were on when you hit enter with a copy of the same text. We dirty the lines in setTextWithOffset, then call RenderText::setText, which should mark us as needing layout. Instead, we check to see if the new text and the current text are equal, and if they are, we return early to avoid layout and pref width recalc. 729923 7 166072 leviw 2012-09-27 15:33:19 -0700 Created attachment 166072 Patch 729943 8 166072 rniwa 2012-09-27 15:46:07 -0700 Comment on attachment 166072 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166072&action=review > Source/WebCore/rendering/RenderText.cpp:1331 > + setText(text, force || dirtiedLines); I talked with Levi in person but there are two options here. One is to force re-layout like we're doing in this patch. Another option is to add equal() check in setTextWithOffset here as well. The only problem is that we'll then be calling equal() twice when setTextWithOffset is called with unequal text. 729951 9 leviw 2012-09-27 15:50:11 -0700 (In reply to comment #8) > (From update of attachment 166072 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166072&action=review > > > Source/WebCore/rendering/RenderText.cpp:1331 > > + setText(text, force || dirtiedLines); > > I talked with Levi in person but there are two options here. One is to force re-layout like we're doing in this patch. Another option is to add equal() check in setTextWithOffset here as well. The only problem is that we'll then be calling equal() twice when setTextWithOffset is called with unequal text. Actually, we just need to combine the two. If we bail early in setTextWithOffset we're fine, and if we don't, we can call with force to skip the comparison :) I'll re-upload shortly. 729952 10 166083 leviw 2012-09-27 15:54:31 -0700 Created attachment 166083 Patch 729962 11 166083 leviw 2012-09-27 16:00:24 -0700 Comment on attachment 166083 Patch Thanks for the extremely rapid turnaround, rniwa! 729979 12 166083 webkit.review.bot 2012-09-27 16:24:50 -0700 Comment on attachment 166083 Patch Clearing flags on attachment: 166083 Committed r129814: <http://trac.webkit.org/changeset/129814> 729980 13 webkit.review.bot 2012-09-27 16:24:55 -0700 All reviewed patches have been landed. Closing bug. 166072 2012-09-27 15:33:19 -0700 2012-09-27 15:54:28 -0700 Patch bug-97763-20120927153237.patch text/plain 1677 leviw SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyOTgwNikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEyLTA5LTI3ICBMZXZpIFdl aW50cmF1YiAgPGxldml3QGNocm9taXVtLm9yZz4KKworICAgICAgICBSRUdSRVNTSU9OKHIxMjkx ODYpOiBQcmVzc2luZyBlbnRlciBhdCB0aGUgZW5kIG9mIGEgbGluZSBkZWxldGVzIHRoZSBsaW5l CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05Nzc2Mwor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIHIxMjkxODYg ZXhwb3NlZCBpbmNvcnJlY3QgYmVoYXZpb3IgaW4gUmVuZGVyVGV4dCB3aGVyZWJ5IFJlbmRlclRl eHQncyBsaW5lcyB3ZXJlCisgICAgICAgIGRpcnRpZWQgYnV0IHRoZSByZW5kZXJlciB3YXNuJ3Qg bWFya2VkIGZvciBsYXlvdXQuIFJpY2ggdGV4dCBlZGl0aW5nIGluIEdNYWlsIGV4cG9zZWQKKyAg ICAgICAgdGhpcyBiZWhhdmlvci4gUmVuZGVyVGV4dDo6c2V0VGV4dFdpdGhPZmZzZXQgaXMgY2Fs bGVkIHdpdGggYSB0ZXh0IHN0cmluZyBpZGVudGljYWwKKyAgICAgICAgdG8gdGhlIGN1cnJlbnQg dGV4dC4gSXQgc3RpbGwgZGlydGllcyBsaW5lcywgdGhlbiBjYWxscyBzZXRUZXh0LCB3aGljaCBo YXMgYSBjaGVjaworICAgICAgICBmb3IgdGhlIGNhc2Ugd2hlbiB0aGUgc3RyaW5ncyBhcmUgdGhl IHNhbWUgYW5kIHJldHVybnMgZWFybHkgYW5kIGRvZXNuJ3QgbWFyayB1cyBhcworICAgICAgICBu ZWVkaW5nIGxheW91dC4KKworICAgICAgICBUaGlzIGNoYW5nZSBmb3JjZXMgc2V0VGV4dCB0byB3 b3JrIGl0cyBtYWdpYyB3aGVuZXZlciB3ZSBkaXJ0eSBsaW5lcyBpbiBzZXRUZXh0V2l0aE9mZnNl dC4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJUZXh0LmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OlJlbmRlclRleHQ6OnNldFRleHRXaXRoT2Zmc2V0KToKKwogMjAxMi0wOS0yNyAgSm9zaHVh IEJlbGwgIDxqc2JlbGxAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIEluZGV4ZWREQjogT3B0aW1p emUgZW5jb2RlU3RyaW5nL2RlY29kZVN0cmluZwpJbmRleDogU291cmNlL1dlYkNvcmUvcmVuZGVy aW5nL1JlbmRlclRleHQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmlu Zy9SZW5kZXJUZXh0LmNwcAkocmV2aXNpb24gMTI5Nzg2KQorKysgU291cmNlL1dlYkNvcmUvcmVu ZGVyaW5nL1JlbmRlclRleHQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMzI4LDcgKzEzMjgsNyBA QCB2b2lkIFJlbmRlclRleHQ6OnNldFRleHRXaXRoT2Zmc2V0KFBhc3NSCiAgICAgfQogCiAgICAg bV9saW5lc0RpcnR5ID0gZGlydGllZExpbmVzOwotICAgIHNldFRleHQodGV4dCwgZm9yY2UpOwor ICAgIHNldFRleHQodGV4dCwgZm9yY2UgfHwgZGlydGllZExpbmVzKTsKIH0KIAogdm9pZCBSZW5k ZXJUZXh0Ojp0cmFuc2Zvcm1UZXh0KCkK 166083 2012-09-27 15:54:31 -0700 2012-09-27 16:24:50 -0700 Patch bug-97763-20120927155350.patch text/plain 2165 leviw SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyOTgwNikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBACisyMDEyLTA5LTI3ICBMZXZpIFdl aW50cmF1YiAgPGxldml3QGNocm9taXVtLm9yZz4KKworICAgICAgICBSRUdSRVNTSU9OKHIxMjkx ODYpOiBQcmVzc2luZyBlbnRlciBhdCB0aGUgZW5kIG9mIGEgbGluZSBkZWxldGVzIHRoZSBsaW5l CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05Nzc2Mwor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIHIxMjkxODYg ZXhwb3NlZCBpbmNvcnJlY3QgYmVoYXZpb3IgaW4gUmVuZGVyVGV4dCB3aGVyZWJ5IFJlbmRlclRl eHQncyBsaW5lcyB3ZXJlCisgICAgICAgIGRpcnRpZWQgYnV0IHRoZSByZW5kZXJlciB3YXNuJ3Qg bWFya2VkIGZvciBsYXlvdXQuIFJpY2ggdGV4dCBlZGl0aW5nIGluIEdNYWlsIGV4cG9zZWQKKyAg ICAgICAgdGhpcyBiZWhhdmlvci4gUmVuZGVyVGV4dDo6c2V0VGV4dFdpdGhPZmZzZXQgaXMgY2Fs bGVkIHdpdGggYSB0ZXh0IHN0cmluZyBpZGVudGljYWwKKyAgICAgICAgdG8gdGhlIGN1cnJlbnQg dGV4dC4gSXQgc3RpbGwgZGlydGllcyBsaW5lcywgdGhlbiBjYWxscyBzZXRUZXh0LCB3aGljaCBo YXMgYSBjaGVjaworICAgICAgICBmb3IgdGhlIGNhc2Ugd2hlbiB0aGUgc3RyaW5ncyBhcmUgdGhl IHNhbWUgYW5kIHJldHVybnMgZWFybHkgYW5kIGRvZXNuJ3QgbWFyayB1cyBhcworICAgICAgICBu ZWVkaW5nIGxheW91dC4KKworICAgICAgICBUaGlzIGNoYW5nZSBhZGRzIHRoZSBzYW1lIGVhcmx5 IGJhaWxpbmcgbG9naWMgaW4gc2V0VGV4dCB0byBzZXRUZXh0V2l0aE9mZnNldCwgYnV0CisgICAg ICAgIGZvcmNlcyBzZXRUZXh0IHRvIHdvcmsgaXRzIG1hZ2ljIHdoZW5ldmVyIHdlIGRpcnR5IGxp bmVzIHRoZXJlIChhbmQgYXZvaWQgZG91YmxlLQorICAgICAgICBjaGVja2luZyB0aGF0IHRoZSBz dHJpbmdzIGFyZSBlcXVhbCkuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyVGV4dC5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJUZXh0OjpzZXRUZXh0V2l0aE9mZnNldCk6CisKIDIw MTItMDktMjcgIEpvc2h1YSBCZWxsICA8anNiZWxsQGNocm9taXVtLm9yZz4KIAogICAgICAgICBJ bmRleGVkREI6IE9wdGltaXplIGVuY29kZVN0cmluZy9kZWNvZGVTdHJpbmcKSW5kZXg6IFNvdXJj ZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUZXh0LmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGV4dC5jcHAJKHJldmlzaW9uIDEyOTc4NikKKysrIFNv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUZXh0LmNwcAkod29ya2luZyBjb3B5KQpAQCAt MTI1Niw2ICsxMjU2LDkgQEAgdm9pZCBSZW5kZXJUZXh0OjpzZXRTZWxlY3Rpb25TdGF0ZShTZWxl YwogCiB2b2lkIFJlbmRlclRleHQ6OnNldFRleHRXaXRoT2Zmc2V0KFBhc3NSZWZQdHI8U3RyaW5n SW1wbD4gdGV4dCwgdW5zaWduZWQgb2Zmc2V0LCB1bnNpZ25lZCBsZW4sIGJvb2wgZm9yY2UpCiB7 CisgICAgaWYgKCFmb3JjZSAmJiBlcXVhbChtX3RleHQuaW1wbCgpLCB0ZXh0LmdldCgpKSkKKyAg ICAgICAgcmV0dXJuOworCiAgICAgdW5zaWduZWQgb2xkTGVuID0gdGV4dExlbmd0aCgpOwogICAg IHVuc2lnbmVkIG5ld0xlbiA9IHRleHQtPmxlbmd0aCgpOwogICAgIGludCBkZWx0YSA9IG5ld0xl biAtIG9sZExlbjsKQEAgLTEzMjgsNyArMTMzMSw3IEBAIHZvaWQgUmVuZGVyVGV4dDo6c2V0VGV4 dFdpdGhPZmZzZXQoUGFzc1IKICAgICB9CiAKICAgICBtX2xpbmVzRGlydHkgPSBkaXJ0aWVkTGlu ZXM7Ci0gICAgc2V0VGV4dCh0ZXh0LCBmb3JjZSk7CisgICAgc2V0VGV4dCh0ZXh0LCBmb3JjZSB8 fCBkaXJ0aWVkTGluZXMpOwogfQogCiB2b2lkIFJlbmRlclRleHQ6OnRyYW5zZm9ybVRleHQoKQo=