449657 - JS_SealObject fails on Array objects

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christopher Lenz]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_4; en-us) AppleWebKit/525.18 (KHTML, like Gecko) Version/3.1.2 Safari/525.20.1
Build Identifier: CVS HEAD 2008-08-07

JS_SealObject seems to fail on array objects in SpiderMonkey from HEAD (1.9?), at least from the JS shell. It works correctly on the 1.8 branch.



Reproducible: Always

Steps to Reproduce:
1. Run the js shell
2. Enter: var a= [1, 2, 3]; seal(a);

Actual Results:  
typein:2: Error: can't seal Array objects



Expected Results:  
No output, array object sealed

The OBJ_IS_NATIVE check in JS_SealObject seems to be failing for Array objects, resulting in the above error.

Getting the same error when deep sealing some object that contains an array somewhere.

Comment 1

[ash_mozilla]

Can confirm that a checkout of tracemonkey tip as of about 1 hour ago still suffers this problem (I'm also on an Intel Mac if that makes any difference)

Comment 2

[ash_mozilla]

Attachment: Possible patch

This patch seems to work from some simple testing, but it gives a slightly bogus error message:

$ echo 'a = [1,2,3]; a[4] = 2; seal(a); a.length = 5' | dist/bin/js 
1: Error: 0 is read-only

Comment 3

[ash_mozilla]

(In reply to comment #2)
> Created an attachment (id=351282) [details]
> Possible patch
> 
> This patch seems to work from some simple testing, but it gives a slightly
> bogus error message:
> 
> $ echo 'a = [1,2,3]; a[4] = 2; seal(a); a.length = 5' | dist/bin/js 
> 1: Error: 0 is read-only

Okay, no it doesn't. Array literal syntax is broken (that is the cause of 0 is read-only)

js> a = new Array()

js> seal(a)
js> a.length = 5
typein:16: Error: a.length is read-only
js> a.length
0
js> a[0] = 1
typein:18: Error: a[0] is read-only
js> a[4] = 5
typein:19: Error: a[4] is read-only
js> a[200000] = 6
typein:20: Error: a[200000] is read-only
js> [1,2,3]
typein:21: Error: 0 is read-only
js> a[0]
js> a = new Array()

js> a[0] = 2
2
js> a
2
js> seal(a)
js> a[1] = 2
2
js> [0]
0
js> [0,1]
typein:16: Error: 0 is read-only
js> [1]
typein:17: Error: 0 is read-only
js> 

All kinda of broken going on there.

Comment 4

[Brendan Eich [:brendan]]

The patch is wrong on several counts. What's needed here is code under the JS_SealObject API that special-cases dense arrays via ENSURE_SLOW_ARRAY.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix

This is fair ride-along fodder for 1.9.1.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 351282 [details] [diff] [review]
Possible patch

Ash, thanks for patching -- don't mean to sound ungrateful. In the spirit of spreading know-how, I'm reviewing even though this patch isn't right. Hope this helps, feel free to ask questions.

/be

> array_setProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
> {
>+    JSScope *scope;
>     uint32 i;
> 
>+    scope = OBJ_SCOPE(obj);
>+    if (SCOPE_IS_SEALED(scope)) {
>+        // I'm fairly sure these flags aren't right.
>+        return js_ReportValueErrorFlags(cx, JSREPORT_ERROR,
>+                                        JSMSG_READ_ONLY, JSDVG_IGNORE_STACK, 
>+                                        ID_TO_VALUE(id), NULL, NULL, NULL);
>+    }

This is not necessary if the dense array is "made slow" by js_MakeArraySlow. In that event OBJ_SET_PROPERTY vectors to js_SetProperty which does the right thing.

>@@ -781,6 +790,7 @@
>         return js_SetProperty(cx, obj, id, vp);
>     }
> 
>+

Don't add extra blank lines.

> /* Test whether a map or object is native. */
> #define MAP_IS_NATIVE(map)                                                    \
>     JS_LIKELY((map)->ops == &js_ObjectOps ||                                  \
>+              (map)->ops == &js_ArrayObjectOps ||                             \

This is wrong, since dense arrays do not have maps that point to JSScopes. But it's not necessary given the fix attached as the second patch in this bug.

Comment 7

[Brendan Eich [:brendan]]

So this is a regression in Firefox 3 to the JS_SealObject API. Should unregress for 1.9.1, not a blocker given the lack of Firefox usage of this API (AFAIK).

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 351294 [details] [diff] [review]
fix

This is good (I have the same patch in my tree), but ash on IRC noticed that this doesn't protect against setting a.length on a sealed array.

Also, would you mind casting cx to (void *) in the printf on line 822?

Comment 9

[ash_mozilla]

(In reply to comment #6)
> (From update of attachment 351282 [details] [diff] [review])
> Ash, thanks for patching -- don't mean to sound ungrateful. In the spirit of
> spreading know-how, I'm reviewing even though this patch isn't right. Hope this
> helps, feel free to ask questions.
> 

mrbkap said pretty much the same things IRC. I'll be the first to admit that I
was poking about in the dark - I had no real idea what the object maps were all
about.

(In reply to comment #8)
> (From update of attachment 351294 [details] [diff] [review])
> This is good (I have the same patch in my tree), but ash on IRC noticed that
> this doesn't protect against setting a.length on a sealed array.
> 
> Also, would you mind casting cx to (void *) in the printf on line 822?

I can't reproduce that anymore, so I'm going to chalk it down to me not quite changing something right.

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix, v2

Thanks to Blake for testing the length-set case.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: fix, v3

Thanks again to Blake for tag-teaming this into the mat.

/be

Comment 12

[Brendan Eich [:brendan]]

Fixed in m-c:

http://hg.mozilla.org/mozilla-central/rev/bc71507c7ccb

/be

Comment 13

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/49e1d6bf0716
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-449657.js,v  <--  regress-449657.js
initial revision: 1.1

Comment 14

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e7403f2cbd61

Comment 15

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2