In strict mode, a JSMSG_READ_ONLY warning should be generated when attempting to set a read-only object property. Currently, the operation fails silently.

Comment on attachment 202144 [details] [diff] [review] Bug fix Nits only: >Index: jsobj.c >@@ -3000,18 +3000,30 @@ js_SetProperty(JSContext *cx, JSObject * >- if ((attrs & JSPROP_READONLY) && JS_VERSION_IS_ECMA(cx)) >- return JS_TRUE; >+ if ((attrs & JSPROP_READONLY) && JS_VERSION_IS_ECMA(cx)) { >+ Nit: empty blank line. >+ JSString *str = js_DecompileValueGenerator(cx, >+ JSDVG_IGNORE_STACK, >+ ID_TO_VALUE(id), >+ NULL); >+ if (str) >+ return JS_ReportErrorFlagsAndNumberUC(cx, JSREPORT_STRICT|JSREPORT_WARNING, >+ js_GetErrorMessage, NULL, >+ JSMSG_READ_ONLY, >+ JS_GetStringChars(str)); >+ else >+ return JS_TRUE; Nit: house style is to overbrace when the then-clause is multiline (even if the language doesn't strictly require it), so this becomes: if (str) { ... } else { ... } Other than that, looks good.

(In reply to comment #3) > >+ if (str) > >+ return JS_ReportErrorFlagsAndNumberUC(cx, JSREPORT_STRICT|JSREPORT_WARNING, > >+ js_GetErrorMessage, NULL, > >+ JSMSG_READ_ONLY, > >+ JS_GetStringChars(str)); > >+ else Else after return is a non-sequitur, just get rid of it and unindent the else clause. /be

This patch is already very old. Brendan, can you super-review, please?

Sorry - forgot to incorporate nits.

Comment on attachment 204073 [details] [diff] [review] Updated patch after fixing nits >- if ((attrs & JSPROP_READONLY) && JS_VERSION_IS_ECMA(cx)) >+ if ((attrs & JSPROP_READONLY) && JS_VERSION_IS_ECMA(cx)) { >+ JSString *str = js_DecompileValueGenerator(cx, >+ JSDVG_IGNORE_STACK, >+ ID_TO_VALUE(id), >+ NULL); This is fallible and expensive, so you don't want to do it (or the dependent if-then below) unless JS_HAS_STRICT_OPTION(cx). >+ if (str) >+ return JS_ReportErrorFlagsAndNumberUC(cx, JSREPORT_STRICT|JSREPORT_WARNING, >+ js_GetErrorMessage, NULL, >+ JSMSG_READ_ONLY, >+ JS_GetStringChars(str)); Nit: multi-line then part should be braced. sr=me with these changes. /be

Comment on attachment 204593 [details] [diff] [review] Updated patch after Brendan's comments >- if ((attrs & JSPROP_READONLY) && JS_VERSION_IS_ECMA(cx)) >+ if ((attrs & JSPROP_READONLY) >+ && JS_VERSION_IS_ECMA(cx) >+ && JS_HAS_STRICT_OPTION(cx)) { Prevailing style for multi-line logical connectives puts && and || at the end of each line, and underhangs terms so they all (including the first term) start in the same column. >+ JSString *str = js_DecompileValueGenerator(cx, >+ JSDVG_IGNORE_STACK, >+ ID_TO_VALUE(id), >+ NULL); >+ if (str) { if (!str) return JS_FALSE, since there was an error reported (probably OOM). That nicely collapses and unindents this: >+ return JS_ReportErrorFlagsAndNumberUC(cx, JSREPORT_STRICT|JSREPORT_WARNING, >+ js_GetErrorMessage, NULL, >+ JSMSG_READ_ONLY, >+ JS_GetStringChars(str)); >+ } > return JS_TRUE; sr=me with these changes, thanks. /be

Comment on attachment 204605 [details] [diff] [review] As requested... >+ if ((attrs & JSPROP_READONLY) && >+ JS_VERSION_IS_ECMA(cx) && >+ JS_HAS_STRICT_OPTION(cx)) { Whoops, this is a logic bug. You do not want ECMA-version script to goto read_only_error if the strict option is not set. There needs to be an inner if (JS_HAS_STRICT_OPTION(cx)) {...} and then a return JS_TRUE; in the outer if's "then" block. Sorry I didn't spot this before. /be

Oops...my bad... I hope the indentation of the call to JS_ReportErrorFlagsAndNumberUC() is OK - I had to unindent the args a bit because they hit the 80 column mark...

Comment on attachment 204609 [details] [diff] [review] Fix logic bug... >+ if (str) Again, if (!str) return JS_FALSE here, then you don't need to overindent this: >+ return JS_ReportErrorFlagsAndNumberUC(cx, >+ JSREPORT_STRICT | >+ JSREPORT_WARNING, >+ js_GetErrorMessage, >+ NULL, >+ JSMSG_READ_ONLY, >+ JS_GetStringChars(str)); It's important to return false if an error was reported (OOM) or thrown as an exception (any error other than OOM). /be

Comment on attachment 204625 [details] [diff] [review] YAP - Yet Another Patch... Thanks, I have a slightly tweaked version of this patch coming up. Sorry to cause so many iterations. Good fix, thanks for doing the essential work. /be

No problem - I am still at the beginning of the learning curve. Looking forward to knowing the ruleset of code formatting inside out :)

Noting js1.6 dependency. /be

I should have thought of this sooner. The sealed object condition also causes readonly errors (not strict warnings; sealing is an extension to ECMA), so it makes sense to share code at this label. Also avoids overindenting! Bob, please check cvs diff if you apply patches to update the js1.6rc1 mini-branch so as to be sure to get all patches, including followups such as this one. Enlist me in that effort, I will help. Thanks, /be

Comment on attachment 204628 [details] [diff] [review] followup fix to consolidate error reporting code at read_only_error: label Elegant solution! The re-use of flags confused me a little to begin with - maybe a comment would be appropriate.

/cvsroot/mozilla/js/tests/ecma_3/Object/8.6.1-01.js,v <-- 8.6.1-01.js initial revision: 1.1

verified windows/linux/mac trunk.

js16 fixes should be in ff2 me thinks.

Brendan/Bkap is this suitable/desirable for the branch? If so can you set the 1.8.1 non on the patch

These fixes are all coming with js1.7. I don't think it's worth going through individual nominations. Let's optimize and manage by exception. If a query shows anything you think should *not* be on the branch, please cite it. /be

fixed by Bug 336373 on the 1.8.1 branch. verified fixed 1.8.1 with windows/macppc/linux 20060707

not fixed on 1.8.0, not making js16