Bug 93247 - REGRESSION(r124739): fast/lists/list-marker-remove-crash.html hits an assertion in MoveParagraphs
Summary: REGRESSION(r124739): fast/lists/list-marker-remove-crash.html hits an asserti...
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: Andrei Bucur
Keywords: AdobeTracked, Gtk, InRadar, LayoutTestFailure
Depends on:
Blocks: 79668
  Show dependency treegraph
Reported: 2012-08-06 03:23 PDT by Csaba Osztrogonác
Modified: 2013-02-27 10:45 PST (History)
13 users (show)

See Also:

Patch (8.31 KB, patch)
2013-02-27 05:57 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:

on GTK:
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
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()) {
            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]
Comment 14 WebKit Review Bot 2013-02-27 10:45:51 PST
Comment on attachment 190498 [details]

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.