434837 - Accessors in prototype chain of arrays don't assign 'this' correctly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Cyrus Omar]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0

Some very strange bugs when adding accessors to Array.prototype.

Assume the following initialization code in the scope of all of the examples:
x = [ "one", "two" ]

1. If only a getter is defined, then `this` in the getter callback is set to Array.prototype rather than the array the getter was invoked on:

Array.prototype.__defineGetter__('test', function() { print(this === x) })
x.test   // false

Array.prototype.__defineGetter__('test2', function() { print(this === Array.prototype) })
x.test2   // true

2. Setting the value of 'test' will cause an error instead of doing nothing:
x.test = 5   // exception

3. If both a getter and a setter are defined, then the same bug as above shows up UNTIL the first call to the setter, then it will work fine, even if the setter doesn't do anything:

Array.prototype.__defineGetter__('test', function() { print(this === x) })
Array.prototype.__defineSetter__('test', function() { print(this === x) })
x.test   // false
x.test = 5  // true
x.test   // true!

Just defining a setter is fine, no exception is thrown when trying to get the value.

4. These problems don't appear for Object.prototype and normal objects as far as I can tell. Method calls also appear to work fine if added to Array.prototype.

I'm not sure if this is a regression in Firefox 3 RC1 or something thats been around, don't have other versions on hand at the moment.

Reproducible: Always

Comment 1

[Mike Shaver (:shaver emeritus)]

Likely a regression from the dense-arrays work; when we fall through to the prototype on fetch of missing property we need to propagate the desired |this| to the getter somehow.

Comment 2

[Cyrus Omar]

Seems like you're right, any accessor added to the prototype chain of an array, whether its on Array.prototype or not, doesn't work. This is actually fairly annoying regression for library developers - any chance this could be fixed before FF3 releases?

Comment 3

[Cyrus Omar]

This seems to be causing other problems (or this may be an unrelated bug):

var x = [ 'a', 'b', 'c', 'd' ]
print(x.__count__)   // always 2, which is Array.prototype.__count__

I take it __count__ is defined as a property and thus is not working for the same reason. I don't know if __count__ is a property actually used anywhere important but I imagine it was created for a reason, so this will likely break something if not fixed.

Comment 4

[Mike Shaver (:shaver emeritus)]

Yeah, this regression could bite someone (though it's been in place since b4, and I haven't heard reports of it breaking people in the wild).  I don't think anyone relies on __count__.

Cyrus: can you point me at extant library code that's broken by this?  That'll help figure out the right course here WRT a ship vehicle for a fix.  I think that library authors generally try to avoid touching the prototype chain for Arrays and Objects because if the adverse effects on for/in, so I'd be a little surprised if there was wide use of getters and setters on the prototype chain for Arrays.  I have been surprised before, however. :)

Crowder: I'm not going to get to this today, can you take a look at whether we need a lookup-then-explicit-call-of-getter in the array_getProperty fallback, or if there's another way to proceed?  Boldly assigning your way.

Comment 5

[Brian Crowder]

Are we not doing the right thing for count?  We end up calling obj_getCount with Array.prototype as the obj, which seems to be the correct behavior.  I'll check out the other issues, though, and please correct me if I am wrong.

Comment 6

[Mike Shaver (:shaver emeritus)]

It looks like you would also see this effect where OBJ_IS_NATIVE o delegated to !OBJ_IS_NATIVE p and p has a getter:

jsobj.c:js_GetPropertyHelper

3705     if (!OBJ_IS_NATIVE(obj2)) {
3706         OBJ_DROP_PROPERTY(cx, obj2, prop);
3707         return OBJ_GET_PROPERTY(cx, obj2, id, vp);
3708     }

Likely a rare case, since until recently all of our non-DOM objects were OBJ_IS_NATIVE, but we might want to get it at the same time?  I fear unintended consequences there, though...

Comment 7

[Mike Shaver (:shaver emeritus)]

__count__ is found on Object.prototype, so calling it with a |this| of Array.prototype isn't going to be right -- |this| should be the array on which the lookup started.  Date shows the right behaviour:

>>> (new Date).hasOwnProperty("__count__")
false
>>> (new Date).__count__
0
>>> d = new Date; d.adhoc = 1; d.__count__
1

Comment 8

[Mike Shaver (:shaver emeritus)]

(Going to lookup-and-explicit-call-of-getter will fix __count__ as well as the scripted cases, I'm pretty sure.)

Comment 9

[Brian Crowder]

Attachment: OBJ_GET_PROPERTY(..proto..) becomes js_GetProperty(..obj..)

As noted on IRC, the result of [].__count__ isn't the same as {}.__count__ or (new Date()).__count__:

js> [].__count__
0
js> [1].__count__
8
js> [1,2,3,4,5,6,7,8,9].__count__         
16


But the setters seem to work right (note that calling a setter on a shavarray causes it to become a slowarray, even if the setter doesn't actually require the mutation):

js> Array.prototype.__defineGetter__('test', function() { print(this === x) })
js> var x = ["one", "two"]
js> x.test
true
js> Array.prototype.__defineSetter__('test', function() { print(this === x) })
js> x.test = 5                                                                 
true

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 321960 [details] [diff] [review]
OBJ_GET_PROPERTY(..proto..) becomes js_GetProperty(..obj..)

Nice, I was afraid that js_GetProperty would recur on us, but obviously it doesn't.

(I think the code is clearer with the non-dense cast-out and needs-prototype cases separate, even though they result in the same tail call.)

Comment 11

[Mike Shaver (:shaver emeritus)]

(In reply to comment #9)
> But the setters seem to work right (note that calling a setter on a shavarray
> causes it to become a slowarray, even if the setter doesn't actually require
> the mutation):

That shouldn't be visible to script authors -- if they can tell which sort of array they have, other than by the speed difference, then we have a bug that needs fixing.  We could check the case of a non-indexed setter found in a delegate and not fault as eagerly, but I don't think it's worth it.

Comment 12

[Brian Crowder]

(In reply to comment #11)
> That shouldn't be visible to script authors -- if they can tell which sort of
> array they have, other than by the speed difference, then we have a bug that
> needs fixing.  We could check the case of a non-indexed setter found in a
> delegate and not fault as eagerly, but I don't think it's worth it.

Yeah, agreed; mostly noting this for the reporter, whose code is liable to be affected by this performance issue, if he's making heavy use of prototyped setters, such as in this example.

Comment 13

[Mike Shaver (:shaver emeritus)]

This isn't enough to force an RC2 (or add to such force, IMO), but is a good ridealong if we are forced to take one.  The change has us use the same code as we used in FF2 for this case, which is also the same code we use for non-dense arrays.  Risk profile is low to very-low, with showers in the afternoon.

Comment 14

[Cyrus Omar]

(In reply to comment #4)
> Cyrus: can you point me at extant library code that's broken by this?  That'll
> help figure out the right course here WRT a ship vehicle for a fix.  I think
> that library authors generally try to avoid touching the prototype chain for
> Arrays and Objects because if the adverse effects on for/in, so I'd be a little
> surprised if there was wide use of getters and setters on the prototype chain
> for Arrays.  I have been surprised before, however. :)

You can actually transparently fix the for...in issues for Object/Array/String.prototype with creative use of __iterator__/Iterator (see http://cyology.com/?p=7).

I'm not sure how many libraries on the web at large use getters and setters, the only ones I know of involve adding IE functionality/compatible syntax to FF and I don't know off the top of my head if any of them require messing with Array.prototype. 

But extension authors and people building apps specifically for Mozilla users (such as myself) would like to use them, especially with the fix above. For example, I've been working on adding run-time documentation to JS objects for an in-browser REPL, so I do things like 

Array.prototype.__defineGetter__(doc.description /* namespaced so it doesn't cause conflicts */, function() "An Array containing " + this.length + " elements.")

Also, custom views of JS objects, i.e. 

Array.prototype.__defineGetter__(mvc.browserView, function() { 
  var span = document.createElement('span')
  span.innerText = "[" + this.toString() + "]"
  return span
})

It would be a hack to require special cases all over the place for Arrays, and if it resulted in unnecessary performance costs (setters that don't do anything probably shouldn't convert arrays to a slower form.) I'd probably end up bypassing the use of accessors for most things and use methods, which kinda negates the purpose of acccessors.

Comment 15

[Brendan Eich [:brendan]]

Uh, __count__ is old and non-standard, but please don't change it to reflect the number of holes pre-allocated for a dense array. That's just not useful.

/be

Comment 16

[Brian Crowder]

Brendan:  I didn't change it to do that, that's what it currently does.  The result comes from array_enumerate, but I didn't closely investigate why it's broken.  Another bug?

Comment 17

[Brendan Eich [:brendan]]

No, this bug. __count__ is the only user of the feature of OBJ_ENUMERATE whereby the count of enumerable properties is returned via *idp for JSENUMERATE_INIT.

/be

Comment 18

[Mike Shaver (:shaver emeritus)]

I don't think that's this bug -- getting __count__ wrong that way doesn't have anything to do with how we set |this| when calling an accessor, it's just something that crowder noticed in testing.

Agree that we should open another bug for it, perhaps set *idp to COUNT instead?

Comment 19

[Brendan Eich [:brendan]]

Oh, I see -- yeah, we should have a bug on the array_enumerate prob for sure.

/be

Comment 20

[Brian Crowder]

This is actually a relatively ugly regression, and allows programmatic detection of shavarrays (not sure how serious an issue that is), is there a specific reason we -don't- want it for RC2?  The fix is straightforward and relatively simple.

Comment 21

[Mike Shaver (:shaver emeritus)]

We're out of time here, and the risk of a further regression to arrays isn't worth fixing the edge case before 3.0.1.  We'll land it first thing after, and it'll be in the 3.0.1 train.  (Definitely need it before major update.)

Comment 22

[Brian Crowder]

Comment on attachment 321960 [details] [diff] [review]
OBJ_GET_PROPERTY(..proto..) becomes js_GetProperty(..obj..)

Was looking at this again in preparation for landing it and realized it's not right for JS_THREADSAFE builds (and therefore breaks the browser).  Need another spin.

Comment 23

[Brian Crowder]

Attachment: Lookup, then call js_NativeGet

Comment 24

[Brian Crowder]

This passes js/tests, trying mochitests shortly.

Comment 25

[Brian Crowder]

Comment on attachment 325047 [details] [diff] [review]
Lookup, then call js_NativeGet

As usual, my build is taking forever, so I'm hoping you can review this while I wait, and I will not land it until it passes mochitests.

Comment 26

[Brian Crowder]

This does pass mochitests.

Comment 27

[Brian Crowder]

Attachment: another approach

This is a competing attempt at fixing this same problem.  Needs much more testing.

Comment 28

[Brian Crowder]

Comment on attachment 325274 [details] [diff] [review]
another approach

This causes dozens of test-failures.

Comment 29

[pevans]

I'm having a similar problem with Array and prototype.  We have some code in an application which calls $("x").getElementsByClassName("y").  In this instance, an HTMLCollection is returned.  An HTMLCollection behaves much like an array, and we use it as such.

In IE 7, and Firefox 2, the HTMLCollection object was so much like an Array that you could use the extended Enumerable functions from prototype on it.  So, if a is the result of getElementsByClassName in the example above, you could write code like:

a.each(function(item) { ... });

Where "each" is one of the extended functions.  But in Firefox 3, "each" is no longer present in the HTMLCollection object.  It is still present in Array.prototype, and other array objects (e.g. [1, 2, 3]); just not in HTMLCollection.

I agree with others earlier--this is an annoying regression.  It's unfortunate that it's still extent in Firefox 3.

Comment 30

[Mike Shaver (:shaver emeritus)]

Comment 29 has nothing to do with this bug.  HTMLCollection never had Array.prototype in its prototype chain, but older versions of Prototype depended on an HTMLElement.prototype override being the right place to affect all elements, which has never been the case in Firefox (though it might be in the future).

http://ejohn.org/blog/getelementsbyclassname-pre-prototype-16/ has more information, including the Prototype team's recommendation on how to fix your use of Prototype.

Comment 31

[pevans]

Thanks for the clarification.

Comment 32

[Brian Crowder]

mrbkap:  Your thoughts on this bug, and the proposed patch, would be much appreciated.

In particular, I think shaver and I are not fully understanding why our lookupProp doesn't save us from having to do this.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #32)
> In particular, I think shaver and I are not fully understanding why our
> lookupProp doesn't save us from having to do this.

The only way that your lookupProp would help you is if you called it yourself. For regular JS objects, js_GetPropertyHelper is the function that actually calls resolve on the object in question, not the engine itself. This is why you need to duplicate the lookup/js_NativeGet pattern in your own getProperty object op (though not class hook). Therefore, I think I like attachment 325047 [details] [diff] [review] as a fix for this bug.

By the way, did anybody file a bug on array_enumerate returning the wrong count for JSENUMERATE_INIT?

Comment 34

[Brian Crowder]

Yeah, the count bug is bug 435226

Comment 35

[Brian Crowder]

Review-ping.

Comment 36

[Mike Shaver (:shaver emeritus)]

Comment on attachment 325047 [details] [diff] [review]
Lookup, then call js_NativeGet

r=shaver, another one that needs test coverage.

Comment 37

[Brendan Eich [:brendan]]

Comment on attachment 325047 [details] [diff] [review]
Lookup, then call js_NativeGet

Do we know obj2 comes back native? Generally want OBJ_IS_NATIVE before sprop = (JSScopeProperty *) prop. And if not native (or if general case), OBJ_DROP_PROPERTY to unlock. Stuck locks are bad...

/be

Comment 38

[Mike Shaver (:shaver emeritus)]

I missed that js_NativeGet re-locks after unlocking around the getter callout.  Not my finest review-day, clearly.  Thanks, brendan; I shall try to not further dishonour your legacy today. :/

Comment 39

[Brian Crowder]

Attachment: fixing bugs

Thanks for the careful review, Brendan.  Is this better?

Comment 40

[Brendan Eich [:brendan]]

Comment on attachment 327487 [details] [diff] [review]
fixing bugs

>+        if (prop && OBJ_IS_NATIVE(obj2)) {
>+            sprop = (JSScopeProperty *) prop;
>+            if (!js_NativeGet(cx, obj, obj2, sprop, vp))
>+                return JS_FALSE;
>+        }
>+
>+        if (prop)
>+            OBJ_DROP_PROPERTY(cx, obj2, prop);

No reason not to write

        if (prop) {
            if (OBJ_IS_NATIVE(obj2)) {
                ...
            }
            OBJ_DROP_PROPERTY(cx, obj2, prop);
        }

r=me with that.

/be

Comment 41

[Brian Crowder]

changeset:   15601:ee80c4447ae2

Comment 42

[Brian Crowder]

Attachment: the previous patch applied to the 1.9 branch

The only change is the filename (jsarray.cpp to jsarray.c)

Comment 43

[Brian Crowder]

Comment on attachment 327487 [details] [diff] [review]
fixing bugs

Don't need approval on this one; need approval on the updated one.

Comment 44

[Brian Crowder]

Attachment: woops, with context

Comment 45

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-434837-01.js,v  <--  regress-434837-01.js
initial revision: 1.1

mozilla-central: changeset:   16470:507b6a243711

Comment 46

[Daniel Veditz [:dveditz]]

Do we still want this for the 1.9.0 branch? If so please request approval on the patch.

Comment 47

[Brian Crowder]

Comment on attachment 329517 [details] [diff] [review]
woops, with context

Yeah, this should get into 1.9.0

Comment 48

[Daniel Veditz [:dveditz]]

Comment on attachment 329517 [details] [diff] [review]
woops, with context

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 49

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

Bob, is your regression test applicable to 1.9.0?

Comment 50

[Bob Clary [:bc] (inactive)]

Al, I think so but see bug 450275 comment 6. I'll verify this later today.

Comment 51

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

Bob, any luck?

Comment 52

[Bob Clary [:bc] (inactive)]

the 1.9.0 fix suffers from the same __count__ issues as the 1.9.1/1.9.2 branches in bug 450275 but otherwise is fine. 

brendan, rather than forking the test for 1.9.1/1.9.2 i'll just remove the __count__ parts of the tests for everyone and mark this as verified unless you object.

Comment 53

[Bob Clary [:bc] (inactive)]

v 1.9.0.6

for 1.9.1/1.9.2 issues, see Bug 474402