Opened 11 years ago

Closed 11 years ago

#3195 closed Bug (fixed)

HTML Parser: Out of memory

Reported by: Frederico Caldeira Knabben Owned by: Frederico Caldeira Knabben
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Review? Cc:

Description

During some tests with our so loved and weird IE, I was able to, somehow, end up with an innerHTML that I was able to reduce to the following:

<TABLE>
	<TR>
		<TD></TD>
		<P></P>
		<TD></TD>
	</TR>
</TABLE>

I can't recall how I got that HTML (maybe during some block styling task), but the fact is that a user may have such thing while using the editor in IE. The browser simply accepts it.

The big problem is not the above though. The real issue is the, if you try to retrieve the above HTML from the editor, or even feed it into the source view, it will throw you "Out of memory" into the html parser code.

Attachments (5)

3195.patch (8.9 KB) - added by Frederico Caldeira Knabben 11 years ago.
3195_2.patch (10.0 KB) - added by Garry Yao 11 years ago.
3195_3.patch (10.2 KB) - added by Frederico Caldeira Knabben 11 years ago.
3195_4.patch (10.4 KB) - added by Frederico Caldeira Knabben 11 years ago.
3195_5.patch (11.3 KB) - added by Garry Yao 11 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: newassigned

Changed 11 years ago by Frederico Caldeira Knabben

Attachment: 3195.patch added

comment:2 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review? added

While coding a fix for this one, I've started writing some tests, to ensure it was not breaking anything. At the end, I found out that there were several cases not passing. I was not anymore able to understand if that was caused by my changes, so I've started fixing all of them, changing the way the parser works a bit. It should be much better now.

comment:3 Changed 11 years ago by Garry Yao

The patch is good and passed almost all the regressions,but it failed on some of my un-commited TC and I've provided an appending patch to fix them with the failed TCs, please review it also.

Changed 11 years ago by Garry Yao

Attachment: 3195_2.patch added

comment:4 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

garry.yao's proposal fix the added tc, but it somehow duplicates the code we have in the "else if" block in that function. Also, it misses cases like <b><i></b></i>.

Also, the forceSimpleAmpersand is a feature of the htmlwriter plugin, not available in the basicWriter, which is well... basic.

I'm coming with a new ticket now.

Changed 11 years ago by Frederico Caldeira Knabben

Attachment: 3195_3.patch added

comment:5 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

comment:6 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

While debugging around #3091, I've found out that this new patch still misses a specific case. I'll come with yet another patch for it.

Changed 11 years ago by Frederico Caldeira Knabben

Attachment: 3195_4.patch added

comment:7 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

Changed 11 years ago by Garry Yao

Attachment: 3195_5.patch added

comment:8 Changed 11 years ago by Garry Yao

R+ for 3195_4.patch, and I'm again appending a specific new TC and fixing them with the patch based on the reviewed patch.

comment:9 Changed 11 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [3314].

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