Closed Bug 596351 Opened 11 years ago Closed 9 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jimb, Assigned: bholley)




(1 file)

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?).

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: cpg
Attached patch patch. v1Splinter Review
Attaching a patch. Flagging jimb for review.
Attachment #606039 - Flags: review?(jimb)
Comment on attachment 606039 [details] [diff] [review]
patch. v1

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


::: 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:
Pushed to m-i:
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.