Opened 10 years ago

Closed 9 years ago

#2018 closed Bug (fixed)

FCKeditorAPI is not cleaned up when removing editor instances manually from the page

Reported by: Frederico Caldeira Knabben Owned by: Martin Kou
Priority: Normal Milestone: FCKeditor 2.6.2
Component: General Version: FCKeditor 2.5
Keywords: Confirmed Review+ Cc:

Description

The fix for #183 broken the FCKeditorAPI "Instances" object cleanup. The "_test/manual/fckeditorapi/test1.html" test page shows the problem.

Attachments (4)

2018_pre.patch (1.7 KB) - added by Martin Kou 10 years ago.
Proof-of-concept patch
2018.patch (2.8 KB) - added by Martin Kou 9 years ago.
2018_2.patch (2.7 KB) - added by Martin Kou 9 years ago.
2018_3.patch (2.4 KB) - added by Martin Kou 9 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned

comment:2 Changed 10 years ago by Martin Kou

I can't really think of any simple hack to fix this without breaking #183 - which unfortunately is also quite important.

There seems to be no way of knowing whether FCKeditor is running under a browser or an embedded browser control, so we can't just target the onbeforeunload trick to Microsoft's WebBrowser control. There are two alternatives that I can think of, but they may bring problems themselves:

  1. Use the topmost window hosting FCKeditorAPI to detect whether an editor iframe who'd raised the unload event is really removed from the DOM tree, with a timeout. If it is not removed, then don't delete it from the FCKeditorAPI.Instances dictionary. Potential problem: Is it possible for editor instances to be removed without removing its iframe element?
  2. We define a method for removing FCKeditor instances and ask the web developers to use it in our documentation instead of using element.removeChild($editorIframe). Potential problem: A lot of people don't read the documentation...

Related to point 2. I've found from a support request last week that removing FCKeditor instances by element.removeChild($editorIframe) doesn't work at present, as it will cause JavaScript errors.

Changed 10 years ago by Martin Kou

Attachment: 2018_pre.patch added

Proof-of-concept patch

comment:3 Changed 10 years ago by Martin Kou

I've written a proof-of-concept patch for alternative one. It works in IE6 and FF3. The patch looks clumsy but much of the clumsiness is there due to the need to avoid crashing the browser (with C++ errors no less!) while the browser tries to access freed memory in an unloaded frame, due to setTimeout().

The patch can be further simplified but a lot of tests will be needed to make sure it doesn't crash browsers. Also more tests will be needed to see if it works on IE7, Safari and Opera.

comment:4 Changed 10 years ago by Martin Kou

Found two more problems with the proof-of-concept code:

  1. Opera 9.50 seems to not trigger the unload event when iframes are being removed.
  2. It doesn't work in Safari at all.

comment:5 Changed 10 years ago by Martin Kou

I guess I should go with the second approach now. As it turns out, even the onunload event in IE is somewhat faulty - it doesn't fire off at the moment the iframe is removed. It seems to fire off only when enough iframes are removed (triggered by GC, perhaps?).

comment:6 Changed 10 years ago by Alfonso Martínez de Lizarrondo

check also #550. That problem existed before #183 was fixed

comment:7 Changed 10 years ago by Martin Kou

I've coded a RemoveInstance method under FCKeditorAPI that looks like this:

                                'RemoveInstance : function( name )' +
                                '{' +
                                        'var instance = this.Instances[name] ;' +
                                        'delete this.Instances[name] ;' +
                                        'if ( instance && instance.GetInstanceObject )' +
                                        '{' +
                                                'var el = instance.GetInstanceObject( "frameElement" ) ;' +
                                                'el.parentNode.removeChild( el ) ;' +
                                        '}' +   
                                '}' +                   

This satisfies the requirement of the fckeditorapi test if the test case was modified to use FCKeditor.RemoveInstance() instead of plain DOM iframe removal.

However, this still doesn't solve the problem of shared objects (e.g. FCK) being erased away and causing JavaScript errors in multiple editor scenarios. Say, you run FCKeditorAPI.RemoveInstance( 'FCKeditor_1' ) in sample11.html with the above function definition, and click the remaining editing area. You'll get a bunch of JavaScript errors saying some shared objects are no longer defined.

Changed 9 years ago by Martin Kou

Attachment: 2018.patch added

comment:8 Changed 9 years ago by Martin Kou

Keywords: Review? added

After some discussion with Fred yesterday, it was decided that reversing the fix to #183 would be the best approach for now. For those who really need to view their website with Microsoft's WebBrowser control, a configuration directive MsWebBrowserControlOnUnloadKludge has been added for them to force the old behavior.

comment:9 Changed 9 years ago by Martin Kou

Keywords: Review? removed

Oops... I left an alert() in the patch... let me clean that up first.

Changed 9 years ago by Martin Kou

Attachment: 2018_2.patch added

comment:10 Changed 9 years ago by Martin Kou

Keywords: Review? added

comment:11 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

I've tried to apply the patch, but it seems to be broken (no trac preview, for example).

Anyway, we could have a more generic name for the configuration instead of "MsWebBrowserControlOnUnloadKludge". What about "MsWebBrowserControlCompat"?

Also, there is no need of comments in the configuration file. Let's leave it well documented in the docs site.

Changed 9 years ago by Martin Kou

Attachment: 2018_3.patch added

comment:12 Changed 9 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:13 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:14 Changed 9 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [2095].

Click here for more info about our SVN system.

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