Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#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&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

Change History (17)

comment:1 Changed 7 years ago by Piotrek Koszuliński

Component: PerformanceCore : Parser
Status: newconfirmed
Version: 4.0 Beta3.6.6 (SVN - trunk)

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

This issue is reproducible on v3 as well.

comment:2 Changed 7 years ago by Piotrek Koszuliński

Summary: This content crashes any browser in ckeditor 4 nightly build...Loading this content into CKEditor crashes any browser

comment:3 Changed 7 years ago by Piotrek Koszuliński

Simplified TC:

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

comment:4 Changed 7 years ago by Jakub Ś

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 7 years ago by 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
Last edited 7 years ago by Jakub Ś (previous) (diff)

comment:6 Changed 7 years ago by TomNM

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

comment:7 in reply to:  6 Changed 7 years ago by 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.

comment:8 Changed 7 years ago by Garry Yao

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 7 years ago by Garry Yao

Milestone: CKEditor 4.0.1CKEditor 4.0
Owner: set to Garry Yao
Status: confirmedreview

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

comment:10 Changed 7 years ago by Piotrek Koszuliński

Status: reviewreview_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 7 years ago by Piotrek Koszuliński

Status: review_failedreview_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 7 years ago by Garry Yao

Status: review_passedreview_failed

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

comment:13 Changed 7 years ago by Garry Yao

Status: review_failedreview

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.

comment:14 Changed 7 years ago by Piotrek Koszuliński

Status: reviewreview_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 7 years ago by Garry Yao

Status: review_failedreview

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

comment:16 Changed 7 years ago by Piotrek Koszuliński

Status: reviewreview_passed

comment:17 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy