269739 - recursive command aliases cause slow/freeze/crash

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Michael Cordover]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Command aliases can refer to themselves, intially causing a slowdown, as
infinite loops tend to do.  The "a script is taking a long time - discontinue
it?" dialogue is displayed after a while which allows the command to be
terminated, although responsiveness of computer is already in a state where a
freeze of Firefox has been known to happen.

Reproducible: Always
Steps to Reproduce:
1. Write a recursive command alias (e.g. back=back;away;nick mjec)
2. Run it (i.e. type /back)
Actual Results:  
Computer response slows down, Chatzilla stops responding.  Eventually script can
be terminated by "this script is taking a long time" dialogue.  In some cases
browser stops responding altogether, sometimes (though generally only with
excessive other CPU/RAM use) system crashes.  Hey, it's Windows.

Expected Results:  
Not allowed the command to run (e.g. with message "Recursive command aliases
create infinite loops and they are a Bad Thing(tm); /back not run").

Chatzilla /does/ say that it doesn't like the command but continues running it
anyway :/.

Comment 1

[James Ross]

There are a couple of ways I can see of solving this:

  - check the call stack when running a command to see if we're recursing
  - simply count the recurse depth in a command manager variable
  - mark the commands themselves as 'running' temporarily

The last one seems good, but we need to allow for at least some controlled
recursion: the /away or /back commands when run on the client view will call
themselves for each network.

Comment 2

[:Gijs (he/him)]

(In reply to comment #1)
>   - mark the commands themselves as 'running' temporarily
> 
> The last one seems good, but we need to allow for at least some controlled
> recursion: the /away or /back commands when run on the client view will call
> themselves for each network.
I like this one, but I think it would be safe to make it only apply to aliases? That way the controlled recursion of those commands is no problem. It would already catch the alias recursion in the act, and if the built-in commands recurse badly, then something much worse is going on (and bugs should be filed & fixed).

Comment 3

[Robert Ginda]

I think option two is the way to go.  Have dispatch() increment something on the command manager when it starts and decrement on the way out.  If the count ever gets higher than, say, commandManager.maxDispatchDepth, then throw an exception.

Comment 4

[James Ross]

Attachment: Enforce max dispatch depth and unwind when hit

This took a little more magic that I had expected, due to the fact that displaying a message requires the command create-tab-for-view to run, so I had to make it unwind the entire stack before displaying the message at the end. It works, though, and the break when unwinding means that any aliases it is in the middle of will NOT run the rest of their code - this means it effectively kills the execution, which I think is probably the best solution.

Comment 5

[James Ross]

Checked in --> FIXED.