436094 - document.all.length is undefined

Status: RESOLVED FIXED

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

Description

[bugzilla]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0

The following code is from a proprietary website.  In Firefox 2.x and MSIE the website accepts the input string without difficulty but in Firefox 3.x the website rejects every entry as invalid - even when the exact same record ID works in any other browser.

function retrieveAsset()
{
   numEl = document.all.length;
   astId = '';
   var reg_exp6 = /\d\d\d\d\d\d$/i;
   var reg_exp9 = /\d\d\d\d\d\d\d\d\d$/i;
   for ( var idx=0; idx < numEl; idx++){
      objName = document.all[idx].name;
      objValue = document.all[idx].value;
      if ( objName == 'retrieveAssetId' ){
         astId = objValue
      }
   }

   if ( astId != null && astId != '' && !isNaN(astId) && (reg_exp6.test(astId) || reg_exp9.test(astId)) && astId > 499999)
      location.href='worklistQueue.do?retrieveAsset=' + astId;
   else
      alert("Search - Asset ID must be a valid number with at least 6 digits and greater than 499999.");
}


Reproducible: Always

Steps to Reproduce:
1.Enter an asset ID with (exactly) six digits > 499999
2.Click the submit button on the page
3.
Actual Results:  
alert("Search - Asset ID must be a valid number with at least 6 digits and greater than 499999.")

Expected Results:  
Requested record should display

Comment 1

[Matthew Middleton (:zzxc)]

The issue with your code is that you are using document.all, which is a non-standard way of accessing the DOM.  You should be using standard methods such as document.getElementsByTagName for this purpose.  See http://developer.mozilla.org/en/docs/Gecko_DOM_Reference for documentation

In any case, document.all.length returns 'undefined' in trunk, whereas it returns the length of the collection in Firefox 2.  This was almost certainly caused by bug 259332 - the property works as expected in the 2007-07-25 nightly and returns undefined in the 2007-07-26 nightly.  This should be fixed for web compatibility.

I am attaching a testcase, comparing the results of document.all.length to document.getElementsByTagName("*").length.  These should return the same result.

Comment 2

[Matthew Middleton (:zzxc)]

Attachment: testcase

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think document.all.length will always return the same as document.getElementsByTagName('*').length. First of all the former won't contain elements without a name or id, second, it'll only contain one entry if there are multiple elements with the same name/id.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

As a workaround, you could change:

   for ( var idx=0; idx < numEl; idx++){
      objName = document.all[idx].name;
      objValue = document.all[idx].value;
      if ( objName == 'retrieveAssetId' ){
         astId = objValue
      }
   }

to

   astId = document.all.retrieveAssetId.value;

until this bug is fixed.


Hmm.. though I wonder, did that code ever work in FF2? You are not just
accessing document.all.length, but also document.all[0], document.all[1] etc,
which as far as I know has never been implemented in firefox.

Comment 5

[bugzilla]

The code did (and does) work properly using FF2 - it did not start giving me trouble until I started using FF3 (every build I tried has the same issue).

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please attach a minimal testcase that works in FF2 but not in FF3.

Comment 7

[bugzilla]

that code works exactly as is in FF2 but does not work in FF3

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please attach it as a working testcase, the code in comment 0 is just part of the file and also includes some location.href manipulation that doesn't seem needed.

Comment 9

[bugzilla]

Attach what, the entire source for the page?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attach as an attachment, see the "Attachments" section of this page.

There is one attachment there, but it doesn't demonstrate the full extent of the bug you are talking about but rather just displays the value of document.all.length.

And please make the testcase as small as possible, i.e. remove parts that are irrelevant to displaying the regression.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. it does indeed look like we're trying to map document.all.length to document.getElementsByTagName("*").length and document.all[number] to document.getElementsByTagName("*")[number]. So presumably my patch broke that somehow.

A testcase would be nice, but I could write one myself that tests for what we're trying to implement.

Comment 12

[bugzilla]

That snippet of code is all I have.

Comment 13

[Aiko]

Attachment: MochiTest

This tests for the behaviour described in comment 11. It passes in IE7, fails in Firefox 3 RC1.

Comment 14

[Samuel Sidler (old account; do not CC)]

Jonas, can you look at this?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This fixes this bug as well as bug 393629, and adds a stack of mochitests for document.all.

This should be safe for trunk as well as branch. The patch does the following:

1. Unbreak document.all.length. I see no reason this would break anything.
2. Make the list of elements whose 'name' attribute we look at match IE.
3. Fix what looks like unsafe usage of JSVAL_TO_INT by making sure that it is
   in fact an integer first.

I'm not sure if the last is needed. Not sure if the Resolve hook is ever called with something other than an integer or string?

It's hard to say how risky 2 is. What we did in FF2 was very inconsistent, in some cases we'll be closer to what FF2 does, is some cases further away. However we're definitely getting closer to what IE does which I think it what matters, so I think we should take this patch as is on both trunk and branch.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 335335 [details] [diff] [review]
Patch to fix

Looks good. The addition of the JSVAL_IS_INT() check is good and we should take that too, or we could get into trouble with XML type property names.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 335335 [details] [diff] [review]
Patch to fix

This patch should be very safe. The only thing that isn't obvious "fix things that are totally broken" is things that bring us closer to IE, and in many cases FF2.

Comment 18

[Samuel Sidler (old account; do not CC)]

This needs to land on trunk first to bake and it's a bit too late to take it for 1.9.0.2. An opposition to taking it in 1.9.0.3?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I really don't think there is a risk that anything would break, the patch is really safe. However I'm ok with waiting for 1.9.0.3 too.

Comment 21

[Samuel Sidler (old account; do not CC)]

Comment on attachment 335335 [details] [diff] [review]
Patch to fix

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to CVS

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2) Gecko/2008090212 Firefox/3.0.2.