537873 - Assignment to readonly properties should throw a TypeError in strict mode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Comment 1

[Jeff Walden [:Waldo]]

Fix will be in two places: first, add the strict-mode flag to the place inside js_SetPropertyHelper, and second, make the JS_HAS_STRICT_OPTION(cx) check above it instead use the logic in jscntxt.cpp!checkReportFlags.  Late now, will look at tomorrow...

Comment 2

[Jim Blandy :jimb]

Stolen with permission. (Would prefer stollen with butter, but, whatever.)

Comment 3

[Jeff Walden [:Waldo]]

Don't noodle away on the problem too long, we have deadlines to make.

Comment 4

[Robert Sayre]

Can we get an update here?

Comment 5

[Jim Blandy :jimb]

I'm actively working on this, together with bug 514574, which needs most of the
same stuff. I should have a patch today or tomorrow.

Comment 6

[Jim Blandy :jimb]

Attachment: Bug 537873: Tests for strict mode assignments to read-only properties, deletions of non-configurable properties.

These tests are basically complete, so I'm posting them if anyone's interested. Will r? along with the code changes.

Comment 7

[Jim Blandy :jimb]

Yay:

js> o.x=1
1
js> "use strict"; o.x=1
typein:6: TypeError: o.x is read-only
js> a[0]=2
2
js> "use strict"; a[0]=2
typein:8: TypeError: a[0] is read-only

Comment 8

[Jim Blandy :jimb]

Attachment: Work in progress: have strict mode code report TypeErrors for assignments, deletions.

This is the patch as it stands. The tests like it overall; I'm looking into some failures.

Comment 9

[Jim Blandy :jimb]

All tests pass, except for assignments to functions' 'name' and 'arity' properties. Should be quick.

Comment 10

[Jim Blandy :jimb]

Attachment: Use ObjectOps::setProperty for both fast and slow arrays.

Comment 11

[Jim Blandy :jimb]

Attachment: Tests for strict mode assignments to read-only properties, deletions of non-configurable properties. [committed]

Comment 12

[Jim Blandy :jimb]

Attachment: Delete unused property operation typedefs: JSDefinePropOp, JSPropertyIdOp. [committed]

Comment 13

[Jim Blandy :jimb]

Attachment: Have strict mode code report TypeErrors for assignments, deletions. [committed]

Comment 14

[Jim Blandy :jimb]

Apparently SunSpider-neutral.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 473253 [details] [diff] [review]
Have strict mode code report TypeErrors for assignments, deletions. [committed]

There's an else after return on jsobj.cpp:5010.

s/JS_TRUE/true/g
s/JS_FALSE/false/g

even where passed for a JSBool formal. We should not use JS_TRUE and JS_FALSE unless spot-fixing in an existing function. In this light, you could convert some functions where you touch enough lines to use true and false instead.

The extra trailing JSBool strict argument cost may not show up in SS but it has to count in instructions and other costs. But I don't see an alternative.

Is there a dominant case that motivates a default parameter value? For auditing during "bring-up" you'd want to require explicit strict actual arg passing, but I wonder if there's a dominant case that should be the default value. It looks like there are 18 false/JS_FALSE trailinga actuals on + lines in the patch, vs. 11 JS_TRUE (no true).

> TODO: Fix error messages.

What's needed here?

> TODO: Is there any way to actually produce JSOP_SETCONST? const {x} = 0
> And thus JSOP_ENUMCONSTELEM?

These arise in global and eval code (not in function code).

Followup bugs for the other TODO items?

/be

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 473253 [details] [diff] [review]
Have strict mode code report TypeErrors for assignments, deletions. [committed]

>+            if (!JS_CHECK_OPERATION_LIMIT(cx) || !DeleteArrayElement(cx, obj, oldlen, JS_TRUE)) {
>+                obj->setArrayLength(oldlen+1);
>+                if (strict)
>+                    return false;
>+                JS_ClearPendingException(cx);
>+                return true;
>+            }

Forgot to note this one and the code like it in js::array_sort: you want to separate the operation limit check and false return:

>+            if (!JS_CHECK_OPERATION_LIMIT(cx)) {
>+                return false;

from the maybe-fallible-due-to-strict logic:

>+            if (!DeleteArrayElement(cx, obj, oldlen, true)) {
>+                obj->setArrayLength(oldlen+1);
>+                if (strict)
>+                    return false;
>+                JS_ClearPendingException(cx);
>+                return true;
>+            }

Nit: space on both sides of + in oldlen+1.

/be

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 473251 [details] [diff] [review]
Tests for strict mode assignments to read-only properties, deletions of non-configurable properties. [committed]

>diff --git a/js/src/tests/ecma_5/strict/10.6.js b/js/src/tests/ecma_5/strict/10.6.js

In my misbegotten youth I adhered to the name-by-spec-section convention, but these days I prefer naming the test files after what they're testing, because the former results in inscrutability when you're staring.


>diff --git a/js/src/tests/ecma_5/strict/15.4.4.11.js b/js/src/tests/ecma_5/strict/15.4.4.11.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/strict/15.4.4.11.js
>@@ -0,0 +1,34 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+
>+/*
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/licenses/publicdomain/
>+ */
>+
>+function arr() {
>+  var a=[20,10,30];
>+  Object.defineProperty(a, 0, {writable:false});
>+  return a;
>+}

You could combine into a one-liner (for the win?):

  return Object.defineProperty([20, 10, 30], 0, { writable: false });

Please do add spaces in the array literal (and around "=") in any case.

This test should be in an extensions directory, because sort's behavior on an array with non-writable properties is implementation-defined (I think you missed that paragraph in ES5 -- before "Otherwise, the following steps are taken").

You could do a non-configurable test too, if you wanted, again so long as it moved to an extensions directory.


>diff --git a/js/src/tests/ecma_5/strict/15.4.4.12.js b/js/src/tests/ecma_5/strict/15.4.4.12.js

...hmm, this is starting to get into more per-Array-method muck than I think I have time to handle (at least if I don't want to make a semi-blind assumption that all the array methods use the exact same manner to set and remove elements, always with strict = true, and such).  What I've read up to the start of this file looks okay, but I don't think I have time to do the rest justice.  :-\  Hopefully it won't be too hard to find someone else who can tackle the remainder...

Comment 18

[Jim Blandy :jimb]

(In reply to comment #17)
/me sees timestamp on comment
Thanks very much for taking the time to review this, Jeff!  Hope your trip was good.

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #16)
> Comment on attachment 473253 [details] [diff] [review]
> Have strict mode code report TypeErrors for assignments, deletions.
> 
> >+            if (!JS_CHECK_OPERATION_LIMIT(cx) || !DeleteArrayElement(cx, obj, oldlen, JS_TRUE)) {
> >+                obj->setArrayLength(oldlen+1);
> >+                if (strict)
> >+                    return false;
> >+                JS_ClearPendingException(cx);
> >+                return true;
> >+            }
> 
> Forgot to note this one and the code like it in js::array_sort: you want to
> separate the operation limit check and false return:
> 
> >+            if (!JS_CHECK_OPERATION_LIMIT(cx)
> >+                return false;
> 
> from the maybe-fallible-due-to-strict logic: ...

This is because operation callback canceling a running script via false return does not imply an error-as-exception thrown, or even an error report (the slow script dialog counts as that, but embeddings can automate, e.g., remember the last dialog response and use it again).

In any event, an operation callback canceling a slow script is not a strict mode error ;-).

/be

Comment 20

[Jim Blandy :jimb]

Comment on attachment 473250 [details] [diff] [review]
Use ObjectOps::setProperty for both fast and slow arrays.

The comment in the patch explains the rationale.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 473250 [details] [diff] [review]
Use ObjectOps::setProperty for both fast and slow arrays.

>@@ -609,6 +609,12 @@ array_length_setter(JSContext *cx, JSObj
>     jsuint newlen, oldlen, gap, index;
>     Value junk;
> 
>+    /* Check for a sealed object first. */
>+    if (obj->sealed())
>+        return js_ReportValueErrorFlags(cx, JSREPORT_ERROR, JSMSG_READ_ONLY,
>+                                        JSDVG_IGNORE_STACK, IdToValue(id), NULL,
>+                                        NULL, NULL);

Nit: always brace multi-line consequents.

>@@ -1051,7 +1057,34 @@ Class js_SlowArrayClass = {
>     PropertyStub,   /* setProperty */
>     EnumerateStub,
>     ResolveStub,
>-    js_TryValueOf
>+    js_TryValueOf,
>+    NULL,           /* finalize    */
>+    NULL,           /* reserved0   */
>+    NULL,           /* checkAccess */
>+    NULL,           /* call        */
>+    NULL,           /* construct   */
>+    NULL,           /* xdrObject   */
>+    NULL,           /* hasInstance */
>+    NULL,           /* mark        */
>+    JS_NULL_CLASS_EXT,
>+    {
>+        NULL,       /* lookupProperty   */
>+        NULL,       /* defineProperty   */
>+        NULL,       /* getProperty      */
>+        /*
>+         * For assignments to 'length', we need to know the setter's strictness. A property's
>+         * setter isn't passed that, but the ObjectOps member is, so use that.
>+         */
>+        array_setProperty,

A slowarray_setProperty would be pickier in bug-finding ways:

static JSBool
slowarray_setProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp)
{
    JS_ASSERT(obj->isSlowArray());

    if (JSID_IS_ATOM(id, cx->runtime->atomState.lengthAtom))
        return array_length_setter(cx, obj, id, vp);

    return js_SetProperty(cx, obj, id, vp);
}

r=me with these addressed.

/be

Comment 22

[Jim Blandy :jimb]

> There's an else after return on jsobj.cpp:5010.

Fixed.

> s/JS_TRUE/true/g
> s/JS_FALSE/false/g
> 
> even where passed for a JSBool formal. We should not use JS_TRUE and JS_FALSE
> unless spot-fixing in an existing function. In this light, you could convert
> some functions where you touch enough lines to use true and false instead.

Done.

> The extra trailing JSBool strict argument cost may not show up in SS but it has
> to count in instructions and other costs. But I don't see an alternative.
> 
> Is there a dominant case that motivates a default parameter value? For auditing
> during "bring-up" you'd want to require explicit strict actual arg passing, but
> I wonder if there's a dominant case that should be the default value. It looks
> like there are 18 false/JS_FALSE trailinga actuals on + lines in the patch, vs.
> 11 JS_TRUE (no true).

Given the way ES5 has made all the foo.prototype functions pass 'true' for the 'Throw' argument of [[Put]] and [[Delete]] --- in other words, electing to behave as if they were written as strict mode code --- perhaps we should prefer 'true' in new code. A default of 'true' would encourage that.

> > TODO: Fix error messages.
> 
> What's needed here?

This seemed unhelpful to me:

js> a=Object.defineProperty([1,2,3],0,{writable:false})
[1, 2, 3]
js> a.reverse()
typein:2: TypeError: a.reverse() is read-only


> Followup bugs for the other TODO items?

Filed bug 596351 for proxies.  Typed arrays reject all attempts to configure elements anyway. E4X objects' properties are not configurable either.

Comment 23

[Jim Blandy :jimb]

(In reply to comment #16)
> Forgot to note this one and the code like it in js::array_sort: you want to
> separate the operation limit check and false return:

Oh, good catch.  I've separated the two.  However, in js::array_sort, I think the code is correct: ES5 says that Array.prototype.sort passes true as [[Delete]]'s Throw argument, and indeed, the code will throw.

Comment 24

[Jim Blandy :jimb]

(In reply to comment #17)
> In my misbegotten youth I adhered to the name-by-spec-section convention, but
> these days I prefer naming the test files after what they're testing, because
> the former results in inscrutability when you're staring.

It's true that numbering is lousy for filename completion. However, names would need to be chosen according to some kind of conventions to be valuable, and as different developers work on the directory, the consistency is going to erode. One thing I do like about the numbers is that it's easy to find existing, related tests when adding new tests.

I don't want to rename them, but I don't mind if you do.

> You could combine into a one-liner (for the win?):
> 
>   return Object.defineProperty([20, 10, 30], 0, { writable: false });

Done.

> Please do add spaces in the array literal (and around "=") in any case.

Done.

> This test should be in an extensions directory, because sort's behavior on an
> array with non-writable properties is implementation-defined (I think you
> missed that paragraph in ES5 -- before "Otherwise, the following steps are
> taken").

Done.

> You could do a non-configurable test too, if you wanted, again so long as it
> moved to an extensions directory.

It actually wasn't my intention to write any tests for non-ES5-prescribed behavior at all.

> >diff --git a/js/src/tests/ecma_5/strict/15.4.4.12.js b/js/src/tests/ecma_5/strict/15.4.4.12.js
> 
> ...hmm, this is starting to get into more per-Array-method muck than I think I
> have time to handle (at least if I don't want to make a semi-blind assumption
> that all the array methods use the exact same manner to set and remove
> elements, always with strict = true, and such).  What I've read up to the start
> of this file looks okay, but I don't think I have time to do the rest justice. 

Basically, all the uses of [[Put]] and [[Delete]] in Mumble.prototype functions pass 'true' for Throw. They've all been declared strict mode code. :)

Comment 25

[Jim Blandy :jimb]

(In reply to comment #21)
> Nit: always brace multi-line consequents.

Done.

> A slowarray_setProperty would be pickier in bug-finding ways:

Done.

Comment 26

[Jim Blandy :jimb]

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

Comment 27

[Jim Blandy :jimb]

bug 596728 filed for the error messages.

Comment 28

[Robert Sayre]

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

Comment 29

[Brendan Eich [:brendan]]

Apologies for missing this utterly in reviewing attachment 473250 [details] [diff] [review] -- it is what caused bug 599159. We need array_length_setter's code that handles non-Array obj delegating to an Array:

    if (!obj->isArray()) {
        jsid lengthId = ATOM_TO_JSID(cx->runtime->atomState.lengthAtom);

        return obj->defineProperty(cx, lengthId, *vp, NULL, NULL, JSPROP_ENUMERATE);
    }

This can go in the object-op, but again favors splitting setProperty ops for dense vs. slow. Waldo, any thoughts?

/be

Comment 30

[Jim Blandy :jimb]

Per discussion on IRC, I'm preparing a patch to revert the portion of this patch that allows assignments to array length properties to be strict-aware, as those introduced more problems than they solved (bug 599159). If that patch lands, I'll re-open this bug.

Comment 31

[Jeff Walden [:Waldo]]

Don't reopen, file a new bug please -- this one's pretty crowded already.

Comment 32

[Brendan Eich [:brendan]]

We reopen on back-out. This wasn't a complete back-out, but close enough. The info here is on target, esp. jorendorff's tests.

/be

Comment 33

[Jeff Walden [:Waldo]]

I must be missing something, doesn't the backout only affect [].length and no other property?  That seemed far from complete to me, but if it was markedly more than that, of course reopen.

Comment 34

[Eric Shepherd [:sheppy]]

What is the net effect of this bug in terms of documentation impact? There's a lot of stuff here, and stuff was backed out, so I want to be clear on what needs to be written before I start. Or, ideally, someone else (waldo) would write it. :)

Comment 35

[Jim Blandy :jimb]

Reopened, as back-out bug 599159 has landed.

Comment 36

[Damon Sicore (:damons)]

Since this was blocking beta 7, should it now block 8?

Comment 37

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 38

[LegNeato]

Fixing fields my automated script accidentally blanked. Apologies for the bugspam

Comment 39

[Jim Blandy :jimb]

I've begun working on this.

Comment 40

[Jim Blandy :jimb]

As far as I can see, the best way forward is for me to resurrect my patch to give setter functions a different type --- a function that accepts a 'strict' flag, indicating whether the store should be handled as a strict mode store. It's massive, but doesn't take much brain: once it compiles, you're done. I'll check the performance impact of the extra parameter passing before proceeding.

Comment 41

[Jim Blandy :jimb]

Impact on instruction counts of adding the new setter parameter, via njn's cachegrind-on-SunSpider comparison scripts.

------------------------------------------------------------------
instructions executed (nb: "on-trace" may overestimate slightly)
------------------------------------------------------------------
3d-cube:
  total          83,425,922      83,427,141     -- 
  on-trace       60,469,152      60,469,152     -- 
3d-morph:
  total          36,929,643      36,931,428     -- 
  on-trace       25,259,862      25,259,862     -- 
3d-raytrace:
  total          76,360,853      76,366,873     -- 
  on-trace       49,059,152      49,059,152     -- 
access-binary-trees:
  total          26,505,973      26,508,011     -- 
  on-trace       14,174,288      14,174,288     -- 
access-fannkuch:
  total         136,284,018     136,285,343     -- 
  on-trace      130,835,399     130,835,399     -- 
access-nbody:
  total          29,310,645      29,310,613     -- 
  on-trace       17,620,955      17,620,955     -- 
access-nsieve:
  total          52,368,698      52,369,831     -- 
  on-trace       47,032,785      47,032,785     -- 
bitops-3bit-bits-in-byte:
  total           7,045,790       7,046,968     -- 
  on-trace        3,497,629       3,497,629     -- 
bitops-bits-in-byte:
  total          52,621,157      52,622,351     -- 
  on-trace       48,474,438      48,474,438     -- 
bitops-bitwise-and:
  total          16,433,529      16,434,913     -- 
  on-trace       13,207,802      13,207,802     -- 
bitops-nsieve-bits:
  total          38,861,408      38,862,627     -- 
  on-trace       34,501,389      34,501,389     -- 
controlflow-recursive:
  total          22,092,187      22,093,337     -- 
  on-trace       18,533,959      18,533,959     -- 
crypto-aes:
  total          65,840,283      65,842,043     -- 
  on-trace       44,086,150      44,086,150     -- 
crypto-md5:
  total          26,153,905      26,155,413     -- 
  on-trace       15,149,031      15,149,031     -- 
crypto-sha1:
  total          20,519,344      20,520,656     -- 
  on-trace       10,443,566      10,443,566     -- 
date-format-tofte:
  total          72,080,152      72,083,507     -- 
  on-trace       29,529,054      29,529,054     -- 
date-format-xparb:
  total          53,612,632      53,617,345     -- 
  on-trace       11,036,934      11,036,934     -- 
math-cordic:
  total          38,138,930      38,139,802     -- 
  on-trace       28,960,518      28,960,518     -- 
math-partial-sums:
  total          25,459,295      25,463,172     -- 
  on-trace        5,586,517       5,586,517     -- 
math-spectral-norm:
  total          40,886,430      40,887,800     -- 
  on-trace       36,503,092      36,503,092     -- 
regexp-dna:
  total          46,339,084      46,336,222     -- 
  on-trace       34,494,293      34,494,293     -- 
string-base64:
  total          24,745,961      24,748,593     -- 
  on-trace        9,516,760       9,516,760     -- 
string-fasta:
  total          72,187,118      72,186,903     -- 
  on-trace       43,681,531      43,681,531     -- 
string-tagcloud:
  total          88,287,042      88,290,874     -- 
  on-trace       17,993,073      17,993,073     -- 
string-unpack-code:
  total          96,298,470      96,209,363     1.001x better
  on-trace       13,240,663      13,240,663     -- 
string-validate-input:
  total          33,185,846      33,186,200     -- 
  on-trace        8,732,714       8,732,714     -- 
-------
all:
  total       1,281,974,315   1,281,927,329     -- 
  on-trace      771,620,706     771,620,706     --

Comment 42

[Jim Blandy :jimb]

Looks to me like no significant impact. Largest instruction count increase is 0.017%, in bitops-3bit-bits-in-byte; median is 0.004%.

That string-unpack-code change is suspicious; I can't imagine why we would be executing *fewer* instructions. Looking into it.

Comment 43

[Jim Blandy :jimb]

Attachment: Add a 'strict' argument to C++ property setter functions.

Just attaching this for posterity; I'd hate to lose it if my computer died.

Comment 44

[Brendan Eich [:brendan]]

Comment on attachment 507293 [details] [diff] [review]
Add a 'strict' argument to C++ property setter functions.

"When in doubt, use brute force." - Ken Thompson

One thought: we put out params last, but strict is after vp. Is there a strict in param is last-ier than out params convention?

There must be some DOM or XBL changes too, or I'm forgetting changes that did away with setters over in those subtrees.

/be

Comment 45

[Brendan Eich [:brendan]]

Cc'ing Nick on comment 42 and prior.

/be

Comment 46

[Nicholas Nethercote [inactive]]

(In reply to comment #42)
> 
> That string-unpack-code change is suspicious; I can't imagine why we would be
> executing *fewer* instructions. Looking into it.

It's probably due to slightly fewer misses in the property cache, that sometimes causes mild non-determinism.  cg_diff is the magic sauce for working such things out;  I have non-portable scripts for running it automatically, not that that helps you :/

Anyway, comment 41 says "no perf impact".

Comment 47

[Jim Blandy :jimb]

(In reply to comment #46)
> It's probably due to slightly fewer misses in the property cache, that
> sometimes causes mild non-determinism.  cg_diff is the magic sauce for working
> such things out;  I have non-portable scripts for running it automatically, not
> that that helps you :/

Yes, I'm fussing with cg_diff right now. (I even have some patches for you!)

Can the property cache cause non-determinism even when I've disabled address space randomization?

Comment 48

[Jim Blandy :jimb]

(In reply to comment #46)
> Anyway, comment 41 says "no perf impact".

Right; I'm just worried that the speedup says "adverse correctness impact." :)

Comment 49

[Jim Blandy :jimb]

(In reply to comment #44)
> One thought: we put out params last, but strict is after vp. Is there a strict
> in param is last-ier than out params convention?

Thanks for looking it over. I'll move that param.

> There must be some DOM or XBL changes too, or I'm forgetting changes that did
> away with setters over in those subtrees.

Doubtless-  I'm going to do SM first, then tend to the browser.

Comment 50

[Nicholas Nethercote [inactive]]

(In reply to comment #47)
> 
> Can the property cache cause non-determinism even when I've disabled address
> space randomization?

I believe so.  I can't remember why, or even if I figured out why.

Comment 51

[Jim Blandy :jimb]

Resurrected an old patch that provides not-horribly-expensive logging of opcodes and stack contents. The execution of the string-unpack-code is unaffected by the patch. The difference in cycle counts is due to fewer calls to js::PropertyTable::search --- probably because the property cache is indexed by bytecode addresses, which the patch does affect.

Comment 52

[Nicholas Nethercote [inactive]]

Told ya! :)  Where are the cg_diff patches?

Comment 53

[Jim Blandy :jimb]

(In reply to comment #52)
> Told ya! :)  Where are the cg_diff patches?

I filed a valgrind bug:
https://bugs.kde.org/show_bug.cgi?id=264689

Comment 54

[Jeff Walden [:Waldo]]

I think the strict mode docs took care of documenting this:

https://developer.mozilla.org/en/JavaScript/Strict_mode

(What a coincidence, I did end up writing it!  ;-) )

Comment 55

[Jim Blandy :jimb]

Attachment: Make --enable-methodjit-spew work in non-DEBUG code.

This isn't strictly necessary for the bug, but it was handy in making sure
the patch hadn't affected SM's behavior.

Comment 56

[Jim Blandy :jimb]

Attachment: Avoid writing past the end of ScriptObjectFixture::uc_code.

This silences a warning when building testScriptObject.o under GCC -O3.

Comment 57

[Jim Blandy :jimb]

Attachment: Re-enable tests for assignments to array lengths in strict mode; add new regression tests.

TODO: test that the array length setter receives the strict flag appropriately even when there's a watchpoint set on it

Comment 58

[Jim Blandy :jimb]

Attachment: Use the standard JS_PropertyStub for setters of typed arrays' read-only properties, not a Jsvalification of the getters.

This makes the patch to give getters and setters distinct types a little easier to read.

Comment 59

[Jim Blandy :jimb]

Attachment: Add a 'strict' argument to C++ property setter functions.

This changes the type of setters to JSStrictPropertyOp, which is just like
JSPropertyOp except that it takes a 'JSBool strict' argument. Most of the
patch is introducing distinct types and using the appropriate stubs.

The following are left for subsequent patches:

x Similar fixes to the browser outside SpiderMonkey.

x Actually *using* the newly available strictness information. This patch
  should have no user-visible effect. I didn't want the interesting stuff
  to get lost in this noise.

Comment 60

[Jim Blandy :jimb]

Attachment: Throw errors when strict mode code assigns to an array's length and the truncation would delete non-configurable elements.

This is the patch that actually fixes the bug.

Comment 61

[Jim Blandy :jimb]

I've got about 50kB of browser-side changes, but they're not done yet.

I may need help getting Windows-only code changed.

We should let the other products know about this change, because if they've got any custom setters, they'll need to change them appropriately.

Comment 62

[Brendan Eich [:brendan]]

Comment on attachment 508270 [details] [diff] [review]
Make --enable-methodjit-spew work in non-DEBUG code.

Sure, whatever it takes so long as the #if terms are minimized. r=me.

I'm an ancient (John Reiser CPP -- ugliest C code ever) pre-processor fan so you'll see #if defined X || ... (no parens around X) in SpiderMonkey. Either way is ok and parens were already used here. I'm easy (sizeof is a different case).

/be

Comment 63

[Brendan Eich [:brendan]]

Comment on attachment 508274 [details] [diff] [review]
Add a 'strict' argument to C++ property setter functions.

>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
. . .
> extern JS_PUBLIC_API(JSBool)
>+JS_StrictPropertyStub(JSContext *cx, JSObject *obj, jsid id, JSBool, jsval *vp);

Any reason not to give the formal its name here?

r=me with this. Thanks,

/be

Comment 64

[Jim Blandy :jimb]

(In reply to comment #63)
> >+JS_StrictPropertyStub(JSContext *cx, JSObject *obj, jsid id, JSBool, jsval *vp);
> 
> Any reason not to give the formal its name here?

Fixed --- thanks.

Comment 65

[Brendan Eich [:brendan]]

Comment on attachment 508275 [details] [diff] [review]
Throw errors when strict mode code assigns to an array's length and the truncation would delete non-configurable elements.

The tests in attachment 508272 [details] [diff] [review] don't cover this patch, so I'm assuming tests of the kind Waldo should review are forthcoming ;-).

/be

Comment 66

[Jim Blandy :jimb]

(In reply to comment #65)
> The tests in attachment 508272 [details] [diff] [review] don't cover this patch, so I'm assuming tests of
> the kind Waldo should review are forthcoming ;-).

Attachment 508272 [details] [diff] re-enables ecma_5/strict/jstests.list, which does check that an error gets thrown. Am I missing something?

Comment 67

[Jim Blandy :jimb]

Attachment: Add 'strict' argument to setters defined throughout Firefox.

This should take care of all setters defined outside SpiderMonkey proper. Very mechanical.

Comment 68

[Brendan Eich [:brendan]]

Comment on attachment 509033 [details] [diff] [review]
Add 'strict' argument to setters defined throughout Firefox.

>+// Apply |op| to |obj|, |id|, and |vp|. If |op| is a setter, treat the assignment as lenient.
>+template<typename Op>
>+static inline JSBool ApplyPropertyOp(JSContext *cx, Op op, JSObject *obj, jsid id, jsval *vp);
>+
>+template<>
>+inline JSBool
>+ApplyPropertyOp<JSPropertyOp>(JSContext *cx, JSPropertyOp op, JSObject *obj, jsid id, jsval *vp)
>+{
>+    return op(cx, obj, id, vp);
>+}
>+
>+template<>
>+inline JSBool
>+ApplyPropertyOp<JSStrictPropertyOp>(JSContext *cx, JSStrictPropertyOp op, JSObject *obj,
>+                                    jsid id, jsval *vp)
>+{
>+    return op(cx, obj, id, false, vp);
>+}

Is this per-spec? Or no spec guidance yet from WebIDL? IIRC a lot of our native stuff reflected via XPConnect, possibly including DOM stuff (although not very old DOM level 0 APIs that predate try/catch being added in ES3) throw on something analogous to assigning to a readonly property. That seems more like strict = true than false.

Anyway r=me, main thing is does it compile and is any conditionally-compiled code covered.

/be

Comment 69

[Brendan Eich [:brendan]]

(In reply to comment #66)
> (In reply to comment #65)
> > The tests in attachment 508272 [details] [diff] [review] don't cover this patch, so I'm assuming tests of
> > the kind Waldo should review are forthcoming ;-).
> 
> Attachment 508272 [details] [diff] re-enables ecma_5/strict/jstests.list, which does check that
> an error gets thrown. Am I missing something?

tests/ecma_5/strict/15.4.5.1.js is pretty short. I don't see shift, pop, or other deleting array ops covered. But I didn't look elsewhere -- sorry if I am the one missing something.

/be

Comment 70

[Brendan Eich [:brendan]]

> old DOM level 0 APIs that predate try/catch being added in ES3) throw on

Missing "does" before "throw".

And come to think of it, my old Mocha engine in Netscape threw on readonly property update attempts. ES1 changed this to silent non-update with assignment expression result being the RHS and I changed to track in SpiderMonkey (the new rewrite of Mocha).

So native stuff should be "as if self-hosted in strict mode" if we are lucky.

/be

Comment 71

[Jeff Walden [:Waldo]]

(In reply to comment #68)
> Is this per-spec? Or no spec guidance yet from WebIDL?

WebIDL likely (and currently, I believe, but too lazy to check) will say that readonly attributes are represented by a property like { [[Get]]: ..., [[Enumerable]]: true, [[Configurable]]: true }.  So assignment to a readonly attribute should do nothing outside strict mode and throw a TypeError in strict mode.

Comment 72

[Jim Blandy :jimb]

(In reply to comment #68)
> Is this per-spec? Or no spec guidance yet from WebIDL? IIRC a lot of our native
> stuff reflected via XPConnect, possibly including DOM stuff (although not very
> old DOM level 0 APIs that predate try/catch being added in ES3) throw on
> something analogous to assigning to a readonly property. That seems more like
> strict = true than false.

The underlying issue here is that setters don't receive any indication of whether the assignment they're implementing occurs in strict or lenient code. If we had such an indication, we could pass it through here.

What I don't understand is why ReifyPropertyOps needs to generate JS function objects for the getter and setter, instead of just passing its |getter| and |setter| arguments directly through to JS_DefinePropertyById. If it could do the latter, then the strictness would propagate to the setter naturally.

But it goes to so much trouble to make the setter a JS function that I assumed it must have a good reason, and the remaining question was, is that function strict mode code or not?

ES5 decided to treat all its internal algorithms as strict mode code (passing true for all the |Throw| parameters of the object internal methods). It would be consistent with that to pass 'true' in ApplyPropertyOp. But ES5 could do what it did without many repercussions because until ES5, nobody could create non-configurable or non-writable properties anyway.

It seemed to me that passing false was the closest thing to the behavior one would get pre-ES5, and thus the more conservative choice.

But I don't have enough context to really think through the long-term consequences here. (In the immediate term, there are none, because no setters that can reach that point check their strictness.)

> Anyway r=me, main thing is does it compile and is any conditionally-compiled
> code covered.

It compiles; the try server likes it:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=38b0e1c2951d

Comment 73

[Jim Blandy :jimb]

(In reply to comment #72)
> The underlying issue here is that setters don't receive any indication of
> whether the assignment they're implementing occurs in strict or lenient code.

Sorry --- I should have specified, "JavaScript setters don't receive any indication...". (The whole point of the patch is to allow C++ setters to get this information!)

Comment 74

[Jim Blandy :jimb]

(In reply to comment #69)
> tests/ecma_5/strict/15.4.5.1.js is pretty short. I don't see shift, pop, or
> other deleting array ops covered. But I didn't look elsewhere -- sorry if I am
> the one missing something.

Each Array.prototype.mumble defined in ES5 section 15.4.4.x is tested in js/src/tests/ecma_5/strict/15.4.4.x.js. I believe Waldo reviewed those.

Comment 75

[Jim Blandy :jimb]

(In reply to comment #70)
> So native stuff should be "as if self-hosted in strict mode" if we are lucky.

So, the synthesized setters should be "as if strict", like (most of) the ES5 chapter 15 algorithms? Done.

Comment 76

[Jim Blandy :jimb]

Last remaining work item, then: checking that watchpoint setters propagate the strictness properly (the TODO item in https://bugzilla.mozilla.org/attachment.cgi?id=508272&action=diff). Will do tomorrow morning.

Comment 77

[Jim Blandy :jimb]

Attachment: Re-enable tests for assignments to array lengths in strict mode; add new regression tests.

Okay, added the tests I wanted; ready for review.

Comment 78

[Brendan Eich [:brendan]]

Comment on attachment 509323 [details] [diff] [review]
Re-enable tests for assignments to array lengths in strict mode; add new regression tests.

>Bug 537873: Re-enable tests for assignments to array lengths in strict mode; add new regression tests.
>
>diff --git a/js/src/tests/ecma_5/extensions/jstests.list b/js/src/tests/ecma_5/extensions/jstests.list
>--- a/js/src/tests/ecma_5/extensions/jstests.list
>+++ b/js/src/tests/ecma_5/extensions/jstests.list
>@@ -22,6 +22,7 @@ script strict-function-statements.js
> script strict-option-redeclared-parameter.js
> script string-literal-getter-setter-decompilation.js
> script uneval-strict-functions.js
>+script watch-array-length.js
> script watch-inherited-property.js
> script watch-replaced-setter.js
> script watch-setter-become-setter.js
>diff --git a/js/src/tests/ecma_5/extensions/watch-array-length.js b/js/src/tests/ecma_5/extensions/watch-array-length.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/extensions/watch-array-length.js
>@@ -0,0 +1,41 @@
>+/*
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/licenses/publicdomain/
>+ */
>+
>+var hitCount;
>+function watcher(p,o,n) { hitCount++; return n; }
>+
>+var a = [1];
>+a.watch('length', watcher);
>+hitCount = 0;
>+a.length = 0;
>+reportCompare(1, hitCount, "lenient; configurable: watchpoint hit");
>+
>+var b = Object.defineProperty([1],'0',{configurable:false});
>+b.watch('length', watcher);
>+hitCount = 0;
>+var result;
>+try {
>+    b.length = 0;    
>+    result = "no error";
>+} catch (x) {
>+    result = x.toString();
>+}
>+reportCompare(1, hitCount, "lenient; non-configurable: watchpoint hit");
>+reportCompare(1, b.length, "lenient; non-configurable: length unchanged");
>+reportCompare("no error", result, "lenient; non-configurable: no error thrown");
>+
>+var c = Object.defineProperty([1],'0',{configurable:false});
>+c.watch('length', watcher);
>+hitCount = 0;
>+var threwTypeError;
>+try {
>+    (function(){'use strict'; c.length = 0;})();
>+    threwTypeError = false;
>+} catch (x) {
>+    threwTypeError = x instanceof TypeError;
>+}
>+reportCompare(1, hitCount, "strict; non-configurable: watchpoint hit");
>+reportCompare(1, c.length, "strict; non-configurable: length unchanged");
>+reportCompare(true, threwTypeError, "strict; non-configurable: TypeError thrown");
>diff --git a/js/src/tests/ecma_5/strict/jstests.list b/js/src/tests/ecma_5/strict/jstests.list
>--- a/js/src/tests/ecma_5/strict/jstests.list
>+++ b/js/src/tests/ecma_5/strict/jstests.list
>@@ -25,7 +25,7 @@ script 15.4.4.8.js
> script 15.4.4.9.js
> script 15.4.4.12.js
> script 15.4.4.13.js
>-skip script 15.4.5.1.js  # Waiting for bug 537873 to be fully resolved.
>+script 15.4.5.1.js
> script 15.5.5.1.js
> script 15.5.5.2.js
> script 15.10.7.js
>@@ -35,6 +35,7 @@ script function-name-arity.js
> script primitive-this-no-writeback.js
> script regress-532254.js
> script regress-532041.js
>+script regress-599159.js
> script unbrand-this.js
> script this-for-function-expression-recursion.js
> script assign-to-callee-name.js
>diff --git a/js/src/tests/ecma_5/strict/regress-599159.js b/js/src/tests/ecma_5/strict/regress-599159.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/strict/regress-599159.js
>@@ -0,0 +1,33 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+
>+/*
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/licenses/publicdomain/
>+ */
>+
>+// Shu's test
>+function test(makeNonArray) {
>+    function C() {}
>+    C.prototype = []
>+    if (makeNonArray)
>+        C.prototype.constructor = C
>+    c = new C();
>+    c.push("foo");
>+    return c.length
>+}
>+assertEq(test(true), 1);
>+assertEq(test(false), 1);
>+
>+// jorendorff's longer test
>+var a = [];
>+a.slowify = 1;
>+var b = Object.create(a);
>+b.length = 12;
>+assertEq(b.length, 12);
>+
>+// jorendorff's shorter test
>+var b = Object.create(Array.prototype);
>+b.length = 12;
>+assertEq(b.length, 12);
>+
>+reportCompare(true, true);

Comment 79

[Brendan Eich [:brendan]]

Rats. Where's that upstream bugzilla fix not yet in b.m.o?

Test looks great, even if as a "oops I pulled a gal" comment. Still under the weather here so I may be missing some corners -- Waldo's your man if you want another review.

/be

Comment 80

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/48dbd9752e5e

Comment 81

[Jim Blandy :jimb]

Concerns have been raised that this will break binary extensions that use JSAPI silently. Filed as Bug 632948.

Comment 82

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d7a159f63e1a
http://hg.mozilla.org/mozilla-central/rev/b1f6d5fcafdb
http://hg.mozilla.org/mozilla-central/rev/26c98c344fab
http://hg.mozilla.org/mozilla-central/rev/5625f3beded9
http://hg.mozilla.org/mozilla-central/rev/4b56bfdf61a7
http://hg.mozilla.org/mozilla-central/rev/cae0570a5c4c
http://hg.mozilla.org/mozilla-central/rev/48dbd9752e5e