#9557 closed Bug (fixed)
Loading this content into CKEditor crashes any browser
Reported by: | TomNM | Owned by: | Garry Yao |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.0 |
Component: | Core : Parser | Version: | 3.4.2 |
Keywords: | Cc: |
Description
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 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 and creative hands-on projects. We want to invite you to our Kids Club Sunday mornings at 10am. <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. <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 /> </p> <p> </p> <p> </p> </font></span></dt>
What can be done about something like this without requiring the user to act differently?
Thanks, Tom
Change History (17)
comment:1 Changed 12 years ago by
Component: | Performance → Core : Parser |
---|---|
Status: | new → confirmed |
Version: | 4.0 Beta → 3.6.6 (SVN - trunk) |
comment:2 Changed 12 years ago by
Summary: | This content crashes any browser in ckeditor 4 nightly build... → Loading this content into CKEditor crashes any browser |
---|
comment:3 Changed 12 years ago by
Simplified TC:
<dt style="text-align: center;"> <p align="center"></p> </dt>
comment:4 Changed 12 years ago by
Version: | 3.6.6 (SVN - trunk) → 3.4.2 |
---|
@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.
comment:5 Changed 12 years ago by
Of course the above code is invalid but it should not break CKEditor.
Some notes about definition lists:
- 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.
- In HTML5 you need to have both dt and dd but dt can be empty (http://www.w3.org/TR/html-markup/dl.html).
- 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
comment:6 follow-up: 7 Changed 12 years ago by
This was code that a client of ours was somehow able to build in the editor, from what we understand.
comment:7 Changed 12 years ago by
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.
comment:8 Changed 12 years ago by
Milestone: | → CKEditor 4.0.1 |
---|
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.
comment:9 Changed 12 years ago by
Milestone: | CKEditor 4.0.1 → CKEditor 4.0 |
---|---|
Owner: | set to Garry Yao |
Status: | confirmed → review |
Moved it to 4.0 because of the impact from the DTD problem.
comment:10 Changed 12 years ago by
Status: | review → review_failed |
---|
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.
comment:11 Changed 12 years ago by
Status: | review_failed → review_passed |
---|
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.
comment:12 Changed 12 years ago by
Status: | review_passed → review_failed |
---|
There's an IE7 TC failure on the newly added one though.
comment:13 Changed 12 years ago by
Status: | review_failed → review |
---|
comment:14 Changed 12 years ago by
Status: | review → review_failed |
---|
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)
comment:15 Changed 12 years ago by
Status: | review_failed → review |
---|
Ok, I proposed another browser-independent way to re-fix #6975, which should pass the tests consistently.
comment:16 Changed 12 years ago by
Status: | review → review_passed |
---|
comment:17 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
On v4 IE8 stopped execution in fragment.js:285.
This issue is reproducible on v3 as well.