480096 - mozillas implementation of [Array.lastIndexOf] handling [Undefined] types in comparison to its native [Array.prototype.lastIndexOf]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[peter seliger]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6

example:

Array.lastIndexOf([2, 3,, 4, 5, 6]); // -1
/*
	versus
*/
[2, 3,, 4, 5, 6].lastIndexOf(); // 2

the result of one of the above 2 lines of code is wrong.

maybe it is the generic [Array.lastIndexOf] that does not perform as one could
expect since the prototypal method used in different cases that try to emulate
the generic implementation always returns the expected result - examples:

//this one is closest to Array.lastIndexOf([2, 3,, 4, 5, 6]); // -1
Array.prototype.lastIndexOf.call([2, 3,, 4, 5, 6]); // 2

Array.prototype.lastIndexOf.apply([2, 3,, 4, 5, 6], [, -4]); // 2
Array.prototype.lastIndexOf.apply([2, 3,, 4, 5, 6], [, -5]); // -1
Array.prototype.lastIndexOf.apply([2, 3,, 4, 5, 6], [window.undefined, -4]); // 2
Array.prototype.lastIndexOf.apply([2, 3,, 4, 5, 6], [window.undefined, -5]); // -1

the generic method again does indeed deliver results one would expect if an
[undefined] value was set explicitly as this methods second argument - code:

Array.lastIndexOf([2, 3,, 4, 5, 6], window.undefined); // 2
Array.lastIndexOf([2, 3,, 4, 5, 6], window.undefined, 1); // -1
Array.lastIndexOf([2, 3,, 4, 5, 6], window.undefined, 2); // 2

questions:
1) why gets omitting the second argument of the generic method treated
   differently from omitting the first argument of the prototypal one?
2) why is the result of omitting the second argument of the generic method
   different from omitting the second argument using prototypal delegation?
3) the last question that does arise - why does omitting an argument gets
   treated differently from setting [undefined] explicitly as argument? -
   could be answered looking at ...
   [https://bugzilla.mozilla.org/show_bug.cgi?id=475925#c6] but only
   if there was a consistency in handling such [undefined] arguments
   throughout both method implementations the generic as well as the
   prototypal one.

   thanks in advance for answering this whole bunch of questions and ...

   kind regards - Peter Seliger - [e-mail]



Reproducible: Always

Comment 1

[Jeff Walden [:Waldo]]

I'm not sure why you're seeing the behavior you're seeing.  [2, 3,, 4, 5, 6] is supposed to omit the property at index 2 entirely -- it shouldn't exist at all -- so both methods should ignore it.  Firefox 3 did not do this, as I recall, so I would have expected 2 as the result in both cases in that browser.  Recent 3.1 builds, as well as bleeding-bleeding-edge trunk builds, all return -1 consistently.

I'm keeping this open to find out why this discrepancy exists in Firefox 3, but I don't think the problem will be fixed there as it's not a security/stability problem, and sites might conceivably rely on it.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This bug is pretty old. The reason it doesn't bite on trunk is because after bug 412296, we no longer access vp[argc]. It could potentially bite a non-fast generic function, but we don't have any of those in the tree. The problem is that with argc < fun.nargs, assuming that vp[2 + argc] is undefined doesn't hold true. I don't know if we can fix this on 1.9.0, but it seems like good hygiene to fix it on trunk.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 364431 [details] [diff] [review]
Proposed fix

r=me, thanks.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/ecec767a3fa5

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for the 1.9.0 branch

I don't know if it's worth taking this on the 1.9.0 branch. It's not an easy bug to run into (you have to pass fewer than the minimum number of expected args through a generic method). It isn't a crash and it isn't exploitable. There are also easy workarounds (that is, passing undefined explicitly or using the Array.prototype.foo.call method). That being said, the 1.9.0 branch is the only branch where this bites, and the patch is small and very safe.

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ecec767a3fa5

Comment 7

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3814e365bbc6

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 365057 [details] [diff] [review]
Fix for the 1.9.0 branch

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Checked into the 1.9.0 branch.

Comment 10

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/0b5d2fa00f89

/cvsroot/mozilla/js/tests/ecma_3/Array/15.5.4.8-01.js,v  <--  15.5.4.8-01.js
initial revision: 1.1

Comment 11

[Bob Clary [:bc] (inactive)]

mrbkap, the test still fails in 1.9.0:

ecma_3/Array/15.5.4.8-01.js
Expected value '-12', Actual value '15'

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

That's actually "correct." See comment 1. This bug makes us consistent in that Array.prototype.lastIndexOf.call(K) === Array.lastIndexOf(K) for any K arguments.

Comment 13

[Bob Clary [:bc] (inactive)]

mrbkap: 1.8.1 and 1.9.0 agree on the result, I guess the test is wrong then?

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Does this mean the test is wrong or that the bug isn't properly fixed?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

The test is wrong for the 1.8.1 and 1.9.0 branches.

Comment 16

[Bob Clary [:bc] (inactive)]

v 1.9.0, 1.9.1, 1.9.2 based on comment 12.