Opened 12 years ago

Closed 11 years ago

#226 closed defect (fixed)

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

Reported by: yermol Owned by: yermol
Priority: normal Milestone: Version 1.0
Component: Xinha Core Version: trunk
Severity: normal Keywords:
Cc: yml@…

Description

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).

Change History (17)

comment:1 Changed 12 years ago by yermol

  • Owner changed from gogo to yermol

comment:2 Changed 12 years ago by 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.

comment:3 Changed 12 years ago by gogo

  • Milestone set to Version 1.0
  • Version changed from yermo to trunk

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 :)

comment:4 Changed 12 years ago by gogo

  • Component changed from Plugin - Fullscreen to Xinha Core

comment:5 Changed 12 years ago by 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) {

comment:6 Changed 12 years ago by 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 :)

comment:7 Changed 12 years ago by niko

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

comment:8 Changed 12 years ago by 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)

comment:9 Changed 12 years ago by gogo

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

Fixed in changeset:169

comment:10 Changed 12 years ago by gogo

  • Resolution fixed deleted
  • Status changed from closed to reopened

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...

comment:11 Changed 12 years ago by 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).

comment:12 Changed 12 years ago by 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

comment:13 Changed 12 years ago by 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.

comment:14 Changed 12 years ago by niko

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

comment:15 Changed 12 years ago by gogo

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

comment:16 Changed 12 years ago by 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

comment:17 Changed 11 years ago by ray

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

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

Note: See TracTickets for help on using tickets.