446634 - SM, MC: dense array previous "length" value is not reported correctly in watch callback (js1_5/extensions/regress-435345-01.js)

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

BUGNUMBER: 435345
STATUS: Watch the length property of arrays
 FAILED! [reported from test()] Watch the length property of arrays: 1 : Expected value 'watcher: propname=length, oldval=0, newval=1; ', Actual value '' 
 FAILED! [reported from test()] Watch the length property of arrays: 2 : Expected value 'watcher: propname=length, oldval=1, newval=2; watcher: propname=length, oldval=2, newval=2; ', Actual value 'watcher: propname=length, oldval=undefined, newval=2; ' 
 FAILED! [reported from test()] Watch the length property of arrays: 3 : Expected value 'watcher: propname=length, oldval=2, newval=1; ', Actual value 'watcher: propname=length, oldval=undefined, newval=1; ' 
 FAILED! [reported from test()] Watch the length property of arrays: 4 : Expected value 'watcher: propname=length, oldval=1, newval=2; ', Actual value 'watcher: propname=length, oldval=undefined, newval=2; ' 
 FAILED! [reported from test()] Watch the length property of arrays: 5 : Expected value 'watcher: propname=length, oldval=2, newval=5; ', Actual value 'watcher: propname=length, oldval=undefined, newval=5; '

Comment 1

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

Brian, can you look at this? If it's a regression we probably need to block on it and get a fix in asap.

Comment 2

[Brian Crowder]

https://bugzilla.mozilla.org/show_bug.cgi?id=435345#c22

Comment 3

[Daniel Veditz [:dveditz]]

Does that mean "don't fix it at all" or "don't need to rush it into 1.9.0.2" ?

Comment 4

[Brian Crowder]

It would be nice to have, but I think it is not wildly important or high-priority.  watch() is, itself, a spidermonkey extension, and watching the "length" property of an array is relatively obscure.  Ideally, we should fix this, but I think it means making shavarrays slower (perhaps measurably so).

Comment 5

[Brendan Eich [:brendan]]

Dan: we should take the fix into a patch release. Further bugs may be fixed later. There's no reason to wait for perfection before putting this bug's fix into the patch-stream, though, and this bug is a regression from Firefox 1-2.

/be

Comment 6

[Brian Crowder]

fwiw, I landed 435345 today, so all that remains is to fix the reporting of the previous length value.

Comment 7

[Brian Crowder]

The most serious part of this bug landed on the branch in bug 435345 (watch didn't work -at all- for array length).  I'm morphing this bug to reflect the remaining piece, which is of lower severity in my opinion, and which may not be fixable without impacting performance measurably.

Comment 8

[Brian Crowder]

I'd like it if this one could drop off the blocker list.

Comment 9

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

Brian, I'm confused. This bug was a follow up to bug 435345 (you said so in comment 26 of that bug), wherein Brendan specifically says (comment 5 of this bug) we should take the fix here in a patch release.

How is it a duplicate of bug 435345?

Comment 10

[Brian Crowder]

Err, sorry.  I marked it as a dupe, wrote a dupe comment, and then rewrote my comment without removing the dupe marker.  The comment #7 here is correct, though.  This is a lower-severity bug (not a blocker).

Comment 11

[Daniel Veditz [:dveditz]]

Sold -- not blocking 1.9.0.x

Comment 12

[Brian Crowder]

Mass unowning JS bugs...  I'm not likely to be able to move these forward.

Comment 13

[Bob Clary [:bc] (inactive)]

search goodness: js1_5/extensions/regress-435345-01.js

Comment 14

[Till Schneidereit [:till]]

Given that we want .watch to die and this doesn't seem to cause any real-world issues, I'm WONTFIXing. Shout if you disagree.