Summary: | REGRESSION(r129186): Pressing enter at the end of a line deletes the line | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||
Component: | HTML Editing | Assignee: | 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
jochen
2012-09-27 00:31:48 PDT
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 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. 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 Some more info: the renderers are *not* dirty. Only the lines are. 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. Created attachment 166072 [details]
Patch
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. (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. Created attachment 166083 [details]
Patch
Comment on attachment 166083 [details]
Patch
Thanks for the extremely rapid turnaround, rniwa!
Comment on attachment 166083 [details] Patch Clearing flags on attachment: 166083 Committed r129814: <http://trac.webkit.org/changeset/129814> All reviewed patches have been landed. Closing bug. |