Opened 9 years ago

Closed 8 years ago

#2018 closed Bug (fixed)

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

Reported by: fredck Owned by: martinkou
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 martinkou 8 years ago.
Proof-of-concept patch
2018.patch (2.8 KB) - added by martinkou 8 years ago.
2018_2.patch (2.7 KB) - added by martinkou 8 years ago.
2018_3.patch (2.4 KB) - added by martinkou 8 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by martinkou

  • Owner set to martinkou
  • Status changed from new to assigned

comment:2 Changed 8 years ago by martinkou

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 8 years ago by martinkou

Proof-of-concept patch

comment:3 Changed 8 years ago by martinkou

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 8 years ago by martinkou

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 8 years ago by martinkou

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 8 years ago by alfonsoml

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

comment:7 Changed 8 years ago by martinkou

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 8 years ago by martinkou

comment:8 Changed 8 years ago by martinkou

  • 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 8 years ago by martinkou

  • Keywords Review? removed

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

Changed 8 years ago by martinkou

comment:10 Changed 8 years ago by martinkou

  • Keywords Review? added

comment:11 Changed 8 years ago by fredck

  • 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 8 years ago by martinkou

comment:12 Changed 8 years ago by martinkou

  • Keywords Review? added; Review- removed

comment:13 Changed 8 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:14 Changed 8 years ago by martinkou

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed with [2095].

Click here for more info about our SVN system.

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