Loading this content into CKEditor crashes any browser

[TomNM]

The following code crashes any browser (including Chrome) when pasted in source mode and then attempting to switch to wysiwyg. I confirmed it will do the same in the ckeditor 4 nightly build.

<span style="font-size: 14pt;"><span style="font-size: 14pt;">I welcome you to Mount Soledad's Children's Ministry! Our Ministry is an exciting and fun place for kids Preschool- 5th grade to come and&nbsp;learn about God and learn how they can put the Bible into practice in their own lives. Every Sunday children get to experience God on their level through interactive worship, dramatic story telling&nbsp;and&nbsp;creative hands-on projects. We want to invite you to our Kids Club Sunday mornings at 10am.&nbsp;<br />
<br />
Our vision here at Mount Soledad is to help children love Jesus, grow in their knowledge of the Bible, learn to praise God through worship, and cultivate a servant attitude to others in need. We help parents be their child's best teacher and example of Christian faith.&nbsp;<br />
<br />
With my passion for children and their families, it is a gift and joy to serve you and your children. Please let me know how we can serve YOU!<br />
<br />
Sarah Johnson, Your Children's Director<br />
</span></span>sarahjohnson@mountsoledad.org<br />
<br />
<dt style="text-align: center;"><span style="font-size: 12pt;"><span style="font-family: Arial;"> </span></span><span style="font-size: 11pt;"><font size="5" face="Impact">
<p align="center"><span style="font-family: Arial;"><br />
</span><br />
&nbsp;</p>
<p>&nbsp;</p>
<p>&nbsp;</p>
</font></span></dt>

What can be done about something like this without requiring the user to act differently?

Thanks, Tom

[Piotrek Koszuliński]

On v4 IE8 stopped execution in fragment.js:285.

This issue is reproducible on v3 as well.

[Piotrek Koszuliński]

Simplified TC:

<dt style="text-align: center;">
<p align="center"></p>
</dt>

[Jakub Ś]

@Reinmar your simplified TC is not the same TC as original one.
Original TC can be reproduced from CKE 3.4.2. while yours from CKEditor 3.5.3.

[Jakub Ś]

Of course the above code is invalid but it should not break CKEditor.

Some notes about definition lists:

1. DL can contain DT and DD, ​http://www.w3.org/TR/html401/struct/lists.html#h-10.3. In XHTML1.1/ HTML4 you don't need to have both dt and dd so only dd or dt inside dl is enough.
2. In HTML5 you need to have both dt and dd but dt can be empty (​http://www.w3.org/TR/html-markup/dl.html).
3. DT in HTML4 it can contain only inline elements, In html5 text and flow-content (It is like inline and block in HTML4) ​http://www.w3.org/TR/xhtml-modularization/abstract_modules.html#s_listmodule

[TomNM]

This was code that a client of ours was somehow able to build in the editor, from what we understand.

[Piotrek Koszuliński]

Replying to TomNM:

This was code that a client of ours was somehow able to build in the editor, from what we understand.

It definitely wasn't build in the editor - CKEditor doesn't produce this kind of invalid HTML. No chance for that. It looks like copy&pasted from Word or something similar.

Anyway - I've pushed t/9557 with patch that fixes this issue. However, as I don't know enough about htmlParser it's rather to show where's the issue, not to fix it. There's an infinitive loop inside fromHtml (see while loop at the end of it), it executes addElement on element which already was added and since in this case currentNode isn't updated execution stuck there.

[Garry Yao]

The patch is ok, but there's another problem revealed by the TC that ​DT element should have a flow content model.

Pushed the DTD fix onto the patch, opened t/9557 on test as well.

[Garry Yao]

Moved it to 4.0 because of the impact from the DTD problem.

[Piotrek Koszuliński]

There's inf loop on IE8 on newly added test.

I had to force push t/9557@tests, because it wasn't based on master.

[Piotrek Koszuliński]

Sorry, it was me testing on master, not on t/9557. It's ofc ok.

BTW. I think that DT's permitted content could change since we've created our DTD - I'm pretty sure I remember that there was a difference between DT and DD.

[Garry Yao]

There's an IE7 TC failure on the newly added one though.

[Garry Yao]

It's a regression from [6978], one fix is committed on t/9557 to address it, with a ticket test of #6975 compensated as well.

[Piotrek Koszuliński]

Failed test: test_6975
Message: Values should be the same. unit.js:402
Expected: <dl><dt>foo</dt><dd>bar</dd><dt>baz</dt><dd>quz</dd></dl>
Actual: <dl><dt>foo<dl><dd>bar<dl><dt>baz<dl><dd>quz</dd></dl></dt></dl></dd></dl></dt></dl>

On Chrome and IE9 on (dev=d343491)&(tests=35ce5c8)

[Garry Yao]

Ok, I proposed another browser-independent way to re-fix #6975, which should pass the tests consistently.

[Garry Yao]

Fixed on master with:

​https://github.com/ckeditor/ckeditor-dev/commit/bf84cd54c69f6069ec69c34750448fb12c7e406a