Last Comment Bug 858381 - Allow making array lengths non-writable
: Allow making array lengths non-writable
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla23
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
: 591059 598996 696234 (view as bug list)
Depends on: 858677 862343 864913 864964 880591 905947 909602
Blocks: es5 853702
  Show dependency treegraph
 
Reported: 2013-04-04 20:53 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2015-10-08 09:16 PDT (History)
17 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP, some stuff works, other stuff doesn't, no JIT handling of any of the required behavior (23.63 KB, patch)
2013-04-04 20:53 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
WIP, closer to being done (27.52 KB, patch)
2013-04-07 22:40 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Rollup patch of everything for fuzzing (276.27 KB, patch)
2013-04-09 18:05 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
gary: feedback-
choller: feedback+
Details | Diff | Splinter Review
stack (4.35 KB, text/plain)
2013-04-10 01:47 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Second rollup patch -- with various perf optimizations reintroduced, more tests (292.06 KB, patch)
2013-04-12 02:04 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
gary: feedback-
Details | Diff | Splinter Review
stack (4.34 KB, text/plain)
2013-04-12 14:15 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Rollup that never property-caches the array length property (291.86 KB, patch)
2013-04-12 15:09 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Another rollup, with trickeration permitting fewer JIT changes (292.41 KB, patch)
2013-04-13 00:55 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
With one fewer stupid assertion fenceposting error (338.68 KB, patch)
2013-04-13 02:24 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
choller: feedback+
gary: feedback+
Details | Diff | Splinter Review
1 - Consolidate some declarations with first uses in baseops::SetPropertyHelper (1.54 KB, patch)
2013-04-15 09:58 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
2 - Use uint32_t for all element indexes (11.78 KB, patch)
2013-04-15 09:59 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
3 - VM changes for non-writable length, including writes to out-of-bounds indexes, plus test updates (40.73 KB, patch)
2013-04-15 10:04 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
jimb: review+
Details | Diff | Splinter Review
4 - Adjust non-writable arrays' capacity so |capacity <= length| is an invariant for arrays with non-writable length (9.27 KB, patch)
2013-04-15 10:06 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review
5 - Reintroduce optimizations to intelligently update initializedLength/capacity in the face of array truncations (5.64 KB, patch)
2013-04-15 10:08 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review
6 - Fix store-to-element-that-might-be-a-hole Ion and baseline codegen (4.89 KB, patch)
2013-04-15 10:10 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
7 - Fix the methodjit setelem code to work with non-writable array lengths (1.34 KB, patch)
2013-04-15 10:12 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
8 - Fix up the Array.prototype.push code to handle non-writable length in "slow" cases (4.05 KB, patch)
2013-04-15 10:14 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter 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 (9.36 KB, patch)
2013-04-15 10:18 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review
9 - Newborn arrays better have writable length! (658 bytes, patch)
2013-04-15 10:18 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
11 - Stop inlining [].push/pop/shift in the methodjit to not violate non-writable array length guarantees (3.16 KB, patch)
2013-04-15 10:20 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jdemooij: review+
Details | Diff | Splinter Review
12 - Fix [].shift, and the Ion optimized-and-inlined versions, to respect non-writable array length (7.36 KB, patch)
2013-04-15 10:22 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review
13 - Fix [].pop to respect non-writable array lengths (5.76 KB, patch)
2013-04-15 10:25 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
14 - Fix [].unshift to respect non-writable lengths (3.44 KB, patch)
2013-04-15 10:26 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
15 - Fix Array.prototype.splice to respect non-writable lengths (3.15 KB, patch)
2013-04-15 10:28 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
16 - Eliminate JIT changes no longer needed given the |capacity <= length| invariant for arrays with non-writable length (7.98 KB, patch)
2013-04-15 10:29 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review
Latest rollup against latest m-i (113.56 KB, patch)
2013-04-15 16:17 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Potential m-c-applicable rollup (290.97 KB, patch)
2013-04-15 16:22 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
choller: feedback+
Details | Diff | Splinter Review
Add a parallel test that pushing a dense element never does so on an array with non-writable length, and an assertion of this. (As long as Object.defineProperty isn't parallel-safe this is all belt-and-suspenders, but best to have a test for it.) NOT RE (1.93 KB, patch)
2013-04-16 13:22 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Better parallel test (2.04 KB, patch)
2013-04-22 16:07 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
nmatsakis: review+
Details | Diff | Splinter Review
New rollup patch, against m-i 75648b269697, for retesting of bug 862343 (126.10 KB, patch)
2013-04-23 15:42 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
13 - [].pop, updated (10.13 KB, patch)
2013-04-23 19:16 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
Test for no-op non-writable array length redefinition (3.67 KB, patch)
2013-04-24 12:43 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-04 20:53:11 PDT
Created attachment 733719 [details] [diff] [review]
WIP, some stuff works, other stuff doesn't, no JIT handling of any of the required behavior

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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-07 22:40:26 PDT
Created attachment 734477 [details] [diff] [review]
WIP, closer to being done

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.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-09 18:05:19 PDT
Created attachment 735518 [details] [diff] [review]
Rollup patch of everything for fuzzing

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 3 Gary Kwong [:gkw] [:nth10sd] 2013-04-09 18:17:43 PDT
Comment on attachment 735518 [details] [diff] [review]
Rollup patch of everything for fuzzing

Inbound rev is currently cc4abdf71cb8, will test this now.
Comment 4 Gary Kwong [:gkw] [:nth10sd] 2013-04-10 01:47:49 PDT
Created attachment 735639 [details]
stack

(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
Comment 5 Christian Holler (:decoder) 2013-04-10 15:25:00 PDT
Testing this now, expect results tomorrow or Friday (I'm on a conference, so it could take a bit).
Comment 6 Christian Holler (:decoder) 2013-04-11 09:27:36 PDT
Comment on attachment 735518 [details] [diff] [review]
Rollup patch of everything for fuzzing

I haven't found any more failures during fuzzing :)
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-11 22:55:11 PDT
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.
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-11 23:43:13 PDT
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.
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-12 02:04:53 PDT
Created attachment 736719 [details] [diff] [review]
Second rollup patch -- with various perf optimizations reintroduced, more tests

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.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-12 02:06:06 PDT
Bug 858677 is needed before this lands to implement trailing-element deletion semantics correctly.
Comment 11 Gary Kwong [:gkw] [:nth10sd] 2013-04-12 14:15:42 PDT
Created attachment 736979 [details]
stack

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.
Comment 12 Gary Kwong [:gkw] [:nth10sd] 2013-04-12 14:27:49 PDT
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.
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-12 15:09:48 PDT
Created attachment 737000 [details] [diff] [review]
Rollup that never property-caches the array length property

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 14 Gary Kwong [:gkw] [:nth10sd] 2013-04-12 15:12:12 PDT
Comment on attachment 736979 [details]
stack

Obsoleted with the 3rd rollup patch fix.
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-12 15:16:28 PDT
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.
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-12 17:36:23 PDT
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.  :-)
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-13 00:55:25 PDT
Created attachment 737096 [details] [diff] [review]
Another rollup, with trickeration permitting fewer JIT changes

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.
Comment 18 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-13 01:15:55 PDT
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.
Comment 19 Brian Hackett (:bhackett) 2013-04-13 01:37:16 PDT
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.
Comment 20 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-13 02:24:04 PDT
Created attachment 737113 [details] [diff] [review]
With one fewer stupid assertion fenceposting error

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 21 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-13 02:26:35 PDT
https://tbpl.mozilla.org/?tree=Try&rev=b9147df4a5dc
Comment 22 Gary Kwong [:gkw] [:nth10sd] 2013-04-13 02:54:48 PDT
Comment on attachment 737000 [details] [diff] [review]
Rollup that never property-caches the array length property

Guessing that this is now obsolete..
Comment 23 Gary Kwong [:gkw] [:nth10sd] 2013-04-15 09:35:35 PDT
Comment on attachment 737113 [details] [diff] [review]
With one fewer stupid assertion fenceposting error

Nothing found over the weekend -> feedback+.
Comment 24 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 09:58:31 PDT
Created attachment 737550 [details] [diff] [review]
1 - Consolidate some declarations with first uses in baseops::SetPropertyHelper

Just a little moving to first use, to help readability and hacking in of new code later.
Comment 25 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 09:59:26 PDT
Created attachment 737551 [details] [diff] [review]
2 - Use uint32_t for all element indexes
Comment 26 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:04:36 PDT
Created attachment 737554 [details] [diff] [review]
3 - VM changes for non-writable length, including writes to out-of-bounds indexes, plus test updates

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.
Comment 27 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:06:02 PDT
Created attachment 737556 [details] [diff] [review]
4 - Adjust non-writable arrays' capacity so |capacity <= length| is an invariant for arrays with non-writable length
Comment 28 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:08:10 PDT
Created attachment 737557 [details] [diff] [review]
5 - Reintroduce optimizations to intelligently update initializedLength/capacity in the face of array truncations

For simplicity I implemented everything as stupidly as possible, at first.  This reintroduces some of the optimizations removed in the course of patching.
Comment 29 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:10:57 PDT
Created attachment 737558 [details] [diff] [review]
6 - Fix store-to-element-that-might-be-a-hole Ion and baseline codegen

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.
Comment 30 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:12:23 PDT
Created attachment 737559 [details] [diff] [review]
7 - Fix the methodjit setelem code to work with non-writable array lengths

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.
Comment 31 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:14:55 PDT
Created attachment 737561 [details] [diff] [review]
8 - Fix up the Array.prototype.push code to handle non-writable length in "slow" cases

InitArrayElements sweeps up a bunch of different methods, as I recall, but the slower push was possibly the most visible one.
Comment 32 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:18:14 PDT
Created 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
Comment 33 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:18:50 PDT
Created attachment 737564 [details] [diff] [review]
9 - Newborn arrays better have writable length!
Comment 34 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:20:37 PDT
Created attachment 737565 [details] [diff] [review]
11 - Stop inlining [].push/pop/shift in the methodjit to not violate non-writable array length guarantees

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.  :-\
Comment 35 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:22:50 PDT
Created attachment 737569 [details] [diff] [review]
12 - Fix [].shift, and the Ion optimized-and-inlined versions, to respect non-writable array length

The Ion change is obviously a temporary state.  :-)  Pretty sure the next patch makes that code look more correct.
Comment 36 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:25:58 PDT
Created attachment 737571 [details] [diff] [review]
13 - Fix [].pop to respect non-writable array lengths

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.
Comment 37 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:26:51 PDT
Created attachment 737573 [details] [diff] [review]
14 - Fix [].unshift to respect non-writable lengths
Comment 38 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:28:19 PDT
Created attachment 737575 [details] [diff] [review]
15 - Fix Array.prototype.splice to respect non-writable lengths
Comment 39 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:29:53 PDT
Created attachment 737576 [details] [diff] [review]
16 - Eliminate JIT changes no longer needed given the |capacity <= length| invariant for arrays with non-writable length

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 40 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 10:37:31 PDT
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 41 Jan de Mooij [:jandem] 2013-04-15 10:55:29 PDT
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?
Comment 42 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 11:20:42 PDT
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.
Comment 43 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 16:17:31 PDT
Created attachment 737737 [details] [diff] [review]
Latest rollup against latest m-i
Comment 44 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-15 16:22:05 PDT
Created attachment 737741 [details] [diff] [review]
Potential m-c-applicable rollup

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.
Comment 45 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-16 13:22:44 PDT
Created attachment 738163 [details] [diff] [review]
Add a parallel test that pushing a dense element never does so on an array with non-writable length, and an assertion of this.  (As long as Object.defineProperty isn't parallel-safe this is all belt-and-suspenders, but best to have a test for it.)  NOT RE

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 46 Christian Holler (:decoder) 2013-04-18 04:13:55 PDT
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 47 Brian Hackett (:bhackett) 2013-04-18 06:11:51 PDT
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 48 Brian Hackett (:bhackett) 2013-04-18 06:39:11 PDT
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 49 Brian Hackett (:bhackett) 2013-04-18 06:49:47 PDT
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 50 Brian Hackett (:bhackett) 2013-04-18 06:50:31 PDT
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 51 Brian Hackett (:bhackett) 2013-04-18 06:59:21 PDT
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?
Comment 52 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-18 16:26:14 PDT
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.
Comment 53 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-22 16:07:11 PDT
Created attachment 740540 [details] [diff] [review]
Better parallel test

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 54 Niko Matsakis [:nmatsakis] 2013-04-23 15:24:00 PDT
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 55 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-23 15:42:17 PDT
Created attachment 741067 [details] [diff] [review]
New rollup patch, against m-i 75648b269697, for retesting of bug 862343
Comment 56 Jason Orendorff [:jorendorff] 2013-04-23 16:53:57 PDT
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 57 Jason Orendorff [:jorendorff] 2013-04-23 17:33:22 PDT
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.
Comment 58 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-23 19:16:28 PDT
Created attachment 741125 [details] [diff] [review]
13 - [].pop, updated

(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.
Comment 59 Jason Orendorff [:jorendorff] 2013-04-24 10:39:50 PDT
(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?
Comment 60 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-24 12:43:38 PDT
Created attachment 741474 [details] [diff] [review]
Test for no-op non-writable array length redefinition

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 61 Jason Orendorff [:jorendorff] 2013-04-24 14:11:57 PDT
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 62 Jim Blandy :jimb 2013-04-24 14:45:30 PDT
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 63 Jason Orendorff [:jorendorff] 2013-04-24 14:52:03 PDT
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 64 Jim Blandy :jimb 2013-04-24 14:52:48 PDT
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 65 Jason Orendorff [:jorendorff] 2013-04-24 15:34:59 PDT
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.
Comment 66 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-24 22:55:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eac2a78a791
Comment 67 Ryan VanderMeulen [:RyanVM] 2013-04-25 19:01:15 PDT
https://hg.mozilla.org/mozilla-central/rev/8eac2a78a791
Comment 68 Mark S. Miller 2013-04-25 19:28:35 PDT
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
?
Comment 69 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-26 01:30:22 PDT
*** Bug 591059 has been marked as a duplicate of this bug. ***
Comment 70 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-26 01:31:02 PDT
*** Bug 598996 has been marked as a duplicate of this bug. ***
Comment 71 Jeff Walden [:Waldo] (remove +bmo to email) 2013-04-26 01:36:21 PDT
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.
Comment 72 Marco Perez 2013-04-26 14:51:48 PDT
Fantastic, the test262 score seems to have dropped (from 185?) to 75. Woohoo! \o/
Comment 74 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-05 16:28:55 PDT
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
Comment 75 Mark S. Miller 2013-08-05 16:39:58 PDT
(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.
Comment 76 Mark S. Miller 2013-08-19 14:03:33 PDT
Since this now depends on a non-closed bug, should this one be re-opened?
Comment 77 Andrew McCreight [:mccr8] 2013-08-19 14:05:22 PDT
No, the code landed here remains in the tree.
Comment 78 André Bargull 2015-10-08 09:16:14 PDT
*** Bug 696234 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.