Closed Bug 886087 Opened 7 years ago Closed 7 years ago

Honor strict argument to JSObject::setProperty

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch v1 (obsolete) — Splinter Review
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.
Assignee: general → jorendorff
Attachment #766381 - Flags: review?(jimb)
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.
Attachment #766381 - Flags: review+
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.  :-(
Attached patch v2Splinter Review
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).
Attachment #766381 - Attachment is obsolete: true
Attachment #766381 - Flags: review?(jimb)
Attachment #767334 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/c8a1289735aa
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.