Last Comment Bug 596351 - Proxies should throw TypeErrors for assignments to read-only properties in ES5 strict mode
: Proxies should throw TypeErrors for assignments to read-only properties in ES...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 737251
Blocks: cpg
  Show dependency treegraph
 
Reported: 2010-09-14 13:13 PDT by Jim Blandy :jimb
Modified: 2012-03-19 16:41 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch. v1 (4.71 KB, patch)
2012-03-14 17:47 PDT, Bobby Holley (:bholley) (busy with Stylo)
luke: review+
Details | Diff | Splinter Review

Description Jim Blandy :jimb 2010-09-14 13:13:55 PDT
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] 2010-11-09 16:24:41 PST
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 2011-02-28 13:37:39 PST
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) (busy with Stylo) 2012-03-14 17:07:11 PDT
I've got patches for this. Taking.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-03-14 17:47:06 PDT
Created attachment 606039 [details] [diff] [review]
patch. v1

Attaching a patch. Flagging jimb for review.
Comment 5 Luke Wagner [:luke] 2012-03-16 09:32:14 PDT
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) (busy with Stylo) 2012-03-16 09:46:51 PDT
Pushed to try, along with some other patches: https://tbpl.mozilla.org/?tree=Try&rev=4d9ad69264c3
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-03-16 12:48:24 PDT
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/bba06c18d52d
Comment 8 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-17 17:09:45 PDT
https://hg.mozilla.org/mozilla-central/rev/bba06c18d52d

Note You need to log in before you can comment on or make changes to this bug.