recursive command aliases cause slow/freeze/crash

RESOLVED FIXED

Status

Other Applications
ChatZilla
--
major
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: Michael Cordover, Assigned: James Ross)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.75])

Attachments

(1 attachment)

(Reporter)

Description

14 years ago
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 :/.
Product: Core → Other Applications

Updated

14 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

13 years ago
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.
(Assignee)

Updated

13 years ago
OS: Windows XP → All
Hardware: PC → All

Comment 2

13 years ago
(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

13 years ago
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.
(Assignee)

Updated

12 years ago
Assignee: rginda → silver
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

12 years ago
Created attachment 227895 [details] [diff] [review]
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.
Attachment #227895 - Flags: review?(samuel)

Updated

12 years ago
Attachment #227895 - Flags: review?(samuel) → review+
(Assignee)

Comment 5

12 years ago
Checked in --> FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [cz-0.9.75]
You need to log in before you can comment on or make changes to this bug.