310351 - Cannot convert NodeList to JS Array with Array.prototype.slice any more

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Aiko]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

Until recently it was possible to convert a NodeList, e.g. the result of
document.getElementsByTagName to a JS array using code like

array = Array.prototype.slice.call(nodeList, 0)

This is broken in current trunk builds and Firefox Beta 1. It was still working
in Deer Park Alpha 2. I'm not sure whether other generic JS array methods are
affected, at least join() still works. 

Maybe this was caused by the splitwindows patch, so hopefully placing this in
DOM: Core isn't too far off.

Reproducible: Always

Steps to Reproduce:
1. Run the attached testcase
Actual Results:  
FAIL
,,
[object HTMLBRElement],[object HTMLBRElement],[object HTMLBRElement]

Expected Results:  
PASS
[object HTMLBRElement],[object HTMLBRElement],[object HTMLBRElement]
[object HTMLBRElement],[object HTMLBRElement],[object HTMLBRElement]

Comment 1

[Aiko]

Attachment: testcase

Comment 2

[Aiko]

I had some more time to browse through archive.mozilla.org.

Works: 1.8b3 20050713
Fails: 1.8b3 20050714

In that time frame, bug 299738 added a PropertyExists check to array_slice, this
looks like a probable cause.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Why is this needed?

It appears that OBJ_LOOKUP_PROPERTY is failing us here. For some reason that
I'm not really sure of, it's returning a null property, even though that
property actually does exist. The OBJ_GET_PROPERTY is returning the correct
value, causing us to not think that the collection is empty.

Comment 4

[Brendan Eich [:brendan]]

It's always possible for some naughty host object to answer "no" to lookups (by
not resolving) but still give a useful value back, or even reify a property
lazily on get.  Is that the case here?  We should probably back away from the
change to slice, but similar changes or existing patterns occur elsewhere in
jsarray.c.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Because of this

This seems to fix this. I feel like more testing is necessary, but don't have
time. :-(

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 198392 [details] [diff] [review]
Because of this

r=jst

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 198392 [details] [diff] [review]
Because of this

Great, thanks!

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH and trunk.

Comment 9

[Aiko]

Verified fixed with today's trunk build, thanks.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Looks as though this may have caused bug 311104.

Comment 11

[Boris Zbarsky [:bzbarsky]]

It looks like this affected Tdhtml some (about 1% or so).  Those testcase do
quite a lot of access to various DOM nodelists, and this patch added some extra
work per each access.  :(

Comment 12

[[e-mail]]

(In reply to comment #0)
>array = Array.prototype.slice.call(nodeList, 0)
That's neat. Now if only [].concat(nodeList) worked like that ;-)

Comment 13

[Bob Clary [:bc] (inactive)]

Checking in regress-310351.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-310351.js,v  <--  regress-310351.js
initial revision: 1.1
done

Comment 14

[Bob Clary [:bc] (inactive)]

The nodeList is live and if the dom changes the test will falsely fail. This saves the nodeList length prior to reportCompare to prevent that.

Checking in regress-310351.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-310351.js,v  <--  regress-310351.js
new revision: 1.2; previous revision: 1.1
done