Yes, this is a duplicate, but the existing bug (maybe even bugs) were getting a bit crowded, with comments and obsolete patches both. And my current patchwork has approximately nothing to do with the work in either of those bugs, so it seems nice to keep separate work separate.

This passes all jit-tests and jstests, with appropriate test changes, which obviously means our existing tests aren't very comprehensive here. (No surprise, seeing as it's hard to write correct tests without having an implementation, even a buggy one, to run them against to verify some semblance of sanity. Running algorithms mentally is hard, yo.) There's basically no new tests for non-writable array lengths yet. No idea how this does on test262. I'd bet pretty good, but probably with some obvious bugs that still need fixing. The big task now (beyond more testcase-writing) is probably auditing all the JIT ICs to ensure they handle non-writable length correctly.

I think I have everything done, functionality-wise. My mq is a sprawling mess of tests intermixed with spot-changes to the JITs in various places (I generally tried to write a test that failed for each JIT-change I made, then verify it was fixed with a spot-change). It's going to take me awhile to properly massage it into some sort of readable, vaguely reviewable series of patches. (Also the patches are atop a few other changes, so there's some semi-unrelated changes amongst this that would be a royal pain to disentangle.) At one point or another it passed all the tests I included here, but I've been doing so much pushing, popping, and removing that I make no guarantees about the final state. ;-) I'm out of time for the day, so I can't do any of that cleanup just now. But in the meantime, I think we can find out what else needs doing still with the fuzzers. Gary, any chance you could look at this (it's against inbound) and see how quickly it blows up?

Comment on attachment 735518 [details] [diff] [review] Rollup patch of everything for fuzzing Inbound rev is currently cc4abdf71cb8, will test this now.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2) > Created attachment 735518 [details] [diff] [review] > Rollup patch of everything for fuzzing x = []; Object.defineProperty(x, 1, {}); for (var y = 0; y < 9; y++) { x.length = 1 } Assertion failure: obj->nativeLookup(cx, shape->propid()) == shape, at jsinterpinlines.h

Testing this now, expect results tomorrow or Friday (I'm on a conference, so it could take a bit).

Comment on attachment 735518 [details] [diff] [review] Rollup patch of everything for fuzzing I haven't found any more failures during fuzzing :)

Excellent! I've been busy combining patch hunks with the relevant tests, removing XXX comments and other hackwork in the patch code, and reintroducing the pre-existing optimizations I removed (the first time through was as by-the-spec as possible). I'm almost finished with the last bit of pre-existing optimization reintroduction. Once that's done, I'll post another rollup patch for more fuzzing, to catch anything the previous passes missed. Then it'll be on to perf-testing the usual awfy suspects. I expect patchbombing to commence tomorrow.

Nicolas just made a clever observation about how to eliminate the extra range-check in the JIT paths: trim the capacity to exactly the length, when the length is made non-writable. The existing index < capacity checks will then subsume a new index < length or length-is-writable check, and so *in theory* no new checks need to be added to the JIT code. That said, there might be places that require capacity to be faithful to the size of the actual memory (for example, memory reporting stuff might trust the capacity to not underestimate, conceivably), so there could be some slight trickiness to this. Definitely a thing to follow up on quickly, if this first patch passes perf muster -- and if it doesn't, that's pretty clearly the thing to do to gain back the perf.

https://tbpl.mozilla.org/?tree=Try&rev=c020441c9470 Okay, this fixes the relatively obvious assertion noted before. It also reintroduces the optimizations present in the current algorithms, so that this shouldn't change perf behavior inside the VM operations. I still need to do benchmarking against awfy stuff to see how it does there, and depending how that goes I want to investigate the capacity hack noted in the previous comment. But this is ready for another go-around on fuzzing, at least. This is still in the all-in-one stage, but if you're curious about vaguely reasonable breakdowns, and what the patch series will look like in basic form when I post it, the try push has it all in individual patches. Not necessarily what you'll see in the end, but pretty close to it, I bet.

Bug 858677 is needed before this lands to implement trailing-element deletion semantics correctly.

x = []; Object.defineProperty(x, 4, { configurable: true, }); for (var y = 0; y < 2; y++) { x.length = 0 } Assertion failure: obj->nativeLookup(cx, shape->propid()) == shape, at jsinterpinlines.h with the second rollup patch in comment 9.

function f() { x.length = 7 }; x = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]; Object.defineProperty(x, 13, { set: (function() { for (var j = 0; j < 9; j++) { Proxy.createFunction({}, f)() } }) }); Array.prototype.shift.call(x); Assertion failure: obj->nativeLookup(cx, shape->propid()) == shape, at jsinterpinlines.h also with the second rollup patch in comment 9.

The first test had the glimmmerings of the underlying issue, had I the wit to see it. These two tests both point out the rest of the issue -- that |shape| as initially looked up in baseops::SetPropertyHelper is invalid after ArraySetLength, because that function can (in several ways) change the length shape of the object. These two tests do it by switching the object to dictionary mode. On second glance I see other possible avenues for this to happen -- through the ToUint32/ToNumber calls to determine the new array's length. I'm not sure offhand that the rest of ArraySetLength (in our implementation, not the spec) is sound against changes to the "length" property's attributes, so I'll need to investigate that before having a final patch. Any issues from this as-yet-unfixed avenue are likely to show up when assigning to an array's length (or Object.defineProperty-ing it) with non-numeric values, e.g. { valueOf: function() { return 17; } }. (Except the bad cases are likely to have that valueOf function, or a toString function, mutating the array on which the length property's set. But given that issue may require a little finickiness to trigger, this is probably good for more testing. Just be aware of the above issue, and that it's known.

Comment on attachment 736979 [details] stack Obsoleted with the 3rd rollup patch fix.

Actually, looking closer, I don't think the "second glance" issue I note in comment 13 is valid -- the spec algorithms do all the ToUint32/ToNumber length computation before referring to any of the attributes of the existing length property. And because js::ArraySetLength tries to parallel that algorithm as closely as possible, it does likewise, and thus sidesteps the issue. So I think this might really be good to go for all-out fuzzing.

Kraken results seem to vary, pre-patch and post-path, in the 1640ms-1660ms range. It's unclear what, if any, difference there is. Perhaps some, perhaps none. Octane results seem to be ~13700 before and ~13750 after. I'm not sure how this could be a perf *improvement*, exactly, or whether to trust that an increase supposedly occurred. It's plausible that some optimization or other got slightly broadened enough to do this, maybe. The only thing I can think of is that I made the [].push fast-path work for any number of arguments, not just one, but I doubt benchmarks are pounding that or we'd have done it already. I dunno, tho, I'm kind of stretching to explain anything here. Sunspider results are steady at ~166ms. Given all this, I could go either way on posting my current patch series, without trying the trim-capacity-to-nonwritable-length trick. JIT code has a high enough cost (immediate and ongoing) to it that I think I'm going to do the trim-to-capacity thing now, to reduce the reviewer cost (and cost to reading the JIT generation code, until the trick gets added). Plus Luke agrees. So I'm going to do that now, and then hopefully post the whole patch series. :-)

https://tbpl.mozilla.org/?tree=Try&rev=8e8cdd844d58 The code's all sorts of ipse dixit. (You can see the important parts of change in the capacity-hack.diff revision, and the removals of most of the JIT changes in remove-jit-changes.diff revision.) But that notwithstanding, I think this correctly implements the capacity <= length invariant for arrays with non-writable length, and thus removes nearly all the extra JIT changes I'd made. (There are still a few that are required for inlining of [].pop and [].shift -- because these *reduce* length, and therefore the capacity trick can't be used to eliminate the check.) This patch is the most likely one to land, so it's the one that needs fuzzing most. I did see the patch queue pass jit-tests --tbpl and jstests in a debug build -- with never-propcache-length.diff, and then remove-jit-changes.diff a second time, as the revisions I tested. So I'm reasonably confident. But I could still be missing something obvious. If this rollup crashes and burns, please consider the feedback? on the previous rollup to as a conditional request. :-) If this passes the try gauntlet, I'll probably start posting patches Saturday or Sunday. The don't-check-non-writable-in-JIT portion is a pretty subtle patch. I'm not sure if I'll keep it separate from everything, or integrate it into the previous portions of the patch. The nice thing about the previous work is it points out all (I think) of the places that try to mutate the length of an array in JIT code. Not even pointing them out, as would happen if I merged it into the earlier patches, seems unideal to me. So I may do the JIT-changes-undoing in a separate patch, notwithstanding that it'd be a smaller set of patches overall if I didn't have it separate.

It's perhaps worth noting specially that in the capacity <= length invariant for arrays with non-writable length part, I have some moderately fugly code that's doing its darndest not to screw up vaguely-latent requirements whose full import I only somewhat managed to parse out of the existing code. Of all the patchwork that's going to happen here, that bit is quite possibly the absolute hairiest. Which may be saying something.

Belatedly answering some questions on IRC last night: > jwalden: bhackett: let's retry: what code, if any, depends on the capacity of an array having its current value, as opposed to a smaller value (but one still >= initializedLength)? > jwalden: bhackett: I see "Don't shrink elements below the minimum capacity" in shrinkElements, but there's no clear referent to what's meant The comment in shrinkElements is really just parroting some ancient heuristic that existed in the code for dense arrays way back when and has survived to the present day. The capacity of a native object is permitted to be any value >= its initialized length and <= the actual amount of allocated space. We already depend on this property when seal()'ing, freeze()'ing or watch()'ing an object. These are cases where we want all attempts to modify the object to flow through paths in the VM, so rather than modify jitcode to check for these bits we just force these objects to have a dense capacity of zero. The code for that is in JSObject::sparsifyDenseElements. The same thing could be done for arrays with non-writable lengths, but I think that lowering the capacity to the nominal length is fine too and code will run much faster. (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8) > That said, there might be places that require capacity to be faithful to the > size of the actual memory (for example, memory reporting stuff might trust > the capacity to not underestimate, conceivably) Memory reporting use a JSMallocSizeOfFun callback that asks the underlying memory allocator how much memory is used for a given heap allocation. See JSObject::sizeOfExcludingThis. This allows internal fragmentation etc. to be accounted for and means that the value of the capacity is ignored for memory reporting purposes.

JSObject::growElements' conditionally asserting |newcap < getArrayLength()| is pretty easy to make fail, but I guess my existing tests here didn't manage to hit that case. Just an assertion botch, no other particular changes of note in this rollup. I considered responding to the capacity comments in the previous comment in this patch, but as a response would be slightly speculative, it seems best for everyone's time to not do that just yet. Assuming fuzzers are happy with this, I'll probably do it just prior to posting patches for review, given the level of confidence in that comment.

Comment on attachment 737000 [details] [diff] [review] Rollup that never property-caches the array length property Guessing that this is now obsolete..

Comment on attachment 737113 [details] [diff] [review] With one fewer stupid assertion fenceposting error Nothing found over the weekend -> feedback+.

Just a little moving to first use, to help readability and hacking in of new code later.

This patch on its own isn't a complete fix, but I think it fixes all the VM portions of a full fix. It doesn't do the capacity <= length invariant noted before. That's in the next patch, kept separate for readability.

For simplicity I implemented everything as stupidly as possible, at first. This reintroduces some of the optimizations removed in the course of patching.

This patch was required before I wrote the capacity-hack patch. I have a few different patches for JIT changes, with associated tests, left over from before the capacity hack got written. The JIT changes will mostly be reverted in a final patch atop all this that begins to rely on the |capacity <= length| invariant, but I thought it worth pointing out all the places that needed changes for the simpler system, even if the simpler system isn't what'll eventually be used.

The fix for this bit of methodjit code was simple enough to just make, rather than disable that particular methodjit optimization. This still matters as long as jit-tests --tbpl runs with methodjit.

InitArrayElements sweeps up a bunch of different methods, as I recall, but the slower push was possibly the most visible one.

I probably could have fixed the code, but I couldn't for the life of me get a test that failed without the changes, to verify change correctness, so I punted. :-\

The Ion change is obviously a temporary state. :-) Pretty sure the next patch makes that code look more correct.

Yup, the goofy Ion code changes in this patch. It's worth noting that this (and the methodjit change to not inline [].pop, I think) is the only bit of JIT code change that's fully necessary after the patch here that makes us depend on the |capacity <= length| invariant for non-writable arrays, guaranteed by the previous patch here.

The only JIT changes that remain are for inlining Array.prototype.pop and Array.prototype.shift, which attempt to decrease the length of the array, and therefore aren't addressed by capacity comparisons.

Comment on attachment 737554 [details] [diff] [review] 3 - VM changes for non-writable length, including writes to out-of-bounds indexes, plus test updates jimb, could you look at the Object-apply-02.js changes only? I kind of cargo-culted and faked it til it worked. :-)

Comment on attachment 737565 [details] [diff] [review] 11 - Stop inlining [].push/pop/shift in the methodjit to not violate non-writable array length guarantees Review of attachment 737565 [details] [diff] [review]: ----------------------------------------------------------------- Agreed this is the right fix. With the default prefs JM no longer compiles anything and even if we have to turn it on this is unlikely to matter much with Ion. ::: js/src/methodjit/FastBuiltins.cpp @@ -968,5 @@ > - types::OBJECT_FLAG_LENGTH_OVERFLOW | > - types::OBJECT_FLAG_ITERATED) && > - !types::ArrayPrototypeHasIndexedProperty(cx, outerScript)) { > - bool packed = !thisTypes->hasObjectFlags(cx, types::OBJECT_FLAG_NON_PACKED); > - return compileArrayPopShift(thisValue, packed, native == js::array_pop); Nit: can you also remove compileArrayPopShift and compileArrayPush, to avoid confusion?

compileArrayPopShift got fixed up in attachment 737569 [details] [diff] [review] and attachment 737571 [details] [diff] [review] (patches 12/13 here). compileArrayPush got fixed in attachment 737563 [details] [diff] [review] (part 9, the first one ;-) ). I assume since they're present in Ion (and JM before), there's some good reason for it, and they should stick around generally. :-) With attachment 737556 [details] [diff] [review] (part 4) the array_push un-inlining in methodjit isn't actually necessary -- it gets reverted in attachment 737576 [details] [diff] [review] (part 16). The pop/shift un-inlining doesn't disappear, tho, as |capacity <= length| doesn't help eliminate checks on array-length reductions.

This probably includes the prereqs my patch needs, but I don't know how different m-c is from m-i to know for sure that it applies.

Everything looks parallel in a debugger up til the native call, so I think this is an adequate test, and the assertion should be a good canary if anything ever changes.

Comment on attachment 737113 [details] [diff] [review] With one fewer stupid assertion fenceposting error Haven't seen more failures other than the one filed as a blocking bug here :)

Comment on attachment 737554 [details] [diff] [review] 3 - VM changes for non-writable length, including writes to out-of-bounds indexes, plus test updates Review of attachment 737554 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsarray.h @@ +82,5 @@ > + * warning or an error was reported (or nothing at all). > + */ > +extern bool > +DefinesPastNonwritableArrayLength(HandleObject obj, uint32_t index, bool strict, JSContext *cx, > + bool *ret); It would be nice if the return value and 'ret' were switched here, to be more in keeping with how this sort of method is written elsewhere in the engine, i.e. the return value indicates exception state. It looks like that would require a bit more code at call sites to this function but I think the consistent interfaces would be worth it (don't need an extra paragraph to explain the interface).

Comment on attachment 737556 [details] [diff] [review] 4 - Adjust non-writable arrays' capacity so |capacity <= length| is an invariant for arrays with non-writable length Review of attachment 737556 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsarray.cpp @@ +620,5 @@ > + // handle this with a check for non-writable length in most places. > + // But in JIT code every check counts -- so we piggyback the check on > + // the already-required range check for index < capacity by making > + // capacity of arrays with non-writable length never exceed the length. > + do { I think this whole do-while loop should be replaced with: if (obj->getDenseCapacity() > newLen) { obj->shrinkElements(cx, newLen); obj->getElementsHeader()->capacity = newLen; } This follows what JSObject::sparsifyDenseElements does in a similar scenario and better encapsulates what's going on. ::: js/src/jsobj.cpp @@ +2755,5 @@ > + // length. See also js::ArraySetLength which initially enforces this > + // requirement. > + newheader->capacity = newheader->hasNonwritableArrayLength() > + ? newcap > + : actualCapacity; I think this would read better if this change was removed and a block like below was added after other modifications to actualCapacity. if (isArray() && !arrayLengthWritable()) actualCapacity = newcap; Or just initialize actualCapacity to newcap in the non-writable length case. ::: js/src/vm/ObjectImpl.h @@ +915,5 @@ > + * related is when the object is an array with non-writable length. In this > + * case the capacity is always less than or equal to the length. This permits > + * JIT code to optimize away the check for non-writable length when assigning > + * to possibly out-of-range elements: such code already has to check for > + * |index < capacity|, and fallback code can easily handle non-writable length. s/can easily handle/checks for/

Comment on attachment 737558 [details] [diff] [review] 6 - Fix store-to-element-that-might-be-a-hole Ion and baseline codegen Review of attachment 737558 [details] [diff] [review]: ----------------------------------------------------------------- It looks like this entire patch (except the test) is reverted by patch 16?

Comment on attachment 737559 [details] [diff] [review] 7 - Fix the methodjit setelem code to work with non-writable array lengths Review of attachment 737559 [details] [diff] [review]: ----------------------------------------------------------------- This change is reverted by patch 16.

Comment on attachment 737563 [details] [diff] [review] 9 - Change the push fast-path to work for any array with writable length, make push work on all other values, and fix Ion's optimized push Review of attachment 737563 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/CodeGenerator.cpp @@ +4127,5 @@ > + // won't set the element in this case (and will throw a TypeError in > + // strict mode code). > + Address elementFlags(elementsTemp, ObjectElements::offsetOfFlags()); > + Imm32 bit(ObjectElements::NONWRITABLE_ARRAY_LENGTH); > + masm.branchTest32(Assembler::NonZero, elementFlags, bit, ool->entry()); This looks like it is reverted by patch 16. In the future, can you hand edit the patches or something to remove changes that are reverted later in the series? ::: js/src/jsarray.cpp @@ +1938,5 @@ > + obj->arrayLengthIsWritable() && > + !ObjectMayHaveExtraIndexedProperties(obj)) > + { > + uint32_t length = obj->getArrayLength(); > + uint32_t count = args.length(); Maybe rename 'count' to 'argCount' or somesuch, given the similarly generic 'length' immediately above?

I figured making the changes, then reverting them later, would make clear that all the relevant places where lengths were updated, were considered in the course of all changes. Simply not making any JIT changes, except to shift/pop code, didn't seem nearly clear enough to be confident in all the right places having been considered and changed/not changed appropriately. And yes, those two parts are reverted fully in part 16.

For some strange reason the previous test was failing -- only on Linux -- and complaining about expect being "bailout" rather than "success". Not sure why that's the case. Anyway, I rewrote a bit and hopefully fixed that issue, and maybe even made the test a little clearer. Let me know if I did anything horribly stupid here. :-)

Comment on attachment 740540 [details] [diff] [review] Better parallel test Review of attachment 740540 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, I don't know why the previous test would have such unpredictable behavior, but this test looks fine.

Comment on attachment 737561 [details] [diff] [review] 8 - Fix up the Array.prototype.push code to handle non-writable length in "slow" cases Review of attachment 737561 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/arrays/push-slowly-loopy-nonwritable-length.js @@ +9,5 @@ > + // Create an array of arrays, to be iterated over for [].push-calling. We > + // can't just loop on push on a single array with non-writable length because > + // push throws when called on an array with non-writable length. > + var arrs = out.arrs = []; > + for (var i = 0; i < 100; i++) Global "var N = 100" please, since this is used both here and below... incidentally, why such a large number? Does it ordinarily take a really long time for Ion to stabilize? @@ +35,5 @@ > + throw new Error("didn't throw!"); > +} > +catch (e) > +{ > + assertEq(e instanceof TypeError, true, "expected TypeError, got " + e); load(libdir + "asserts.js"); ... var obj = {}; assertThrowsInstanceOf(() => test(obj), TypeError); This also allows you to declare a and arrs where they're first assigned. ::: js/src/jsarray.cpp @@ +1137,5 @@ > if (obj->shouldConvertDoubleElements()) > break; > > + if (start + count > obj->getArrayLength() && !obj->arrayLengthIsWritable()) > + break; Swap the order of the two halves here? I think it reads better that way, and the second condition is less likely. The first half (start + count > obj->getArrayLength()) is repeated in a later if-statement in this function. The C++ compiler can common that up though. I don't see a way to eliminate the new branch.

Comment on attachment 737571 [details] [diff] [review] 13 - Fix [].pop to respect non-writable array lengths Review of attachment 737571 [details] [diff] [review]: ----------------------------------------------------------------- Why delete the fast dense path in this patch? It seems like it could be retained without any extra work, just add an "&& obj->arrayHasWritableLength()" to the guard. (?) Clearing review flag for now because of that issue. I'll have to get to the rest of this tomorrow. Please add this test: assertEq(Object.freeze([]).pop(), undefined); // shouldn't throw ::: js/src/ion/CodeGenerator.cpp @@ +4058,5 @@ > + // Handle the failure case when the array length is non-writable in the > + // OOL path. > + Address elementFlags(elementsTemp, ObjectElements::offsetOfFlags()); > + Imm32 bit(ObjectElements::NONWRITABLE_ARRAY_LENGTH); > + masm.branchTest32(Assembler::NonZero, elementFlags, bit, ool->entry()); This means we can add the corresponding assertion to js::ion::ArrayPopDense (and ArrayShiftDense), right? ::: js/src/jit-test/tests/arrays/ion-pop-nonwritable-length.js @@ +20,5 @@ > + > + for (var i = 0, sz = arrs.length; i < sz; i++) > + { > + var arr = arrs[i]; > + f(arr); Why not f(arrs[i])? Are we trying to defeat some JIT optimization here? Does it really work? @@ +34,5 @@ > + throw new Error("didn't throw!"); > +} > +catch (e) > +{ > + assertEq(e instanceof TypeError, true, "expected TypeError, got " + e); Same assertThrowsInstanceOf comment.

(In reply to Jason Orendorff [:jorendorff] from comment #57) > Why delete the fast dense path in this patch? It seems like it could be > retained without any extra work, just add an "&& > obj->arrayHasWritableLength()" to the guard. (?) The separate dense/slow methods were nigh-indistinguishable. Having two versions seemed pointless for the four (count 'em) differences between them. 1. GetLengthProperty versus getArrayLength: 2. "Optimizing" for our not supporting non-writable array length when length == 0. 3. If denseInitializedLength > index, setDenseInitializedLength(index). 4. SetLengthProperty versus JSObject::setArrayLength. Given Ion-compilation of pops, I'm straining to see the value of having two separate implementations given the minuscule optimization opportunities afforded here. GetLengthProperty has a fast path for getArrayLength already. #3 we can just always do now, actually, so no need for a fast path method just for it. 2 and 4 seem like pretty minimal optimizations to me to make when the length is writable. > Please add this test: > assertEq(Object.freeze([]).pop(), undefined); // shouldn't throw The last argument passed to [[Put]] here in ES5 is true, so the length-set will throw a TypeError even tho it's setting to the current value. So this should throw a TypeError. I added a test. > ::: js/src/ion/CodeGenerator.cpp > @@ +4058,5 @@ > > + // Handle the failure case when the array length is non-writable in the > > + // OOL path. > > + Address elementFlags(elementsTemp, ObjectElements::offsetOfFlags()); > > + Imm32 bit(ObjectElements::NONWRITABLE_ARRAY_LENGTH); > > + masm.branchTest32(Assembler::NonZero, elementFlags, bit, ool->entry()); > > This means we can add the corresponding assertion to js::ion::ArrayPopDense > (and ArrayShiftDense), right? Am I interpreting correctly: you want an assertion that the length isn't writable in those functions? The out-of-line path gets used not just for this case where !obj->arrayLengthIsWritable() but also for cases like (gussied up to be Ion-compiled): var arr = [1, 2, 3]; arr.length = 4; arr.pop(); So such an assertion wouldn't hold. > > + for (var i = 0, sz = arrs.length; i < sz; i++) > > + { > > + var arr = arrs[i]; > > + f(arr); > > Why not f(arrs[i])? Are we trying to defeat some JIT optimization here? Probably test-copy/paste detritus, I dunno. Will change. I added a couple other tests here to respond to the minor changes in this patch. I don't think any of them failed before, or even failed at intermediate stages in the changes here, but I almost fooled myself into thinking they did. And these tests have low enough overhead I don't see a reason not to add them. As for why N=100 earlier, no particular reason. Just wanted a nice big number so it'd always hit Ion eventually, and I wanted to be way past any heuristic number.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #58) > Given Ion-compilation of pops, [...] I was completely misinterpreting the code in CodeGenerator::emitArrayPopShift. I think this means js::ion::ArrayPopDense is a terrible name. I also misinterpreted the description of patch 11. Bad day for understanding things yesterday. No worries. Ignore the whole assertion thing too. > The last argument passed to [[Put]] here in ES5 is true, so the length-set > will throw a TypeError even tho it's setting to the current value. So this > should throw a TypeError. I added a test. Oh, right. [[CanPut]] returns false. So but then: Object.defineProperty(Object.freeze([]), "length", {value: 0}); // don't throw Is there a test for that?

Hmm, maybe there weren't length-redefinition tests yet. Here are some, hitting a bunch of edge cases. (And yes, I verified the numbers in the comments.)

Comment on attachment 737573 [details] [diff] [review] 14 - Fix [].unshift to respect non-writable lengths Review of attachment 737573 [details] [diff] [review]: ----------------------------------------------------------------- Same comments about the test.

Comment on attachment 737554 [details] [diff] [review] 3 - VM changes for non-writable length, including writes to out-of-bounds indexes, plus test updates Review of attachment 737554 [details] [diff] [review]: ----------------------------------------------------------------- This is going to take me a day or so to digest. Some initial comments: ::: js/src/jit-test/tests/arrays/length-set-after-define-nonconfigurable.js @@ +1,4 @@ > +var arr = [1]; > +Object.defineProperty(arr, 1, { value: undefined, configurable: false }); > +for (var y = 0; y < 9; y++) > + arr.length = 1; This test really needs some comments. Why nine times? (I have to admit, I had to read carefully before I realized that the assignment to .length would try to truncate a non-configurable property.) ::: js/src/jit-test/tests/arrays/length-set-reduce.js @@ +1,4 @@ > +var arr = [1]; > +Object.defineProperty(arr, 1, { value: undefined, configurable: false }); > +for (var y = 0; y < 9; y++) > + arr.length = 1; This test looks identical to length-set-after-define-nonconfigurable.js. ::: js/src/jit-test/tests/debug/Object-apply-02.js @@ +46,5 @@ > + : push.call("hello", "world"); > + assertEq("throw" in cv, true); > + var ex = cv.throw; > + assertEq(ex instanceof Debugger.Object, true); > + assertEq(ex.proto, frame.eval("TypeError.prototype").return); If you're going to frame.eval, why not: AssertEq(frame.evalWithBindings("ex instanceof TypeError", { ex: ex }), true)

Comment on attachment 737575 [details] [diff] [review] 15 - Fix Array.prototype.splice to respect non-writable lengths Review of attachment 737575 [details] [diff] [review]: ----------------------------------------------------------------- Same comments about the test.

Comment on attachment 737554 [details] [diff] [review] 3 - VM changes for non-writable length, including writes to out-of-bounds indexes, plus test updates Review of attachment 737554 [details] [diff] [review]: ----------------------------------------------------------------- Waldo told me on IRC he only wanted review for the debugger tests. *WHEW* Looks good to me, with the issues in the prior review addressed.

Comment on attachment 737575 [details] [diff] [review] 15 - Fix Array.prototype.splice to respect non-writable lengths Review of attachment 737575 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsarray.cpp @@ +2376,5 @@ > /* > * Optimize only if the array is already dense and we can extend it to > + * its new length. (If the array's length is non-writable, let the > + * generic code handle this case so that error reporting is vaguely > + * more accurate.) It's pretty complicated what's going on here when the array's length is non-writable, might be worth a few more sentences in this comment. On IRC, you mentioned: - growing a fixed-length array, aside from being clearly pointless, would typically violate the capacity <= length invariant on such arrays - the dense case isn't just worse w.r.t. error reporting, it'd actually be wrong: setDenseInitializedLength affects the |in| operator.

Glad to see this fixed! Now that it is, does this in any way change the status of https://bugzilla.mozilla.org/show_bug.cgi?id=591059 or https://bugzilla.mozilla.org/show_bug.cgi?id=598996 ?

Yup, duplicates both, per comment 0. I'm not sure what the exact effect of this was on test262 numbers. I did a run and saved the failures, and when I get updated to a nightly with the fix in place I'll rerun and see just how much/what changed. Should definitely be a pretty large drop, tho.

Fantastic, the test262 score seems to have dropped (from 185?) to 75. Woohoo! \o/

Blog post about this: http://whereswalden.com/2013/08/05/new-in-firefox-23-the-length-property-of-an-array-can-be-made-non-writable-but-you-shouldnt-do-it/

I also updated the new-in document for this: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/23 and the Object.defineProperty docs, to note that this works now, the Firefox versions where it didn't work, and suggested that it not be used: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #73) > Blog post about this: > > http://whereswalden.com/2013/08/05/new-in-firefox-23-the-length-property-of- > an-array-can-be-made-non-writable-but-you-shouldnt-do-it/ As I just commented on your blog post: Programs generally consist of the composition of more loosely coupled components. Between components we try to coordinate about API assumptions. When the two sides make different assumptions, composition errors happen. If an array is passed between components with the assumption that its length won’t change, by making its length non-writable, you get an early signal that you’re mis-coordinating on the nature of the API. The is particularly pressing of course when composing mutually suspicious components. In that case, one component may be purposely trying to violate assumptions of its counterparty, in order to confuse that counterparty into an exploitable state. In ocap JS code, we freeze arrays all the time and recommend that others do so as well. We will continue recommending this.

Since this now depends on a non-closed bug, should this one be re-opened?

No, the code landed here remains in the tree.