598996 - Remove restriction on redefining Array 'length' property

Status: RESOLVED DUPLICATE of bug 858381

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

DefinePropertyOnArray has an error case for 'length', which has a comment of the form (recent mozilla-central version, may be shifting around a bit due to other patches):

        /*
         * Our optimization of storage of the length property of arrays makes
         * it very difficult to properly implement defining the property.  For
         * now simply throw an exception (NB: not merely Reject) on any attempt
         * to define the "length" property, rather than attempting to implement
         * some difficult-for-authors-to-grasp subset of that functionality.
         */
This does not conform to ES5 and Jim asked for a bug on it.

/be

Comment 1

[Brendan Eich [:brendan]]

As Jim points out the only issue is a change from writable to not.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: draft patch

Waldo, I based this closely on the spec + erratum. The custom js.msg entry that's now a TYPEERR could probably be a generic can't-configure, please suggest a msg. Could use your advice on tests too.

/be

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 499444 [details] [diff] [review]
draft patch

js> var a = Object.defineProperty([], "length", { writable: false })
typein:1: TypeError: can't redefine non-configurable property 'length'

You're falling over on the "Non-standard:" comment in DefinePropertyOnObject, which is the optimization hazard to which I alluded as making this hard to implement correctly.  I think this applies to every possible way to redefine the "length" property which this patch seemingly purports to support at a glance.  Perhaps you could insert a special-case solely for [].length in DefinePropertyOnObject, although this would be a truly, truly awful hack.

At risk of being overly dogmatic or antagonistic (others should assent to or dissent from this proposition if they feel strongly one way or the other): I think it is good for the person writing the patch to write at least the first pass on tests.  It shouldn't take much to write tests that exercise at least the basic cases in the "length" definition algorithm through Object.defineProperty.  (It's not that much effort to write even a basic test per step, but I'm still at a point where I'll mostly take what I can get.)  Trickiness testing all the interpreter/methodjit special-cases for setting indexes and the length property itself is a bit harder, but just hitting one or two of the basic cases would be good.  To me writing tests is intrinsically part of fixing a bug or implementing a feature, and I believe it is hazardous in the long run for us hackers to not write our own tests for our own bugs and features.  So I'm requesting that you include tests in the next version of this patch.  They don't have to be ridiculously comprehensive, but they should at cover the basic cases and demonstrate that the patch works in at least *some* cases.

Comment 4

[Robert Sayre]

(In reply to comment #3)
> 
> At risk of being overly dogmatic 

Check!

A "draft patch" would seem to indicate that some required work would be missing.

Comment 5

[Jeff Walden [:Waldo]]

It's certainly reasonable a draft patch would fix some things but perhaps not others, or perhaps suggest an approach without claiming ultimate correctness or desirability.  But the "non-standard" rejection case is the heart of the conundrum which must be addressed in this bug.

Based on a few tests, I believe this patch always hits the "non-standard" rejection case in DefinePropertyOnObject when redefining "length", so it doesn't behave any differently from how we behave now.

Look, I know this is harsh.  But the lightest manual testing would have exposed the problem very quickly.

Comment 6

[Brendan Eich [:brendan]]

Waldo, you have to know when to dial it down. I threw this patch together while on a virtual Christmas vacation and asked for your feedback. Would feedback? have gotten a more balanced reply? Why should it matter?

Here's what is particularly unbalanced about your overlong comments here: *you* introduced the non-standard error and failed to implement ES5 fully. This is your bug. Have some humility, or at least civility, in rejecting partial patch gifts!

I took assignment, so I was going to write tests before too long. To assume from a "draft patch" that I was not ever going to write tests and lecture me on that straw man is even more galling. I'm tempted to assign this bug to you, but then it probably wouldn't get fixed for Firefox 4. Right?

/be

Comment 7

[Jeff Walden [:Waldo]]

I'm sorry, yes -- I do interpret r? differently than I do feedback?, and I hadn't realized this was effectively a feedback? (or nothing at all, I'd have commented on that assuming I noticed it amongst other bugmail).  It looked like a regular review request to me, not a strawman, partly for being r? and partly for the test request, even despite the "draft patch" designation.

But substantively -- special-case array_length_getter and array_length_setter in the "non-standard" special-case in DefinePropertyOnObject and I *think* you have this covered.  I suspect I'm still missing something on this point, a compliant hackaround can't be that simple.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: draft plus potential awful hackaround, minimal manual testing, touch with forty-foot poles only

I got too curious about how bad the awful hackaround I mused would be, despite it being long past sleepytime, so I had to churn something out building on the previous patch.  The result?  My inner Pythonista wants to go postal right now, but the result is roughly NS_SUCCESS_I_DID_SOMETHING:

[jwalden@find-waldo-now src]$ dbg/js
js> var a = Object.defineProperty([1, 2, 3], "length", {writable:false}); a.length = 4
4
js> a
[1, 2, 3]
js> 'use strict'; a.length = 3
3
js> 'use strict'; a.length = 4
typein:4: TypeError: can't define array length property
js> 'use strict'; a.length = 2
typein:5: TypeError: can't define array length property
js> a
[1, 2, 3]

Or:

[jwalden@find-waldo-now src]$ dbg/js
js> var a = [1, 2, 3];
js> Object.defineProperty(a, "length", { writable: false })
[1, 2, 3]
js> Object.getOwnPropertyDescriptor(a, "length")
({value:3, writable:false, enumerable:false, configurable:false})

Does it work any more than that?  No idea!  (resisting the urge to channel TMBG...)  And tracer's probably horribly out-of-sync with it.  And methodjit too?  And I probably just accidentally the whole performance.  But I guess it's proof-of-concepty enough to maybe run with it.  Maybe.

Comment 9

[Brendan Eich [:brendan]]

Attachment: draft, updated on top of patch for 591846

Comment 10

[Brendan Eich [:brendan]]

Blocks a softblocker, have patch (new one coming).

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: draft, needs tests and tryservering but so far so good

Comment 12

[Brendan Eich [:brendan]]

Attachment: this passes tests

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 508043 [details] [diff] [review]
this passes tests

Needs a bit more work in wake of bug 591846 followup patch.

/be

Comment 14

[Brendan Eich [:brendan]]

http://tbpl.mozilla.org/?tree=MozillaTry&rev=64fa59ba9130 for the patch I'm about to attach looks good to me. Phil?

/be

Comment 15

[Mike Shaver (:shaver emeritus)]

Win32 debug crash expected? doesn't look known to me.

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> Win32 debug crash expected? doesn't look known to me.

Not likely this patch -- no Object.defineProperty(foo, 'length', ...) in sight, and why windows only? I think I've seen that one before. Phil may know.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: rebased to tm tip

Comment 18

[Phil Ringnalda (:philor)]

Yeah, we need to do something about that message. In all of "PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)" there's only one word you are supposed to see, "normally." The "crashes" are just the normal way of killing off the child processes from the jsctypes tests, and the log was only orange because of the leak, which is bug 626956. Earns the Good Treekeeping Seal of Approval.

Comment 19

[Jeff Walden [:Waldo]]

One thing tests here should do is be wary of accessing an array's length *only* through JSOP_LENGTH.  a.length goes through very different code paths from a[(function() { return "length"; })()], so we probably want some sort of helper that checks both against an expected value.

Comment 20

[Brendan Eich [:brendan]]

Attachment: rebased again, more testing effort

I will keep working on the testcase (more suggestions welcome) but it would be good to start review. Thanks,

/be

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 508649 [details] [diff] [review]
rebased again, more testing effort

How long have we had arrays without length properties?  And what's the reason for this special case?  Do you know?  I don't understand why that path is "optimized", could you explain?  Seems like any place that assumes there's always a length property on an array is therefore wrong.  I see three places at least in this patch:

  "abcabc".match(/abc/).push(1, 2, 3);
  Object.defineProperty('abcabc'.match(/abc/), "length", { value: 0 });
  Object.defineProperty('abcabc'.match(/abc/), 5, { value: 17 });

And it's the sort of thing where the problem's likely to manifest itself in future work unless patch author and reviewer are aware of this wrinkle.  This is an utter special cases, so I wouldn't count on that happening.  If we have to keep the wrinkle around, I'd prefer JSObject::arrayLengthIsWritable() or something to at least unify the idiom in one place, to make it more likely to be chosen/understood, with comments in it explaining why it exists.

The test is checking exact error messages.  That requires the test be in an extensions/ directory so as not to break other engines using our tests, but then other engines wishing to use our tests will be left out cold (as they are if it's left where it is).  Check for |instanceof TypeError| or whatever instead.  This would solve the comment about error messages changing when bug 596728 is fixed.

Not to nitpick locations, but ecma_5/Array/ seems better to me since this is pure ES5 newness.

Rather than comparing stringified stuff, arrayEquals (copies of it are floating in different tests, could and should be copied into ecma_5/shell.js or wherever makes most sense) might be better, possibly augmented to account for holes with a |var HOLE_VALUE = {};| available for use in the arrayEquals call.

Your immutability tests are checking for different things than [].length being not writable.  Object.freeze() will mark the object as not extensible, which in some cases may well be an earlier check than for [].length not being writable (since in those cases you're adding a property).  Instead -- er, additionally, I should say! -- do all those forbidden ops on an array with [].length redefined to be non-writable, without using a freeze/seal shortcut.

This needs some trace tests for the various (non-native-method) failure cases.  What most worries me about the change is if the interpreter and tracer make potentially-invalid assumptions about [].length being writable.  It seems the only way to really guard against this is to go through all the potential property-setting ops and verify none make invalid assumptions regarding this point.  JSOP_INITELEM is one such pain point.  I didn't *see* any others on a quick read-through, but I could be mistaken.

I could be missing other places that need changing for this.  I went through the interpreter ops (although I'm a little sleepy right now, so I wouldn't trust that as the final word on interpreter correctness), but everything else could still use that fine-toothed check.  I'll do that on the next pass for sure, but please do one before as well, so we don't accidentally miss any places.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> How long have we had arrays without length properties?

What do you mean? If you mean arrays for which ('length' in a) is false, never. That would be a bad bug.

If you mean arrays for which !a.hasOwnProperty('length'), ditto. But what if you mean slow arrays that have no shape for 'length' linked from lastProp, we have had those since shavarrays.

Originally, years -- 'length' was shared permanent as you know. Then in bug 548671, you shadowed that Array.prototype length via the addProperty call in makeDenseArraySlow, as the comment in the patch says.

But even after bug 548671 was patched, we were using the old shavarray js_NewSlowArrayObject (now NewSlowEmptyArray) from several places to create slow empty arrays that had no 'length' shape and would not gain one later. See, e.g., jsregexpinlines.h, RegExp::createResult for one call to NewSlowEmptyArray.

Thus,

js> a = /b(.)/.exec("bc")
["bc", "c"]
js> a.length          
2
js> a.hasOwnProperty('length')
true
js> d = Object.getOwnPropertyDescriptor(a, 'length')
({value:2, writable:true, enumerable:false, configurable:false})

all works per spec, but a has no own 'length' property.

I'm not about to change anything about this here, for this bug. Bug 575997 is still open.

> If we have
> to keep the wrinkle around, I'd prefer JSObject::arrayLengthIsWritable() or
> something to at least unify the idiom in one place, to make it more likely to
> be chosen/understood, with comments in it explaining why it exists.

Will do, I'll try to make it not hide bugs.

> Check for |instanceof TypeError| or whatever
> instead.  This would solve the comment about error messages changing when bug
> 596728 is fixed.

Ok -- but we have lots of error message matching, already. Is there an effort to get rid of all the existing ones?

> Not to nitpick locations, but ecma_5/Array/ seems better to me since this is
> pure ES5 newness.

I went with the regression from ES5 and any place is fine (jit-test) policies but you are right.

> Rather than comparing stringified stuff, arrayEquals (copies of it are floating
> in different tests, could and should be copied into ecma_5/shell.js or wherever
> makes most sense) might be better, possibly augmented to account for holes with
> a |var HOLE_VALUE = {};| available for use in the arrayEquals call.

I'll try to do this.

> Your immutability tests are checking for different things than [].length being
> not writable. 

Good point -- it means more, and in a certain order. It does make existing length non-writable but if we are to smoke out shared-permanent leaks then it's better to defineProperty.

It may take a while to get this all done, but I'll keep at it. If you want to tag in, feel free.

/be

Comment 23

[Jeff Walden [:Waldo]]

(In reply to comment #22)
> What do you mean?
> 
> But even after bug 548671 was patched, we were using the old shavarray
> js_NewSlowArrayObject (now NewSlowEmptyArray) from several places to create
> slow empty arrays that had no 'length' shape and would not gain one later. See,
> e.g., jsregexpinlines.h, RegExp::createResult for one call to
> NewSlowEmptyArray.

I meant this.  Having an Array, a SlowArrayClass array, without a shape for the length property was something I absolutely didn't expect, save for the one comment that noted it.  When did this start?  Last I'd known, every array we created was either dense, with an implicit length property, slow with an explicit length property, or slow with a length property that would be lazily resolved into place if explicitly requested.  And what optimization is broken if we required every user of js_NewEmptySlowArray to define a length property before exposing it to non-array code?

> Ok -- but we have lots of error message matching, already. Is there an effort
> to get rid of all the existing ones?

I'm not aware of one.  It's certainly low priority at this exact moment to fix up existing tests.  But it's easy enough to not make the problem worse.  I've been writing tests with instanceof error-checking for quite some time now.

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> I meant this.  Having an Array, a SlowArrayClass array, without a shape for the
> length property was something I absolutely didn't expect, save for the one
> comment that noted it.  When did this start?

As comment 22 noted, it never ended. The patch for bug 548671 missed this.

>  Last I'd known, every array we
> created was either dense, with an implicit length property, slow with an
> explicit length property, or slow with a length property that would be lazily
> resolved into place if explicitly requested.

Resolved how? There are no resolve hooks on the classes defined in jsarray.cpp.

>  And what optimization is broken
> if we required every user of js_NewEmptySlowArray to define a length property
> before exposing it to non-array code?

First, that is not this bug.

Second, it's late to be monkeying with that stuff, which is tested by the stupid benchmarks.

Third, the right fix would define length first, in NewSlowEmptyArray, as the first shape, to maximize slow array shape sharing.

> > Ok -- but we have lots of error message matching, already. Is there an effort
> > to get rid of all the existing ones?
> 
> I'm not aware of one.  It's certainly low priority at this exact moment to fix
> up existing tests.  But it's easy enough to not make the problem worse.  I've
> been writing tests with instanceof error-checking for quite some time now.

Right-o.

/be

Comment 25

[Jeff Walden [:Waldo]]

(In reply to comment #24)
> >  Last I'd known, every array we
> > created was either dense, with an implicit length property, slow with an
> > explicit length property, or slow with a length property that would be
> > lazily resolved into place if explicitly requested.
> 
> Resolved how? There are no resolve hooks on the classes defined in
> jsarray.cpp.

"Last I'd known", as I said.  I looked more, and d'oh!  I was thinking of the function case, where the length property is resolved into place lazily.  I did not realize that, with the patch in bug 548671, I'd enabled arrays to have lengths but not have a length property structure!  I would have considered that a bug if I had noticed it.

> Second, it's late to be monkeying with that stuff, which is tested by the
> stupid benchmarks.
> 
> Third, the right fix would define length first, in NewSlowEmptyArray, as the
> first shape, to maximize slow array shape sharing.

How do benchmarks run faster because we have no length shape/property on arrays created in very particular ways?  It's not this bug.  We absolutely agree on that.  But I'd still like an answer, even if that answer is not directly relevant to this bug.

We have a case where a property on a native object seemingly exists, but nativeLookup(property) returns NULL: that's a wrong.  This wrong requires another wrong atop it to deal with that case, when no such care should be necessary.  I would like to know whether there is an identified issue that prevents us fixing the original wrong instead of compounding it.  The three people who've learned about this in the last couple days have all expressed considerable astonishment at this possibility after learning of or being told of it, which tells me we should not let this bugbear lie if killing it is straightforward and breaks no optimizations.

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #25)
> (In reply to comment #24)
> > Resolved how? There are no resolve hooks on the classes defined in
> > jsarray.cpp.
> 
> "Last I'd known", as I said.

Let's look at code, not go back in forth in bugs based on faulty memory (mine is aging every day).

> I looked more, and d'oh!  I was thinking of the
> function case, where the length property is resolved into place lazily.  I did
> not realize that, with the patch in bug 548671, I'd enabled arrays to have
> lengths but not have a length property structure!  I would have considered that
> a bug if I had noticed it.

Would you bug yourself on this? I'd like to see that fix separate from this one. We can make this depend on that, for sure.

The "optimization" may not matter, and we can find out at leisure. The point here is not to cross the streams. This bug's patch can rebase after the length-for-all bug is fixed.

Neither bug (this or the one you'll file) seems like a blocker, unless you have a testcase showing something bad.

/be

Comment 27

[Jeff Walden [:Waldo]]

That's fine.  Filed bug 631098, posted patch, r?you.

Comment 28

[David Bruant]

I don't know if it's the right place, but there are a lot of test failures (http://samples.msdn.microsoft.com/ietestcenter/Javascript/ES15.2.html) regarding calling Object.defineProperty on arrays. Tests are ranging from 15.2.3.6-4-117 to 15.2.3.6-4-189. Individual files can be found here: http://hg.ecmascript.org/tests/test262/file/f25942ef2292/test/suite/ietestcenter/chapter15/15.2/15.2.3/15.2.3.6

With the hope it helps.

Comment 30

[Brendan Eich [:brendan]]

One thought now that shared-permanent is gone: could we make array length be a data property with well known first slot, and update it from an appropriate ObjectOps hook as ECMA-262 does with custom [[DefineOwnProperty]].

Then life would be pretty good.

/be

Comment 31

[Mark S. Miller]

It apparently works to freeze an array as a whole, in which case the 'length' property appears to be non-configurable non-writable data property, as expected. However, even then, trying to defineProperty the length property to non-writable fails. Can this annoyance be fixed now? It is responsible for the only remaining "skip" entry in the SES whitelist

http://code.google.com/p/es-lab/source/browse/trunk/src/ses/whitelist.js#259 (as of this writing)

Comment 32

[Mark S. Miller]

> Object.defineProperty(Object.freeze([]), 'length', {writable: false})
InternalError: defining the length property on an array is not currently supported

Comment 33

[Brian O'Keefe [:bokeefe]]

Attachment: Updated patch, to account for m-c changes

I had a bit too much free time on my hands, so I updated the previous patch to apply to the current m-c tip. All of the tests are passing locally, plus a huge swath of the test262 Ch15 tests (everything with a name starting with "Object.defineProperty - 'O' is an Array").

I think I've covered everything that was mentioned in comment 21, except for potential pitfalls with the interpreter/tracer.

Also, I wasn't sure what to do about Object-apply-02.js. It was assuming that it could call Array.prototype.push on a string, but that throws now, because length is read-only on strings. The spec is hand-wavy about allowing that (http://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.7).

Comment 34

[Brian O'Keefe [:bokeefe]]

Attachment: Updated patch

It looks like other browsers are quite okay with "push" on strings, so I changed it to only throw for arrays.

Comment 35

[Mark S. Miller]

(In reply to Brian O'Keefe from comment #34)
> It looks like other browsers are quite okay with "push" on strings, so I
> changed it to only throw for arrays.

What does this mean? Can you give an example of a push on a string?

Comment 36

[Brian O'Keefe [:bokeefe]]

(In reply to Mark S. Miller from comment #35)
> What does this mean? Can you give an example of a push on a string?

Something like this:
> str = "hello"
"hello"
> Array.prototype.push(str, "world")
2
> str
"hello"
> s = Object(str)
(new String("hello"))
> Array.prototype.push(s, "world")
4
> s
(new String("hello"))

So, push on a string doesn't actually do anything, but it also doesn't throw a TypeError. That's what happens now in Firefox, so this is really just preserving the existing behavior on strings.

Comment 37

[Mark S. Miller]

Not throwing looks like a Firefox bug to me. Please check the ES5 spec. If the ES5 spec does not require a throw, I suspect that is a spec bug we should fix in ES6.

Comment 38

[David Bruant]

(In reply to Brian O'Keefe from comment #36)
> (In reply to Mark S. Miller from comment #35)
> > What does this mean? Can you give an example of a push on a string?
> 
> Something like this:
> > str = "hello"
> "hello"
> > Array.prototype.push(str, "world")
> 2

It seems to me that you're applying Array.prototype.push on Array.prototype (which happens to be an Array with a length) and not on the string. Hence the '2' in the console (which is the new Array.prototype length) and the later '4'. You just added elements to Array.prototype.
I believe there is a .call missing (but the end result is the same even with the .call)


(In reply to Mark S. Miller from comment #37)
> Not throwing looks like a Firefox bug to me. Please check the ES5 spec. If
> the ES5 spec does not require a throw, I suspect that is a spec bug we
> should fix in ES6.
First step of Array.prototype.push is coercing |this| into an object. Firefox is doing what the spec asks for from what I can tell, though it's a bit dumb (because the object generated after the first step is never reachable from user code!).
Although I'd prefer an error thrown, I have high doubts it'll be possible to change the behavior in ES6 for test262/break-the-web reasons.

Comment 39

[Jeff Walden [:Waldo]]

Comment on attachment 727149 [details] [diff] [review]
Updated patch

Review of attachment 727149 [details] [diff] [review]:
-----------------------------------------------------------------

These days we have semi-separate storage space for elements ("0", "1", ..., "4294967295") and for all other properties.  So implementing this isn't quite enough -- we also have to futz a bunch with the property-setting and property-definition code paths, to make sure they properly implement non-writable length semantics.  Also all the JITs need the same sort of updates.

I've started working on a patch doing that, and on the rest of the stuff involved in all this.  It's going to be a pile of awful hacks, and I don't expect it to be clean or well-understood, but hopefully it'll get us there.  More as I continue working on this.

Comment 40

[Mark S. Miller]

Now that https://bugzilla.mozilla.org/show_bug.cgi?id=858381 is fixed, does that in any way change the status of this bug?

Comment 41

[Jeff Walden [:Waldo]]

Yeah, that bug basically *was* this one, but with less ancient noise to it -- making this a duplicate.