Opened 15 years ago

Closed 15 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 15 years ago.
3195_2.patch (10.0 KB) - added by Garry Yao 15 years ago.
3195_3.patch (10.2 KB) - added by Frederico Caldeira Knabben 15 years ago.
3195_4.patch (10.4 KB) - added by Frederico Caldeira Knabben 15 years ago.
3195_5.patch (11.3 KB) - added by Garry Yao 15 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 15 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: newassigned

Changed 15 years ago by Frederico Caldeira Knabben

Attachment: 3195.patch added

comment:2 Changed 15 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 15 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 15 years ago by Garry Yao

Attachment: 3195_2.patch added

comment:4 Changed 15 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 15 years ago by Frederico Caldeira Knabben

Attachment: 3195_3.patch added

comment:5 Changed 15 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

comment:6 Changed 15 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 15 years ago by Frederico Caldeira Knabben

Attachment: 3195_4.patch added

comment:7 Changed 15 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

Changed 15 years ago by Garry Yao

Attachment: 3195_5.patch added

comment:8 Changed 15 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 15 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [3314].

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