The default bug view has changed. See this FAQ.

Proxies should throw TypeErrors for assignments to read-only properties in ES5 strict mode

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jimb, Assigned: bholley)

Tracking

unspecified
mozilla14
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Even once bug 537873 has landed, SpiderMonkey proxies will not throw a TypeError when the 'set' trap returns false (indicating that the assignment failed). If an assignment written in strict mode code invokes a proxy's 'set' trap, and that trap returns false, the proxy implementation in SpiderMonkey should detect that and throw a TypeError.
Who will own this one? It seems easy and possibly even important (do our security wrappers fail set traps ever?).

/be
(Reporter)

Comment 2

6 years ago
As an aside, JSWrapper::set didn't get fixed up when we change the setters' type to JSStrictPropertyOp; it passes 'false' for the strictness flag, even though it now has the real assignment's strictness available.

Propagating strictness through will fix the case where the proxy is handing off the assignment to a JS object. But for other sorts of proxies, JSProxyHandler::set (see the |desc.attrs & JSPROP_READONLY| cases) and its overriders should throw when the assignment is strict, instead of just returning true.
Assignee: general → jimb
I've got patches for this. Taking.
Assignee: jimb → bobbyholley+bmo
Blocks: 650353
Created attachment 606039 [details] [diff] [review]
patch. v1

Attaching a patch. Flagging jimb for review.
Attachment #606039 - Flags: review?(jimb)

Comment 5

5 years ago
Comment on attachment 606039 [details] [diff] [review]
patch. v1

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

Stealing

::: js/src/jsproxy.cpp
@@ +186,5 @@
> +        if (desc.attrs & JSPROP_READONLY) {
> +            if (strict) {
> +                JSAutoByteString bytes(cx, JSID_TO_STRING(id));
> +                if (!bytes)
> +                    return JS_FALSE;

'return false'

@@ +190,5 @@
> +                    return JS_FALSE;
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                                     JSMSG_CANT_REDEFINE_PROP, bytes.ptr());
> +            }
> +            return !strict;

I know its shorter, but for easy of reading, I'd rather see:

    return false;
  }
  return true;

@@ +225,5 @@
> +                    return JS_FALSE;
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                                     JSMSG_CANT_REDEFINE_PROP, bytes.ptr());
> +            }
> +            return !strict;

ditto and ditto
Attachment #606039 - Flags: review?(jimb) → review+
Pushed to try, along with some other patches: https://tbpl.mozilla.org/?tree=Try&rev=4d9ad69264c3
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/bba06c18d52d
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/bba06c18d52d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 737251
You need to log in before you can comment on or make changes to this bug.