Bug 97763

Summary: REGRESSION(r129186): Pressing enter at the end of a line deletes the line
Product: WebKit Reporter: jochen
Component: HTML EditingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, enrica, eric, inferno, leviw, rniwa, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description jochen 2012-09-27 00:31:48 PDT
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
Comment 1 Abhishek Arya 2012-09-27 04:12:58 PDT
Check out comment in chromium bug and please add a link here.
Comment 2 jochen 2012-09-27 04:23:07 PDT
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
Comment 3 Levi Weintraub 2012-09-27 11:30:23 PDT
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.
Comment 4 Levi Weintraub 2012-09-27 13:25:22 PDT
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
Comment 5 Levi Weintraub 2012-09-27 13:51:03 PDT
Some more info: the renderers are *not* dirty. Only the lines are.
Comment 6 Levi Weintraub 2012-09-27 14:20:42 PDT
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.
Comment 7 Levi Weintraub 2012-09-27 15:33:19 PDT
Created attachment 166072 [details]
Patch
Comment 8 Ryosuke Niwa 2012-09-27 15:46:07 PDT
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.
Comment 9 Levi Weintraub 2012-09-27 15:50:11 PDT
(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.
Comment 10 Levi Weintraub 2012-09-27 15:54:31 PDT
Created attachment 166083 [details]
Patch
Comment 11 Levi Weintraub 2012-09-27 16:00:24 PDT
Comment on attachment 166083 [details]
Patch

Thanks for the extremely rapid turnaround, rniwa!
Comment 12 WebKit Review Bot 2012-09-27 16:24:50 PDT
Comment on attachment 166083 [details]
Patch

Clearing flags on attachment: 166083

Committed r129814: <http://trac.webkit.org/changeset/129814>
Comment 13 WebKit Review Bot 2012-09-27 16:24:55 PDT
All reviewed patches have been landed.  Closing bug.