Opened 10 years ago

Closed 10 years ago

#3790 closed Bug (fixed)

Find & Replace causes "too much recursion"

Reported by: Damian Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: 3.0RC IBM Confirmed Review+ Cc:

Description

When editing a moderate sized document with many searched for items, the search & replace can cause "too much recursion" errors in FF.

Sample document attached (taken from source view).

Attachments (5)

sample.html (37.3 KB) - added by Damian 10 years ago.
3790.patch (1.8 KB) - added by Garry Yao 10 years ago.
3790_2.patch (4.9 KB) - added by Garry Yao 10 years ago.
3790_3.patch (5.7 KB) - added by Garry Yao 10 years ago.
3790_4.patch (7.7 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (16)

Changed 10 years ago by Damian

Attachment: sample.html added

comment:1 Changed 10 years ago by Garry Yao

Keywords: Confirmed added
Owner: set to Garry Yao
Status: newassigned

The known Gecko Javascript stack deepth limit is 1K, however, I notice the browser is reporting unresponsive script at around 400.
We should be able to avoid this by rewriting the recursion algorithm to stack based.

comment:2 Changed 10 years ago by Damian

The search and replace seems slower than in previous times, could this also be related to the problem?

Changed 10 years ago by Garry Yao

Attachment: 3790.patch added

comment:3 in reply to:  2 Changed 10 years ago by Garry Yao

Replying to damo:

The search and replace seems slower than in previous times, could this also be related to the problem?

Do you mean by comparing with replacing feature of v2?

comment:4 Changed 10 years ago by Garry Yao

Keywords: Review? added

The unresponsive script still show up with your sample page even with the iteration-based version, but that's been due to browser execution time limit.

comment:5 Changed 10 years ago by Garry Yao

Cutting off the highlighting feature during replacement to speed it up.

Changed 10 years ago by Garry Yao

Attachment: 3790_2.patch added

Changed 10 years ago by Garry Yao

Attachment: 3790_3.patch added

comment:6 Changed 10 years ago by Garry Yao

The new patch is take one step further, save one undo snapshot for all the replacement once, should result in an acceptable result now.

comment:7 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

There is definitely a huge performance enhancement with the latest patch. A more complete test is needed on it, but it works pretty well for me.

There are just small notes regarding the changes on range.js:

  • At line 735, we should have the following at this point:
if ( !collapsed && this.startContainer.equals( this.endContainer ) ) 
  • The additions at line 775-776 seem unneeded. The range will never be collapsed at that point, because of the condition at line 748.
  • The line space addition at line 724 seems unneeded.

comment:8 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: 3.0RC added

Changed 10 years ago by Garry Yao

Attachment: 3790_4.patch added

comment:9 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

Fixing logic errors Fred found above and adding TC for the changes to CKEDITOR.dom.range::trim.

comment:10 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Please change the reference to "this.collapsed" to simply "collapsed" at line 748, as well as attaching the ! character to the parenthesis when committing.

comment:11 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3734] and [3733].

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