Opened 14 years ago

Closed 13 years ago

#5626 closed Bug (fixed)

CKeditor 3.2.1 : html content attached makes ckeditor crash the browser FF/IE

Reported by: Alex Medina Owned by: Paweł Horzela
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version: 3.2.1
Keywords: Cc: alex@…

Description

Tested in Ckeditor 3.2.1 demo site. I tested mostly with FF but it was also reported with IE.

The content in the file attached causes FF and IE to utilize tons of memory and the browser reports a script not responding alert error ("unresponsive script" in FF). If you click on "Continue" in the browser alert window, the browser crashes.

The attached small_html_ckeditor_bug.html web page causes this error with very little html code.

Notes/observations:

  • If you only remove the the center tags, the problem goes away.
  • If you only remove the last li tags, the problem goes away.
  • if you only remove the inner table, the problem goes away.

Attachments (4)

small_html_ckeditor_bug.html (1.2 KB) - added by Alex Medina 14 years ago.
5626.patch (1.1 KB) - added by Garry Yao 14 years ago.
5626_ref.patch (750 bytes) - added by Paweł Horzela 13 years ago.
Patch
5626_2.patch (543 bytes) - added by Paweł Horzela 13 years ago.
Patch

Download all attachments as: .zip

Change History (20)

Changed 14 years ago by Alex Medina

comment:1 Changed 14 years ago by Garry Yao

Keywords: Confirmed added
Milestone: CKEditor 3.4
Priority: HighNormal

Reproduced in fullpage mode.

Changed 14 years ago by Garry Yao

Attachment: 5626.patch added

comment:2 Changed 14 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Ticket Test added:
run OR view source.

comment:3 Changed 14 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4CKEditor 3.5

I think the idea behind the patch, as well as the tests are wrong. Leaving those <li>s as is could give unexpected results on the editor operations.

comment:4 Changed 14 years ago by Tobiasz Cudnik

Owner: Garry Yao deleted
Status: assignednew

comment:5 Changed 14 years ago by Tobiasz Cudnik

Keywords: Confirmed removed
Status: newconfirmed

comment:6 Changed 14 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

comment:7 Changed 14 years ago by Tobiasz Cudnik

Status: assignedreview

After deeper checking, i think that Garry's solution is fine, as all LIs are still fixed by a code executed before the new one.

I'm opting for another review.

comment:8 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

I tend to agree with Fred. We should opt to fix this code (gather this items in lists etc.) and not leaving it as is. For example now we can not use the list properties dialog on the list items.

comment:9 Changed 13 years ago by Paweł Horzela

If there is <LI> inside <TD> it's not allowed in DTD. We create new element <UL> between <TD> and <LI> to have <TD><UL><LI>.

Changed 13 years ago by Paweł Horzela

Attachment: 5626_ref.patch added

Patch

comment:10 Changed 13 years ago by Paweł Horzela

Owner: changed from Tobiasz Cudnik to Paweł Horzela
Status: review_failedreview

comment:11 Changed 13 years ago by Garry Yao

Status: reviewreview_failed

The idea is good, but why you're moving the children? Test case:

<div>
	text
	<li>item</li>
</div>

produces

<div>
	<ul>
		text
		<li>
			item</li>
	</ul>
</div>

comment:12 Changed 13 years ago by Garry Yao

Also, we must also not add the 'ul' directly, the <ul> might not fit in position as well, it should be something like:

parser.onTagOpen( 'ul', {} );

Changed 13 years ago by Paweł Horzela

Attachment: 5626_2.patch added

Patch

comment:13 Changed 13 years ago by Paweł Horzela

Status: review_failedreview

comment:14 Changed 13 years ago by Garry Yao

Status: reviewreview_passed

comment:15 Changed 13 years ago by Garry Yao

Ticket test updated with [5989].

comment:16 Changed 13 years ago by Paweł Horzela

Resolution: fixed
Status: review_passedclosed

Fixed with [5990].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy