Bug 97763 – REGRESSION(r129186): Pressing enter at the end of a line deletes the line

• Component: HTML Editing
• Status: RESOLVED FIXED

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

[Abhishek Arya]

Check out comment in chromium bug and please add a link here.

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

[Levi Weintraub]

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.

[Levi Weintraub]

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

[Levi Weintraub]

Some more info: the renderers are *not* dirty. Only the lines are.

[Levi Weintraub]

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.

[Levi Weintraub]

Created attachment 166072 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 166072 [details]
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.

[Levi Weintraub]

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

[Levi Weintraub]

Created attachment 166083 [details]
Patch

[Levi Weintraub]

Comment on attachment 166083 [details]
Patch

Thanks for the extremely rapid turnaround, rniwa!

[WebKit Review Bot]

Comment on attachment 166083 [details]
Patch

Clearing flags on attachment: 166083

Committed r129814: <http://trac.webkit.org/changeset/129814>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.