Thursday, April 26, 2007

Tip: Make sure you declare JavaScript For loops correctly or Firefox go boom

So far this year at my day job I have been working on my first AJAX application. It's still ASP.NET on the server, and also happens to be my first .NET 2.0 app. To the user though, once you are on a page, there are no postbacks. First off, if you are developing AJAX applications, you just have to get Firebug, as they say web deveopment evolved indeed. Jay Atwood (who works for my old masters) has an excellent article at Coding Horror called Firefox as an IDE, and it pretty much echoes my sentiments exactly on Firebug. This app I am working on is a combination of ASP.NET AJAX controls, a third-party control I don't want to mention by name, and a good chunk of custom code.

So what do you do when Firebug in Firefox, you new favorite development tool, consumes 100% CPU utilization running your app and you have to crash it to get your machine back? I went through the stages of grief in about 30 seconds, and wrote off Firefox compatability with my application for a while. See I chalked it up to a problem with the 3rd party control I was using (itself AJAXed to the nines) until I had another client-side problem with the app in Internet Explorer and the thought of debugging this without Firebug seemed a fate worse than having to sit through Year of the Dog. So I dug into the code, which I hadn't written, and I saw JavaScript similar to this in the execution path that was causing Firefox to hang:

var startIndex=0;

for(i=startIndex; i < endIndex; i++) {
//Do some stuff here
}

This code works 100% in Internet Explorer 7, why is this causing Firefox to go crazy? Well once I figured out the execution path, I was able to breakpoint it in Firebug, because as I suspected Firefox is getting stuck in an infinite loop. What happens? i is reinitialized to startIndex after every run of the loop. You have to declare the loop like this for it to work fine:

var startIndex=0;

for(var i=startIndex; i < endIndex; i++) {
//Do some stuff here
}

Putting the var before i is the way it ought to be as far as I can tell, but both Internet Explorer and Firefox do the wrong thing by developers here. Both browsers should be sticklers about requiring var in a loop variable declaration and produce a clear JavaScript interpreter error before the code has the change to run. Once I found this issue, I searched through all the rest of the custom JavaScript code in the application and found that about half the JavaScript for loops were declared with var and the other half were not. Fixing all these loops resolved all the weirdness (but not out right infinite loops, odd) other developers and some testers were reporting using Firefox, imagine that!

10 comments:

Anonymous said...

Your evaluation of what is going on in the Javascript is incorrect. "i" is not initialized to startIndex at the beginning of each iteration of the loop in Firefox:

/* prints 0 to 999 in both IE and Firefox */
var endIndex=1000;
var startIndex=0;

for(i=startIndex; i < endIndex; i++) {
document.write(i + '>br>');
//Do some stuff here
}

My guess is that your "//Do some stuff here" manipulated (or called functions which manipulated) a variable "i" as well, and in Javascript, if you use a variable without declaring it with "var", the scope is global (otherwise it is local to the containing function). Why this does not affect Internet Explorer is anyone's guess. Without seeing the remainder of the source code, I'd speculate that there is separate code to support IE and Firefox and only the Firefox code uses another global instance of "i".

However, as the sample code above demonstrates (tested in Firefox 2.0.0.3 and IE 7), the loop completes normally. There is no bug, error or differences between the way IE and Firefox handle the scoping rules and loop.

Anonymous said...

My apologies, code snippet should be (hopefully this comes out right):

/* prints 0 to 999 in both IE and Firefox */
var endIndex=1000;
var startIndex=0;

for(i=startIndex; i < endIndex; i++) {
document.write(i + '<br>');
//Do some stuff here
}

Joshua Paine said...

Anonymous is correct (about everything), and I'm glad no browsers flag failing to use var there as an error, because it's not. Two reasons:

1) This isn't really correct:

function bob()
{
var str='';
for(var i=0; i<5; i++)
str+='yo';
for(var i=0; i<5; i++)
str+='hi';
alert(str);
}

Because i has already been declared with var, there's no need to do it again in the second loop, though Firefox at least doesn't raise an error there either.

2) Not declaring your counter with var at all and thus polluting the global namespace and leaving yourself open to the weird interactions you experienced is something I have yet to hear of anyone doing knowingly on purpose, and it should never be done. But never is a very long time, and there might be a reason to do it that I can't think of (especially if, e.g., you did it in an inner function and the counter was declared in the containing function), so I'd hesitate to disallow it.

In a less extreme example, Crockford and (I'm sure) many others think this is a never-do:

if(resultINeedLater = someFunction())

but I like the line of code it saves me, and I rarely make mistakes about assignment instead of equality, and if I do I do not find them subtle bugs but catch them easily. Just some preference of my brain that I'm glad I can indulge even though it seems like a bad idea to some.

Brook said...

Your complaint isn't with the browsers. Your complaint is with ECMA-262 12.6.3, which explicitly permits for() without var in the first expression list. Were IE and FF to get sticky over that point, they'd not be compliant with the specification.

Joe said...

Simplifying what Brook said: this is how variable scope is supposed to work in JavaScript.

As a result, if the body of the "for" loop decrements the value of the counter, you can get an infinite loop.

Andrew said...

Also, javascript limits scope only within functions. Putting var within a for loop does not limit the scope of i to the loop. You can only limit the scope of i to within it's enclosing function. So to avoid confusion (as in the above) it's often recommended to var all your variables at the start of functions, rather than when they are first assigned.

Dave Murdock said...

@anonymous: I haven't been schooled like that in a long time ;-). Here is some sample code (greatly simplified from the original) that will cause Firefox to use 100% CPU for a while:

function multipleLoops() {
var startIndex = 0;
var endIndex = 1000;
var count = 0;

for(i=startIndex; i < endIndex; i++) {
count = count + doSomeStuff();
}

alert(i);
}

function doSomeStuff() {
var count = 0;

for(i=0; i < 10; i++) {
count = count + i;
}

return count;
}

This is pretty low complexity code, no branches obviously, and now both Firefox 2.0.0.3 and IE 7.0.5730.11 prompt to kill the "unresponsive script", though IE does it nearly immediately, Firefox lets it spin for 5-10 seconds. So Anon you were totally right that something else was "manipulating i", but you are wrong that we have separate code running for IE and Firefox, its all the same.

Dave Murdock said...

@Joshua Paine: The developer that orignally wrote the JavaScript was unintentionally declared i in the global namespace. I don't know you would ever want to do this on purpose either, but maybe someone has. And I am sorry, I dislike your if example, sacrificing clarity for brevity (and reducing 1 line at that) just doesn't seem worth it to me. You might understand what you wrote when you wrote it, but if you have to go back to this code and you have been using a language that has = as an equality operator, not just an assignment, the fog of confusion can set in.

Dave Murdock said...

@brook, joe, + andrew: From Crockford: "JavaScript's C-like syntax, including curly braces and the clunky for statement, makes it appear to be an ordinary procedural language. This is misleading because JavaScript has more in common with functional languages like Lisp or Scheme than with C or Java."

This sums up my confusion on scoping in a nutshell. It never occurred to me that if you did for(var i... that i was scoped at the function anyway. It also feels like I living in crazy town that if you don't use var, that i was globally scoped, I expected it was either scoped at the loop, or if you had quizzed me I would have said the function before reading all these comments, never would I have answered global. So yeah brook, I don't like the design of JavaScript here. It looks and feels like a C-derived language, but when things like this don't work like I expect from C-derived language experience, well its time to get back to basics. I am going to pick up JavaScript: The Definitive Guide based on Crockford's recommendation:
http://www.amazon.com/exec/obidos/ASIN/0596101996/wrrrldwideweb

Dave Murdock said...

It looks like the let block scopes variables to the block level in JavaScript 1.7 with Firefox 2.