Bug 93247

Summary: REGRESSION(r124739): fast/lists/list-marker-remove-crash.html hits an assertion in MoveParagraphs
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: HTML EditingAssignee: Andrei Bucur <abucur>
Status: RESOLVED FIXED    
Severity: Critical CC: abucur, dstockwell, enrica, gyuyoung.kim, inferno, mifenton, ossy, rakuco, rniwa, simon.fraser, WebkitBugTracker, webkit.review.bot, zan
Priority: P1 Keywords: AdobeTracked, Gtk, InRadar, LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 79668    
Attachments:
Description Flags
Patch none

Description Csaba Osztrogonác 2012-08-06 03:23:55 PDT
https://bugs.webkit.org/show_bug.cgi?id=90476 is a security bug,
and the new test introduced in it asserts on all platforms.

on Qt:
STDERR: ASSERTION FAILED: isEndOfParagraph(endOfParagraphToMove)
STDERR: /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebCore/editing/CompositeEditCommand.cpp(1137) : void WebCore::CompositeEditCommand::moveParagraph(const WebCore::VisiblePosition&, const WebCore::VisiblePosition&, const WebCore::VisiblePosition&, bool, bool)

on Lion:
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20%28Tests%29/r124741%20%281606%29/fast/lists/list-marker-remove-crash-crash-log.txt

on GTK:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r124743%20%2835379%29/fast/lists/list-marker-remove-crash-crash-log.txt
Comment 1 Csaba Osztrogonác 2012-08-06 03:29:01 PDT
The patch was rolled out - https://bugs.webkit.org/show_bug.cgi?id=93241

*** This bug has been marked as a duplicate of bug 93241 ***
Comment 2 Abhishek Arya 2012-08-06 09:08:13 PDT
The security fix has no relation to this editing bug. http://trac.webkit.org/changeset/124739 just tries to prevent an empty anonymous block continuation from cleaning up early [and cleanup code didn't exist before https://trac.webkit.org/changeset/120477]
Comment 3 Abhishek Arya 2012-08-06 10:48:24 PDT
This bug will track the editing bug. security bug went in http://trac.webkit.org/changeset/124783
Comment 4 Csaba Osztrogonác 2012-08-07 08:59:16 PDT
I fixed the expectations, because assertions are valid in debug mode only - https://trac.webkit.org/changeset/124890
Comment 5 Ryosuke Niwa 2012-08-08 14:27:59 PDT
I cannot reproduce this assertion failure on my MacPro with Snow Leopard at r125081.
Comment 7 Ryosuke Niwa 2012-08-08 22:37:25 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > I cannot reproduce this assertion failure on my MacPro with Snow Leopard at r125081.
> 
> Did you try to reproduce it in debug mode? :)

Yes! I'm gonna try Lion now :)
Comment 8 Ryosuke Niwa 2012-08-09 01:39:00 PDT
Yeah, I can reproduce the assertion failure on Lion. Unfortunately, this is one of those impenetrable InsertListCommand bugs.
Comment 9 Simon Fraser (smfr) 2012-09-21 14:15:53 PDT
Crashes for me every time I run tests.
Comment 10 Ryosuke Niwa 2012-11-01 10:12:52 PDT
<rdar://problem/12604537>
Comment 11 Csaba Osztrogonác 2012-11-21 01:42:32 PST
The bug is still valid.
Comment 12 Andrei Bucur 2013-02-26 04:41:43 PST
It seems we don't update the end position after updating start in InsertListCommand::listifyParagraph.
I'm really not familiar with that code, but common sense dictates we should do that (if start changed, why shouldn't end change as well?). Something around these lines:

        // We inserted the list at the start of the content we're about to move
        // Update the start of content, so we don't try to move the list into itself.  bug 19066
        // Layout is necessary since start's node's inline renderers may have been destroyed by the insertion
        if (insertionPos == start.deepEquivalent()) {
            listElement->document()->updateLayoutIgnorePendingStylesheets();
            start = startOfParagraph(originalStart, CanSkipOverEditingBoundary);
+            end = endOfParagraph(start, CanSkipOverEditingBoundary);
        }

After adding that new line the test doesn't crash any more. Is this the right fix?
Comment 13 Andrei Bucur 2013-02-27 05:57:19 PST
Created attachment 190498 [details]
Patch
Comment 14 WebKit Review Bot 2013-02-27 10:45:51 PST
Comment on attachment 190498 [details]
Patch

Clearing flags on attachment: 190498

Committed r144213: <http://trac.webkit.org/changeset/144213>
Comment 15 WebKit Review Bot 2013-02-27 10:45:57 PST
All reviewed patches have been landed.  Closing bug.