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

VERIFIED FIXED

Status

()

VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: daumling, Unassigned)

Tracking

({verified1.8.1})

Trunk
x86
Windows XP
verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Updated

13 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 1

13 years ago
Created attachment 202144 [details] [diff] [review]
Bug fix
Attachment #202144 - Flags: review?(mrbkap)
(Reporter)

Comment 2

13 years ago
Created attachment 202145 [details]
Test case
Attachment #202145 - Flags: review?(bob)
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.
Attachment #202144 - Flags: superreview?(brendan)
Attachment #202144 - Flags: review?(mrbkap)
Attachment #202144 - Flags: review+
(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
(Reporter)

Comment 5

13 years ago
This patch is already very old. Brendan, can you super-review, please?
(Reporter)

Comment 6

13 years ago
Created attachment 204073 [details] [diff] [review]
Updated patch after fixing nits

Sorry - forgot to incorporate nits.
Attachment #202144 - Attachment is obsolete: true
Attachment #204073 - Flags: superreview?(brendan)
Attachment #202144 - Flags: superreview?(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
(Reporter)

Comment 8

13 years ago
Created attachment 204593 [details] [diff] [review]
Updated patch after Brendan's comments
Attachment #204073 - Attachment is obsolete: true
Attachment #204593 - Flags: superreview?(brendan)
Attachment #204073 - Flags: superreview?(brendan)
(Reporter)

Updated

13 years ago
Blocks: 318402
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
Attachment #204593 - Flags: superreview?(brendan) → superreview+
(Reporter)

Comment 10

13 years ago
Created attachment 204605 [details] [diff] [review]
As requested...
Attachment #204593 - Attachment is obsolete: true
Attachment #204605 - Flags: superreview?(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
Attachment #204605 - Flags: superreview?(brendan) → superreview-
(Reporter)

Comment 12

13 years ago
Created attachment 204609 [details] [diff] [review]
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...
Attachment #204605 - Attachment is obsolete: true
Attachment #204609 - Flags: superreview?(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
Attachment #204609 - Flags: superreview?(brendan) → superreview-
(Reporter)

Comment 14

13 years ago
Created attachment 204625 [details] [diff] [review]
YAP - Yet Another Patch...
Attachment #204625 - Flags: superreview?(brendan)
(Reporter)

Updated

13 years ago
Attachment #204609 - Attachment is obsolete: true
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
Attachment #204625 - Flags: superreview?(brendan)
Created attachment 204626 [details] [diff] [review]
patch I'm checking in r=me crediting Michael
Attachment #204625 - Attachment is obsolete: true
Attachment #204626 - Flags: review+
(Reporter)

Comment 17

13 years ago
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
Blocks: 309169
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Created attachment 204628 [details] [diff] [review]
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
Attachment #204628 - Flags: review?(daumling)
(Reporter)

Comment 20

13 years ago
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.
Attachment #204628 - Flags: review?(daumling) → review+
Created attachment 204727 [details] [diff] [review]
followup patch I checked in
Attachment #204628 - Attachment is obsolete: true
Attachment #204727 - Flags: review+

Comment 22

13 years ago
/cvsroot/mozilla/js/tests/ecma_3/Object/8.6.1-01.js,v  <--  8.6.1-01.js
initial revision: 1.1
Flags: testcase+

Comment 23

13 years ago
verified windows/linux/mac trunk.
Status: RESOLVED → VERIFIED

Comment 24

13 years ago
js16 fixes should be in ff2 me thinks.
Flags: blocking1.8.1?

Comment 25

12 years ago
Brendan/Bkap is this suitable/desirable for the branch?  If so can you set the 1.8.1 non on the patch
Flags: blocking1.8.1? → blocking1.8.1-
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

Updated

12 years ago
Summary: Setting a read-only property does not generate a warning in strict mode → Setting a read-only property does not generate a warning in strict mode (ecma_3/Object/8.6.1-01.js)

Comment 27

12 years ago
fixed by Bug 336373 on the 1.8.1 branch. 
verified fixed 1.8.1 with windows/macppc/linux 20060707
Keywords: verified1.8.1

Comment 28

12 years ago
not fixed on 1.8.0, not making js16
No longer blocks: 309169

Updated

12 years ago
Attachment #202145 - Flags: review?(bclary)
You need to log in before you can comment on or make changes to this bug.