886087 - Honor strict argument to JSObject::setProperty

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

ES5 specifies that some functions do strict assignments. We get these wrong. For example:

    js> var obj = {pop: [].pop, 0: "zero", get length() { return 1; }};
    js> obj.pop()
    "zero"
    js> "use strict"; obj.pop();
    typein:3:14 TypeError: setting a property that has only a getter

It should throw because Array.prototype.pop does a strict assignment to obj.length, which has a getter and no setter. Whether the caller is strict shouldn't matter.

Chrome and Safari do a non-strict assignment in both cases.

We should try to follow the spec.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

I reviewed the patch that introduced this behavior, in bug 536306; using the caller's strictness was an improvement over the previous behavior, which was to crash.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 766381 [details] [diff] [review]
v1

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

Take it if you want it.  :-)  I had to look because I thought bug 858381 proved this stuff all worked -- but I guess array length special-casing made things work for non-writable length, but not for anything along these lines.  Blargh.

Were it me I might plausibly hold off on landing til after the branch, just in case something stupid warrants a temporary backout that'd have to span release branches.  Could be paranoia, tho; up to you.

::: js/src/jsobj.cpp
@@ +4985,5 @@
>      return JS_ReportErrorFlagsAndNumber(cx,
> +                                        strict
> +                                        ? JSREPORT_ERROR 
> +                                        : JSREPORT_WARNING | JSREPORT_STRICT |
> +                                          JSREPORT_STRICT_MODE_ERROR,

Is JSREPORT_STRICT_MODE_ERROR even necessary here any more, now that strict is passed in by the caller?

Looking more broadly, it looks like almost every single use of JSREPORT_STRICT_MODE_ERROR these days is in a context where we *know* strictness, and should be testing and passing it directly.  (The only exception I see is dom/bindings/DOMJSProxyHandler.cpp's use, which we can only eliminate with MOP changes from bug 826587.  I can't wait til all these other projects get done, to get back to working on that.  :-\ )  We should change these in some sort of followup, although you might as well fix this one while you're here.

Comment 3

[Jeff Walden [:Waldo]]

Oh, for what it's worth all those set-length calls before bug 858381 were passing false -- I should have thought to have tested SetLengthProperty's changes there with a getter-only length accessor when writing tests there.  :-(

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2

Carrying forward review. The only changes here are to tests:

- Remove a few tests involving array.sort() from
  js/src/tests/js1_6/extensions/regress-414098.js

- Add more thorough array.sort() tests in
  js/src/jit-test/tests/arrays/sort-getter-only.js

But as a result, this now has wait for assertEqual (bug 866849 part 1).

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 767334 [details] [diff] [review]
v2

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

::: js/src/jsobj.cpp
@@ +4991,4 @@
>  {
>      return JS_ReportErrorFlagsAndNumber(cx,
> +                                        strict
> +                                        ? JSREPORT_ERROR 

trailing ws

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c8a1289735aa

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0711903a413e