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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Aiko, Assigned: mrbkap)

Tracking

(Depends on: 1 bug, {fixed1.8, regression, testcase})

Trunk
x86
Windows XP
fixed1.8, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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]
(Reporter)

Comment 1

12 years ago
Created attachment 197746 [details]
testcase
(Reporter)

Updated

12 years ago
Keywords: regression, testcase

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

12 years ago
Assignee: general → general
Component: DOM → JavaScript Engine
QA Contact: ian → general
(Reporter)

Comment 2

12 years ago
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.
(Assignee)

Comment 3

12 years ago
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #198370 - Flags: review?(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
(Assignee)

Comment 5

12 years ago
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. :-(
Attachment #198370 - Attachment is obsolete: true
Attachment #198392 - Flags: review?(jst)
(Assignee)

Updated

12 years ago
Attachment #198370 - Flags: review?(brendan)
Comment on attachment 198392 [details] [diff] [review]
Because of this

r=jst
Attachment #198392 - Flags: review?(jst) → review+
(Assignee)

Updated

12 years ago
Attachment #198392 - Flags: superreview?(brendan)
Comment on attachment 198392 [details] [diff] [review]
Because of this

Great, thanks!

/be
Attachment #198392 - Flags: superreview?(brendan)
Attachment #198392 - Flags: superreview+
Attachment #198392 - Flags: approval1.8b5+
(Assignee)

Comment 8

12 years ago
Fix checked into MOZILLA_1_8_BRANCH and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
(Reporter)

Comment 9

12 years ago
Verified fixed with today's trunk build, thanks.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

12 years ago
Depends on: 311090
Looks as though this may have caused bug 311104.
Blocks: 311067
No longer blocks: 311067
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

12 years ago
(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

12 years ago
Checking in regress-310351.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-310351.js,v  <--  regress-310351.js
initial revision: 1.1
done
Flags: testcase+

Comment 14

11 years ago
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

Updated

10 years ago
Depends on: 379248
You need to log in before you can comment on or make changes to this bug.