Bug 93247 – REGRESSION(r124739): fast/lists/list-marker-remove-crash.html hits an assertion in MoveParagraphs

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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 ***

[Abhishek Arya]

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]

[Abhishek Arya]

This bug will track the editing bug. security bug went in http://trac.webkit.org/changeset/124783

[Csaba Osztrogonác]

I fixed the expectations, because assertions are valid in debug mode only - https://trac.webkit.org/changeset/124890

[Ryosuke Niwa]

I cannot reproduce this assertion failure on my MacPro with Snow Leopard at r125081.

[Csaba Osztrogonác]

(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? :)

The assertion is still valid, check the Lion, GTK and Qt _debug_ bots:
- http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20%28Tests%29/r125146%20%281718%29/fast/lists/list-marker-remove-crash-stderr.txt
- http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r125143%20%2835465%29/fast/lists/list-marker-remove-crash-stderr.txt
- http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r125143%20%2824391%29/fast/lists/list-marker-remove-crash-crash-log.txt

[Ryosuke Niwa]

(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 :)

[Ryosuke Niwa]

Yeah, I can reproduce the assertion failure on Lion. Unfortunately, this is one of those impenetrable InsertListCommand bugs.

[Simon Fraser (smfr)]

Crashes for me every time I run tests.

[Ryosuke Niwa]

<rdar://problem/12604537>

[Csaba Osztrogonác]

The bug is still valid.

[Andrei Bucur]

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?

[Andrei Bucur]

Created attachment 190498 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190498 [details]
Patch

Clearing flags on attachment: 190498

Committed r144213: <http://trac.webkit.org/changeset/144213>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.