Assignment to readonly properties should throw a TypeError in strict mode

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Waldo, Assigned: jimb)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla1.9.3a1
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: softblocker, fixed-in-tracemonkey)

Attachments

(10 attachments, 5 obsolete attachments)

29.12 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.20 KB, patch
luke
: review+
Details | Diff | Splinter Review
64.84 KB, patch
brendan
: review+
Details | Diff | Splinter Review
579 bytes, patch
brendan
: review+
Details | Diff | Splinter Review
563 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.75 KB, patch
brendan
: review+
Details | Diff | Splinter Review
147.24 KB, patch
brendan
: review+
Details | Diff | Splinter Review
4.44 KB, patch
brendan
: review+
Details | Diff | Splinter Review
50.06 KB, patch
brendan
: review+
Details | Diff | Splinter Review
3.73 KB, patch
brendan
: review+
Details | Diff | Splinter Review
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...
Blocks: 482298
blocking2.0: --- → ?

Updated

7 years ago
blocking2.0: ? → beta1+
Keywords: dev-doc-needed

Updated

7 years ago
blocking2.0: beta1+ → beta2+
(Assignee)

Updated

7 years ago
No longer blocks: 445494

Updated

7 years ago
blocking2.0: beta2+ → betaN+
(Assignee)

Updated

7 years ago
Assignee: jwalden+bmo → jim
(Assignee)

Comment 2

7 years ago
Stolen with permission. (Would prefer stollen with butter, but, whatever.)
Don't noodle away on the problem too long, we have deadlines to make.

Updated

7 years ago
blocking2.0: betaN+ → beta5+

Comment 4

7 years ago
Can we get an update here?
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Comment 6

7 years ago
Created attachment 469292 [details] [diff] [review]
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.

Updated

7 years ago
blocking2.0: beta5+ → beta6+
(Assignee)

Comment 7

7 years ago
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
(Assignee)

Comment 8

7 years ago
Created attachment 470508 [details] [diff] [review]
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.
(Assignee)

Updated

7 years ago
Blocks: 590690
(Assignee)

Comment 9

7 years ago
All tests pass, except for assignments to functions' 'name' and 'arity' properties. Should be quick.
(Assignee)

Comment 10

7 years ago
Created attachment 473250 [details] [diff] [review]
Use ObjectOps::setProperty for both fast and slow arrays.
(Assignee)

Comment 11

7 years ago
Created attachment 473251 [details] [diff] [review]
Tests for strict mode assignments to read-only properties, deletions of non-configurable properties. [committed]
Attachment #469292 - Attachment is obsolete: true
(Assignee)

Comment 12

7 years ago
Created attachment 473252 [details] [diff] [review]
Delete unused property operation typedefs: JSDefinePropOp, JSPropertyIdOp. [committed]
(Assignee)

Comment 13

7 years ago
Created attachment 473253 [details] [diff] [review]
Have strict mode code report TypeErrors for assignments, deletions. [committed]
Attachment #470508 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
Apparently SunSpider-neutral.
(Assignee)

Updated

7 years ago
Attachment #473251 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

7 years ago
Attachment #473253 - Flags: review?(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
Attachment #473253 - Flags: review?(brendan) → review+
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 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...
Attachment #473251 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 18

7 years ago
(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.
(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
(Assignee)

Comment 20

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

The comment in the patch explains the rationale.
Attachment #473250 - Flags: review?(brendan)

Updated

7 years ago
Attachment #473252 - Flags: review+
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
Attachment #473250 - Flags: review?(brendan) → review+
(Assignee)

Comment 22

7 years ago
> 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.
(Assignee)

Comment 23

7 years ago
(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.
(Assignee)

Comment 24

7 years ago
(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. :)
(Assignee)

Comment 25

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

Done.

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

Done.
(Assignee)

Updated

7 years ago
Attachment #473251 - Flags: review?(jorendorff)
Attachment #473251 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 26

7 years ago
http://hg.mozilla.org/tracemonkey/rev/a409054e1395
Whiteboard: [fixed-in-tracemonkey]
(Assignee)

Comment 27

7 years ago
bug 596728 filed for the error messages.

Comment 28

7 years ago
http://hg.mozilla.org/mozilla-central/rev/a55097cd48d8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 599159
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
(Assignee)

Comment 30

7 years ago
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.
Don't reopen, file a new bug please -- this one's pretty crowded already.
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
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.
Depends on: 612260
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. :)
(Assignee)

Updated

7 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 35

7 years ago
Reopened, as back-out bug 599159 has landed.
Since this was blocking beta 7, should it now block 8?

Updated

7 years ago
blocking2.0: beta7+ → beta9+

Updated

7 years ago
Whiteboard: [fixed-in-tracemonkey]
Whiteboard: softblocker

Comment 37

7 years ago
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)
No longer blocks: 482298, 590690, 537863
blocking2.0: beta9+ → betaN+
No longer depends on: 599159, 612260
Keywords: dev-doc-needed

Comment 38

7 years ago
Fixing fields my automated script accidentally blanked. Apologies for the bugspam
Keywords: dev-doc-needed

Updated

7 years ago
Blocks: 482298, 590690, 537863
Depends on: 599159, 612260
(Assignee)

Updated

7 years ago
Attachment #473253 - Attachment description: Have strict mode code report TypeErrors for assignments, deletions. → Have strict mode code report TypeErrors for assignments, deletions. [committed]
(Assignee)

Updated

7 years ago
Attachment #473252 - Attachment description: Delete unused property operation typedefs: JSDefinePropOp, JSPropertyIdOp. → Delete unused property operation typedefs: JSDefinePropOp, JSPropertyIdOp. [committed]
(Assignee)

Updated

7 years ago
Attachment #473251 - Attachment description: Tests for strict mode assignments to read-only properties, deletions of non-configurable properties. → Tests for strict mode assignments to read-only properties, deletions of non-configurable properties. [committed]
(Assignee)

Comment 39

7 years ago
I've begun working on this.
(Assignee)

Comment 40

7 years ago
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.
(Assignee)

Comment 41

7 years ago
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     --
(Assignee)

Comment 42

7 years ago
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.
(Assignee)

Comment 43

7 years ago
Created attachment 507293 [details] [diff] [review]
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 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
Cc'ing Nick on comment 42 and prior.

/be
(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".
(Assignee)

Comment 47

7 years ago
(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?
(Assignee)

Comment 48

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

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

Comment 49

7 years ago
(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.
(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.
(Assignee)

Comment 51

7 years ago
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.
Told ya! :)  Where are the cg_diff patches?
(Assignee)

Comment 53

7 years ago
(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
Component: JavaScript Engine → jemalloc
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!  ;-) )
Component: jemalloc → JavaScript Engine
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 55

7 years ago
Created attachment 508270 [details] [diff] [review]
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.
Attachment #508270 - Flags: review?(brendan)
(Assignee)

Comment 56

7 years ago
Created attachment 508271 [details] [diff] [review]
Avoid writing past the end of ScriptObjectFixture::uc_code.

This silences a warning when building testScriptObject.o under GCC -O3.
Attachment #508271 - Flags: review?(jorendorff)
(Assignee)

Comment 57

7 years ago
Created attachment 508272 [details] [diff] [review]
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
(Assignee)

Comment 58

7 years ago
Created attachment 508273 [details] [diff] [review]
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.
Attachment #508273 - Flags: review?(brendan)
(Assignee)

Comment 59

7 years ago
Created attachment 508274 [details] [diff] [review]
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.
Attachment #507293 - Attachment is obsolete: true
Attachment #508274 - Flags: review?(brendan)
(Assignee)

Comment 60

7 years ago
Created 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.

This is the patch that actually fixes the bug.
Attachment #473250 - Attachment is obsolete: true
Attachment #508275 - Flags: review?(brendan)
(Assignee)

Comment 61

7 years ago
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.
Attachment #508271 - Flags: review?(jorendorff) → review+
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
Attachment #508270 - Flags: review?(brendan) → review+
Attachment #508273 - Flags: review?(brendan) → review+
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
Attachment #508274 - Flags: review?(brendan) → review+
(Assignee)

Comment 64

7 years ago
(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 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
Attachment #508275 - Flags: review?(brendan) → review+
(Assignee)

Comment 66

7 years ago
(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?
(Assignee)

Comment 67

7 years ago
Created attachment 509033 [details] [diff] [review]
Add 'strict' argument to setters defined throughout Firefox.

This should take care of all setters defined outside SpiderMonkey proper. Very mechanical.
Attachment #509033 - Flags: review?(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
Attachment #509033 - Flags: review?(brendan) → review+
(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
> 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
(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.
(Assignee)

Comment 72

7 years ago
(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
(Assignee)

Comment 73

7 years ago
(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!)
(Assignee)

Comment 74

7 years ago
(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.
(Assignee)

Comment 75

7 years ago
(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.
(Assignee)

Comment 76

7 years ago
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.
(Assignee)

Comment 77

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

Okay, added the tests I wanted; ready for review.
Attachment #508272 - Attachment is obsolete: true
Attachment #509323 - Flags: review?(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);
Attachment #509323 - Flags: review?(brendan) → review+
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
(Assignee)

Comment 80

7 years ago
http://hg.mozilla.org/tracemonkey/rev/48dbd9752e5e
Whiteboard: softblocker → softblocker, fixed-in-tracemonkey
(Assignee)

Comment 81

7 years ago
Concerns have been raised that this will break binary extensions that use JSAPI silently. Filed as Bug 632948.
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
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.