Table inside of Div producing unresponsive script

Confirmed with IE and FF.

I've spent a whole day today looking at this bug and thinking of a reasonable solution.

First... an analysis of why the bug happened. The infinite loop happens in the FCKDomRangeIterator class - this class have caused quite a number of infinite loop bugs since 2.5 was released. And every time there's an infinite loop, the loop's behavior has always been the same - the iterator tries to split a block tag to two parts, one small and visibly empty part on top, and one big visibly untouched part in the bottom. The iterator then returns the top part, and keeps the untouched bottom part for the next iteration. Since the bottom part was untouched, the next iteration's logic finds itself in the same situation - need to split, return an "empty" block, and leave the same problematic block for the next iteration.

What's happening in this case... in each run of the FCKDomRangeIterator::GetNextParagraph() function...

1. A range object was initialized as a collapsed range and pointing to the beginning of the <div> block.
2. The range object was gradually expanded just before the <table> element. The scanned area inside the range is one paragraph.
3. A block split was determined to be needed by the GetNextParagraph() function since the range object doesn't include the whole block, yet GetNextParagraph() still needs to return an independent block as the next paragraph.
4. A shallow copy of the <div> block is created, lets call it newBlock.
5. The contents inside the range (i.e. the start tag of <span> node) is extracted into a document fragment.
6. Since DOM operations do not permit extracting only the start tag of a DOM node, the <span> node is instead shallow copied to the document fragment, while the original <span> in the master <div> block is untouched.
7. The document fragment was appended to newBlock.
8. newBlock was added into the document, just before the old <div> block.
9. newBlock was returned by GetNextParagraph(), while the old <div> block's reference was stored in the FCKDomRangeIterator object as the DOM node to be scanned in the next iteration.

There are two errors here:

1. Why is splitting a block needed when I'm formatting a block? Or changing the text alignment of the block? Or putting the block into a blockquote? (all three operations mentioned would hang the browser with the same test input) The block splitting operation in GetNextParagraph() makes sense for things like making lists, but not for others.
2. The very possibility of GetNextParagraph() going into infinite loop needs to be eliminated. If the next block to be scanned is just the same as the block I'm scanning, I'm guaranteed to have an infinite loop.

I've coded some logic today to try to eliminate the possibility of GetNextParagraph() of going infinite loop. It works by examining the returned contents from range.ExtractContents() in line 265 of fckdomrangeiterator.js. If the returned contents has only one editable point and that the corresponding node is empty (e.g. a stack of <span>, <b>, <i> nodes with no text inside), then we conclude that ExtractContents() hasn't removed anything visible from the master block and thus we're gonna run into an infinite loop if we proceed to split the block. The block splitting operation would be stopped at this point to prevent an infinite loop.

This stops the infinite looping in the provided test case, but it still wouldn't completely stop GetNextParagraph() from splitting the <div> block for a finite number of times. Anyway I'm attaching the infinite loop avoidance code I've got so far here to get some comments on whether the current approach is reasonable.

Patch for avoiding infinite loops in FCKDomRangeIterator. This is NOT intended to be the final fix to #1717.

Replying to martinkou:

1. Why is splitting a block needed when I'm formatting a block? Or changing the text alignment of the block? Or putting the block into a blockquote? (all three operations mentioned would hang the browser with the same test input) The block splitting operation in GetNextParagraph() makes sense for things like making lists, but not for others.

Looking at the sample source provided in this ticket, this operation may sound strange. Let me just change it a bit:

<div><span class="text">Some text
<table>
    <tbody>
        <tr>
            <td>test</td>
        </tr>
    </tbody>
</table></span></div>

Now, the split operation sounds reasonable. We must separate the "Some text" block from the table block, having the following result.

<div style="text-align: center;"><span class="text">Some text </span></div>
<div><span class="text">
<table>
    <tbody>
        <tr>
            <td style="text-align: center;">test</td>
        </tr>
    </tbody>
</table>
</span></div>

I'm proposing a patch to it. The idea is quite simple. If we find a block boundary, which stops the range expansion, potentially splitting the block, that block is now forced to be the next one to be processed when calling GetNextParagraph again, no matter what happens when splitting.

I'm not calling this one for review because there is still a chance for enhancement. The problems is that the function will still return a block containing only "<span> </span>", which has no value. So, one enhancement to it would be forcing the function to ignore such "visually empty blocks", so the final output would be just like this:

<div><span class="text">
<table>
    <tbody>
        <tr>
            <td style="text-align: center;">test</td>
        </tr>
    </tbody>
</table>
</span></div>

Martin has proposed something similar. But for that, we could introduce Range.CheckIsVisuallyEmpty(), that would work is some ways similarly to CheckStartOfBlock and CheckEndOfBlock. In that we can do the range check inside the expansion loop (at line 214). I'll work on that.

I've tried the CheckIsVisuallyEmpty idea and found out that it is totally wrong. We must not ignore empty paragraphs. For example:

<p>Some text</p>
<p></p>
<p>More text</p>

If we select all the above code and center it, we expect all paragraphs to be centered, including the empty one. So, it is wrong to ignore it.

So, I propose fixing this ticket with the patch I've attached before (attachment:1717.patch). It will fix the reported problem, as we'll have no more loops.

It is not to say that the resulting output will be nice, for the TC provided by the reporter, but it is also a fact that the TC input data is completely invalid (not producible with FCKeditor, btw), and so unexpected results are expected.

Opps... wrong keyword!

Fixed with [1511]. Click here for more info about our SVN system.