396252 - firefox javascript parser breaks on parentNode.removeChild() in for-loop, serious (but short) example provided

Status: VERIFIED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[ABANDONED ACCOUNT]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6

Run the example at http://www.pulseforce.com/firefoxJSbug.html

I have annotated the bug with alert() boxes that walks you through the entire process of Firefox corrupting an array, with a VERY clear and SHORT example that demonstrates the bug beyond doubt.

Then look at the page source code. I promise you, it's very short and clear. Ignore the alert() boxes when you read the code, that's all it takes. If you just remove all the alert()-code you can look at just the broken bit. All in all it's 4 lines and that is counting the end brackets } ;-)

Here it is, repeated:
var params = document.getElementById("FlashMovie").getElementsByTagName("param");
for(i = 0; i < params.length; i++) {
	alert("Removing, Param Name: " + params[i].name + " (params[i].name)\nLocation in array: " + i + " (i)\nNo. of params found (bugged in FF, will be decreasing as the array is corrupted and not all parameters are deleted): " + params.length + " (params.length)");
	params[i].parentNode.removeChild(params[i]);
}

I've taken every step I can to provide a detailed report and hope that this is enough and I would appreciate some feedback.

Reproducible: Always

Steps to Reproduce:
1. Read my report.
2. Run the provided sample.
Actual Results:  
Firefox corrupts a javascript array and truncates it.

Expected Results:  
Keep the array intact, but the addition of one small line breaks Firefox's javascript parser.

Bug exists in Firefox 2 and should be considered Major or Critical due to basic javascript parsing being broken.

Comment 1

[Phil Ringnalda (:philor)]

See http://developer.mozilla.org/en/docs/getElementsByTagName or http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-1938918D - getElementsByTagName returns a live nodelist, so when you remove one from the document, you also remove it from params.

Comment 2

[ABANDONED ACCOUNT]

Fine, thanks for clearing up the shrinking array, the bug should be reclassified since the fact that the array shrinks does not matter. :-)

But the serious bug is still there in that it only removes two params (the first and third) and bugs out on the remaining two in the array! Don't close this bug report.

This is despite the fact that it is ABLE to loop through all four found params, as evident by looping through with just an alert() box and no parentNode.removeChild(). You'll see four alert boxes.

However, if you try to also remove the param when looping then you'll quickly see the bug and only see two alert boxes, confirming the bug.

Comment 3

[ABANDONED ACCOUNT]

Don't forget that this is very short and simple, standardized code and that Internet Explorer (blech) handles it correctly while in Firefox there's a bug that makes the entire for-loop cracked, skipping two elements at absurd locations in the array. The second and fourth array element are skipped in this example, and only in Firefox, not in IE (which handles it correctly).

Comment 4

[Phil Ringnalda (:philor)]

You're still not thinking through what a live nodelist is, and what has to happen when you remove the first element in it.

i = 0; params.length = 4
remove params[0], params.length = 3, params[0] = old params[1], params[1] = old params[2], params[3] = old params[4]
i = 1; params.length = 3
remove params[1] (the original params[2]), params.length = 2
i = 2; params.length = 2, 2 is not less than 2, loop ends.



It's generally a good habit to say

var numOfParams = params.length;
for (var i = 0; i < numOfParams; i++)

anyway, even when you aren't dealing with a nodelist, so you don't have to make 4 (or 20, or 5000) calls to .length to get the same number over and over.

Comment 5

[ABANDONED ACCOUNT]

I see, so the difference between FF and IE is that the list of nodes in FF is live, and by removing the first node we'll be offsetting the array. Alright, so I just loop backwards instead, begin by the last node?

for (var i = (params.length-1); i >= 0; i--)

In fact, I remember reading a long time ago, that it is faster to initialize i to the length and decrease it, than it is to loop upwards instead.

Oh and as for the good habit, it's not worth the extra line of code. I investigated this earlier today, funny thing that you should mention it now.

For average size arrays (max 1000 items) the difference is a few nanoseconds (not even a millisecond). For arrays of 100000 items the difference was 70ms for arrayLen vs 90ms for array.length, and for arrays of 1 million items the difference was 920ms for arrayLen and 990ms for array.length. It could be argued that it is still a good idea. But who has arrays of 100000 items (when the difference begins to show)?

You can try it out here since I saved the sample: http://www.pulseforce.com/node/bmark.html

While we're on the subject, without adding an extra line of code the above solution would accomplish the same thing:

for (var i = (params.length-1); i >= 0; i--)

This works well if we don't care in what order we process the array.

If I understood things right, the difference would be that a variable, such as var arrayLen = array.length points directly to the integer in memory, which array.length will first look up the pointer for the array, and then jump forward to the offset required for the .length property, and these extra instructions will of course take more time. Still, as was proven it will not matter at all unless you reach 100000 item arrays, or in my opinion even more (around half a million items), because the 20 millisecond difference at 100k items was not much at all.

Comment 6

[ABANDONED ACCOUNT]

Just read through the W3 page and noticed that Internet Explorer strikes again. It's really in the standard that the nodelist is supposed to be live, so the fact that the length doesn't change in IE is a bug in IE. Sigh. All the work we have to do because of IE sickens me. I'd say that 40% of coding is correcting things for IE.

Thank god that Firefox is rapidly gaining marketshare. I love it!