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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

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.

Comment 1

[Brendan Eich [:brendan]]

Who will own this one? It seems easy and possibly even important (do our security wrappers fail set traps ever?).

/be

Comment 2

[Jim Blandy :jimb]

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.

Comment 3

[Bobby Holley (:bholley)]

I've got patches for this. Taking.

Comment 4

[Bobby Holley (:bholley)]

Attachment: patch. v1

Attaching a patch. Flagging jimb for review.

Comment 5

[Luke Wagner [:luke]]

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

Comment 6

[Bobby Holley (:bholley)]

Pushed to try, along with some other patches: https://tbpl.mozilla.org/?tree=Try&rev=4d9ad69264c3

Comment 7

[Bobby Holley (:bholley)]

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/bba06c18d52d

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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