315436 - Setting a read-only property does not generate a warning in strict mode (ecma_3/Object/8.6.1-01.js)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Michael Daumling]

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 1

[Michael Daumling]

Attachment: Bug fix

Comment 2

[Michael Daumling]

Attachment: Test case

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 4

[Brendan Eich [:brendan]]

(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

Comment 5

[Michael Daumling]

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

Comment 6

[Michael Daumling]

Attachment: Updated patch after fixing nits

Sorry - forgot to incorporate nits.

Comment 7

[Brendan Eich [:brendan]]

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 8

[Michael Daumling]

Attachment: Updated patch after Brendan's comments

Comment 9

[Brendan Eich [:brendan]]

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 10

[Michael Daumling]

Attachment: As requested...

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Michael Daumling]

Attachment: Fix logic bug...

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 13

[Brendan Eich [:brendan]]

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 14

[Michael Daumling]

Attachment: YAP - Yet Another Patch...

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[Brendan Eich [:brendan]]

Attachment: patch I'm checking in r=me crediting Michael

Comment 17

[Michael Daumling]

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

Comment 18

[Brendan Eich [:brendan]]

Noting js1.6 dependency.

/be

Comment 19

[Brendan Eich [:brendan]]

Attachment: followup fix to consolidate error reporting code at read_only_error: label

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 20

[Michael Daumling]

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.

Comment 21

[Brendan Eich [:brendan]]

Attachment: followup patch I checked in

Comment 22

[Bob Clary [:bc] (inactive)]

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

Comment 23

[Bob Clary [:bc] (inactive)]

verified windows/linux/mac trunk.

Comment 24

[Bob Clary [:bc] (inactive)]

js16 fixes should be in ff2 me thinks.

Comment 25

[Mike Schroepfer]

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

Comment 26

[Brendan Eich [:brendan]]

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

Comment 27

[Bob Clary [:bc] (inactive)]

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

Comment 28

[Bob Clary [:bc] (inactive)]

not fixed on 1.8.0, not making js16