Closed Bug 596351 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: jimb, Assigned: bholley)

References

Details

Attachments

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

/be
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]:
-----------------------------------------------------------------

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
Depends on: 737251
You need to log in before you can comment on or make changes to this bug.