Bug 97763 - REGRESSION(r129186): Pressing enter at the end of a line deletes the line
: REGRESSION(r129186): Pressing enter at the end of a line deletes the line
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-09-27 00:31 PST by
Modified: 2012-09-27 16:24 PST (History)


Attachments
Patch (1.64 KB, patch)
2012-09-27 15:33 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch (2.11 KB, patch)
2012-09-27 15:54 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-09-27 00:31:48 PST
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 From 2012-09-27 04:12:58 PST -------
Check out comment in chromium bug and please add a link here.
------- Comment #2 From 2012-09-27 04:23:07 PST -------
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 From 2012-09-27 11:30:23 PST -------
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 From 2012-09-27 13:25:22 PST -------
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 From 2012-09-27 13:51:03 PST -------
Some more info: the renderers are *not* dirty. Only the lines are.
------- Comment #6 From 2012-09-27 14:20:42 PST -------
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 From 2012-09-27 15:33:19 PST -------
Created an attachment (id=166072) [details]
Patch
------- Comment #8 From 2012-09-27 15:46:07 PST -------
(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.
------- Comment #9 From 2012-09-27 15:50:11 PST -------
(In reply to comment #8)
> (From update of attachment 166072 [details] [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 From 2012-09-27 15:54:31 PST -------
Created an attachment (id=166083) [details]
Patch
------- Comment #11 From 2012-09-27 16:00:24 PST -------
(From update of attachment 166083 [details])
Thanks for the extremely rapid turnaround, rniwa!
------- Comment #12 From 2012-09-27 16:24:50 PST -------
(From update of attachment 166083 [details])
Clearing flags on attachment: 166083

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