Closed
Bug 310351
Opened 19 years ago
Closed 19 years ago
Cannot convert NodeList to JS Array with Array.prototype.slice any more
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: Seno.Aiko, Assigned: mrbkap)
References
Details
(Keywords: fixed1.8, regression, testcase)
Attachments
(2 files, 1 obsolete file)
442 bytes,
text/html
|
Details | |
3.81 KB,
patch
|
jst
:
review+
brendan
:
superreview+
brendan
:
approval1.8b5+
|
Details | Diff | Splinter Review |
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]
Keywords: regression,
testcase
Updated•19 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•19 years ago
|
Assignee: general → general
Component: DOM → JavaScript Engine
QA Contact: ian → general
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•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
Attachment #198370 -
Flags: review?(brendan)
Comment 6•19 years ago
|
||
Comment on attachment 198392 [details] [diff] [review] Because of this r=jst
Attachment #198392 -
Flags: review?(jst) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #198392 -
Flags: superreview?(brendan)
Comment 7•19 years ago
|
||
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•19 years ago
|
||
Fix checked into MOZILLA_1_8_BRANCH and trunk.
Verified fixed with today's trunk build, thanks.
Status: RESOLVED → VERIFIED
Comment 10•19 years ago
|
||
Looks as though this may have caused bug 311104.
Comment 11•19 years ago
|
||
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•19 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•19 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•18 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
You need to log in
before you can comment on or make changes to this bug.
Description
•