loadPlugins() _loadback() etc design flaw causing endless loops on plugin error.

[yermol]

The design of all instances where a script uses the

- invoke background load, - set onload handler callback - return false

structure is flawed.

If, for instance, a plugin has a syntax error loadPlugins() will go into an endless loop atttempting to load and reload the plugin. This will slow down and eventually crash the browser.

A solution I have implemented in the unified_backend branch is to keep a counter of load attempts for each plugin and if the number of attempts exceeds a certain maximum (in my case 10) an error popup is generated.

I am implementing the same pattern in other areas where I find this problem. (linker plugin for instance).

[niko]

i had this problem too allready, very annoying - most of the time i had to kill the browser :(

another idea: define an array within loadPlugin (HTMLArea.prototype.loadedPlugins) where the store the Name of the allready loaded plugins. bevore loading we check if the plugin should be allready loaded by searching through loadedPlugins. (instead of the if(eval('typeof ' + pluginName) != 'undefined')

this would avoid having to load it 10 times using a counter as you suggested.

[gogo]

Yes, I see the problem. For plugins, the best method is of course to keep track of what has been attempted to load, and only attempt to load once, there doesn't seem to be any reason to try loading 10 times, if it fails, it fails, I don't think it's going to succeed later :)

[niko]

would a fix like this be nice for you? (don't know if HTMLArea._loadedPlugins beautiful and i didn't know where to write HTMLArea._loadedPlugins = new Array();)

@@ -1068,7 +1068,7 @@
   for (var i = toolbar.length; --i >= 0;) {
     for (var j = toolbar[i].length; --j >= 0; ) {
       if (toolbar[i][j]=="popupeditor") {
-        if(typeof FullScreen == "undefined") {
+        if(HTMLArea._loadedPlugins.contains("FullScreen"))
           HTMLArea.loadPlugin("FullScreen", function() { editor.generate(); } );
           return false;
         }
@@ -1084,7 +1084,7 @@
     {
       case 'best':
       {
-        if(typeof EnterParagraphs == 'undefined')
+        if(HTMLArea._loadedPlugins.contains("EnterParagraphs"))
         {
           HTMLArea.loadPlugin("EnterParagraphs", function() { editor.generate(); } );
           return false;
@@ -1800,9 +1800,10 @@
   return _editor_url + "plugins/" + pluginName;
 };

+HTMLArea._loadedPlugins = new Array();
 HTMLArea.loadPlugin = function(pluginName, callback) {
   // Might already be loaded
-  if(eval('typeof ' + pluginName) != 'undefined')
+  if(HTMLArea._loadedPlugins.contains(pluginName))
   {
     if(callback)
     {
@@ -1811,6 +1812,8 @@
     return;
   }

+  HTMLArea._loadedPlugins.push(pluginName);
+
   var dir = this.getPluginDir(pluginName);
   var plugin = pluginName.replace(/([a-z])([A-Z])([a-z])/g,
           function (str, l1, l2, l3) {

[gogo]

That doesn't quite look right..shouldn't this

-        if(typeof EnterParagraphs == 'undefined')
+        if(HTMLArea._loadedPlugins.contains("EnterParagraphs"))

be

-        if(typeof EnterParagraphs == 'undefined')
+        if(!HTMLArea._loadedPlugins.contains("EnterParagraphs"))

(notice the bang (!)) and same for FullScreen??

Can you also check that this works in the example when using more than one editor, I tried to do a similar thing today but something funny is going on when multiple editors are in use, I can't figure out why.

Thanks :)

[niko]

you are right of course :D i didn't test the patch...

[niko]

did a bit debugging into this problem... things get complicated now!

• generate for the first editor is called
• the first editor calls loadPlugin
• loadPlugin calls _loadback and the generate-function of the first editor is quited
• generator for the second editor is called
• loadPlugin is NOT called (as it is allready in loadePlugins - which is correct)
• but the plugin-file is not yet loaded, so there is not Plugin-object avaliable for the second editor and we get a JS error

without this patch things are not better i suppose, the plugin-file just gets loaded twice.

possible solution:

• HTMLArea._loadedPlugins saves which plugin-files have been requested
• HTMLArea._reallyLoadedPlugins saves which plugins-files have been loaded
• in loadPlugin if a plugin is in _loadedPlugins but not in _reallyLoadedPlugins call callback-function with a timeout
• OR we create a HTMLArea._loadPluginCallbackFunctionPluginName? = new Array();
  • and push all callback-functions into the array
  • when file is loaded call all those functions
  • (not sure if we need just one _loadPluginCallbackFunction array or one for every plugin)

[gogo]

Fixed in changeset:169

[gogo]

Actually, only loadPlugins is fixed in changeset:169 there are some other places where loadback is used in a similar way to load dialog.js for example. These should also be fixed somehow...

[gogo]

Thinking about it some more, dialog.js, inline-dialog.js and popupwin.js won't be an issue because if they fail the callback will never get called (presently), and thus there can be no infinite loop (at most there would be 3 calls).

[niko]

much thanks for that commit!

...for dialog.js and the other files i see this problem:

• a page with two xinhas
• xinha1.generate is called
• loadback(dialog.js) is called
• xinha1.generate is exited (later entered by callback-function)
• xinha2.generate is called
• loadback(dialog.js) is called AGAIN

i did not test/debug this and i'm not sure if it si really like that... is probably more a problem of changeset:140

[gogo]

Hmm, yes you are right niko. Although it's not such a serious problem as the loadplugin problem because it won't make for infinite loops, at the very most 3*<number of editors> I think. But it should be fixed still, what we need is a "require_once()" type of function which works mostly like loadplugin but for any file.

[niko]

...so why not move the loading/failed/ready-logic from the loadPlugin-functions into _loadback? would this have any side-effects?

[gogo]

It would if we ever need to _loadback a file more than once.

[niko]

why should we ever need to load a file more than once?

loadback is only used to load additional javascrip-functions/classes etc... and those only have to be loaded once!

...or we just add a 3rd parameter once=true to make it configurable :D

[ray]

rev [792]: added id=url to dynamically generated script nodes in _loadback(). This way we can be sure each file is loaded exactly once