Last Comment Bug 310351 - Cannot convert NodeList to JS Array with Array.prototype.slice any more
: Cannot convert NodeList to JS Array with Array.prototype.slice any more
Status: VERIFIED FIXED
: fixed1.8, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
:
Mentors:
Depends on: 379248 311090
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-28 13:10 PDT by Aiko
Modified: 2007-04-30 04:45 PDT (History)
4 users (show)
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (442 bytes, text/html)
2005-09-28 13:11 PDT, Aiko
no flags Details
Why is this needed? (896 bytes, patch)
2005-10-03 15:45 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Because of this (3.81 KB, patch)
2005-10-03 18:27 PDT, Blake Kaplan (:mrbkap)
jst: review+
brendan: superreview+
brendan: approval1.8b5+
Details | Diff | Splinter Review

Description Aiko 2005-09-28 13:10:20 PDT
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 2005-09-28 13:11:17 PDT
Created attachment 197746 [details]
testcase
Comment 2 Aiko 2005-10-03 14:14:15 PDT
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) 2005-10-03 15:45:26 PDT
Created attachment 198370 [details] [diff] [review]
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] 2005-10-03 16:56:32 PDT
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) 2005-10-03 18:27:35 PDT
Created attachment 198392 [details] [diff] [review]
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, jst@mozilla.com) 2005-10-03 18:30:46 PDT
Comment on attachment 198392 [details] [diff] [review]
Because of this

r=jst
Comment 7 Brendan Eich [:brendan] 2005-10-03 21:39:06 PDT
Comment on attachment 198392 [details] [diff] [review]
Because of this

Great, thanks!

/be
Comment 8 Blake Kaplan (:mrbkap) 2005-10-03 22:53:01 PDT
Fix checked into MOZILLA_1_8_BRANCH and trunk.
Comment 9 Aiko 2005-10-04 09:47:15 PDT
Verified fixed with today's trunk build, thanks.
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-10-04 13:15:18 PDT
Looks as though this may have caused bug 311104.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2005-10-06 12:23:52 PDT
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 neil@parkwaycc.co.uk 2005-10-08 17:29:34 PDT
(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:] 2005-10-09 19:44:50 PDT
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:] 2006-09-14 22:48:14 PDT
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

Note You need to log in before you can comment on or make changes to this bug.