[Webkit-unassigned] [Bug 33668] WebCore::InsertListCommand::modifyRange infinite loop (100% CPU usage)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33668





--- Comment #24 from Ryosuke Niwa <rniwa at webkit.org>  2010-08-25 15:04:45 PST ---
(In reply to comment #23)
> (From update of attachment 63104 [details])
> > +static Node* enclosingListChild(Node* node, Node* listNode)
> 
> I think it’s a little confusing that this function is just named enclosingListChild even though it takes the two arguments. A clearer name might help. You didn't add the function, so the name is not your fault.

Actually, I'm the one who added that function.  It finds the enclosing child of the specified list so I could have named it like enclosignListChildOfList but that seemed rather redundant.  But I can change the name if you have a better alternative.

> > +                // save and restore endOfSelection and startOfLastParagraph when necessary
> > +                // since moveParagraph and movePragraphWithClones can remove nodes.
> 
> The word "save" here should be capitalized.

Will fix.

> > +                // FIXME: This is very inefficient and expensive operation.
> 
> A better comment would explain why it's inefficient and why it's not easy to fix or something like that. Just saying it’s inefficient is strange. Doesn’t make clear what the tradeoff is.

Changed to: This is an inefficient way to keep selection alive because indexForVisiblePosition walks from the beginning of the document to the endOfSelection everytime this code is executed. But not using index is hard because there are so many ways we can lose selection inside doApplyForSingleParagraph.

> > +                int indexForEndOfSelection = indexForVisiblePosition(endOfSelection);
> >                  doApplyForSingleParagraph(forceCreateList, listTag, currentSelection.get());
> > +                if (endOfSelection.isNull() || endOfSelection.isOrphan() || startOfLastParagraph.isNull() || startOfLastParagraph.isOrphan()) {
> > +                    RefPtr<Range> lastSelectionRange = TextIterator::rangeFromLocationAndLength(document()->documentElement(), indexForEndOfSelection, 0, true);
> > +                    endOfSelection = lastSelectionRange->startPosition();
> > +                    startOfLastParagraph = startOfParagraph(endOfSelection);
> > +                }
> 
> This indexForEndOfSelection approach seems weak. It’s a sort of “I give up and can’t figure out any other way to make it work” approach. Not that I have anything better to suggest. As long as we have sufficient test coverage someone can come back and tackle this later I guess.

It really is "I give up and can't figure out any other way to make it work".  I tried many different ways but I can always find a test that crashes after adding 10-20 lines of code everywhere else.  Until we invent a better way of iterating through paragraphs, we should stick to this rather inefficient and desperate approach. At least, it prevents us from crashing.

> > +            // FIXME: moveParagraphWithClones (deleteSelection) does not remove the old list properly
> > +            // See bug 33668 and the test editing/execCommand/insert-list-orphaned-item-with-nested-lists.html
> 
> This comment is confusing. "Does not remove properly" doesn't tell you what it does. It says what it doesn't do. It would be better if the comment was more specific.

Updated to:
            // Manually remove listNode because moveParagraphWithClones sometimes leaves it behind in the document.
            // See the bug 33668 and editing/execCommand/insert-list-orphaned-item-with-nested-lists.html.
            // FIXME: This might be a bug in moveParagraphWithClones or deleteSelection.

> It seems impossible for one test to cover all the new code paths here. I'm surprised you added only one test.

Fortunately, there are many existing tests that uses new code paths so all new code paths are covered.  e.g. removing any line of new code will result in either crash or failure of some tests.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.