224306 - can't mutate empty object delegating to sealed prototype object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

If you JS_SealObject(cx, proto, JS_FALSE); obj = JS_NewObject(cx, clasp, proto,
NULL); v = JSVAL_ZERO; JS_SetProperty(cx, obj, "foo", &v); (or in script, you
set obj.foo = 0), the code at
http://lxr.mozilla.org/mozilla/source/js/src/jsobj.c#2780 will throw a read only
error.

This is easy to patch.  I wonder whether, at the same time, I should not patch
things so sealed object (scopes) do not require non-flyweight (thin or fat) locks.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix

I had to think and flail a bit to arrive at the cx->lockedSealedScope solution.
 To make up for yet another (JS_THREADSAFE only) cx member, I finally moved
codePool and tempPool to the C stack, with pointers to them in JSCodeGenerator.
 Finally, I removed JS_UnsealObject and its use in js.c.  When in doubt, leave
it out -- and it made the sealed-object-scope-avoids-locking goal very hard to
achieve.

diff -w version next.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

Comment 3

[Brendan Eich [:brendan]]

Patch shaves 100 bytes of text off my -g -O libmozjs.so build.

Testcase coming up.

/be

Comment 4

[Phil Schwartau]

> Finally, I removed JS_UnsealObject and its use in js.c.

So in the JS shell, we'll still be able to seal objects, but not unseal them?

Comment 5

[Brendan Eich [:brendan]]

Phil: right.  The js shell support is just for testing.  The lock-free advantage
to using a sealed tree of standard objects via a "superglobal" prototype to the
global object.  There's no use-case to justify unsealing, and I don't know how
to make it thread-safe given the lock-free optimization in this bug's patch.

/be

Comment 6

[Brendan Eich [:brendan]]

>  The lock-free advantage to using a sealed tree of standard objects via a
> "superglobal" prototype to the global object.

Oops, forgot to finish that sentence: "The lock-free advantage [...] outweighs
the undemonstrated utility of JS_UnsealObject."

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: add -P (for sealed superProto global) option for testing

and fixed an overstrict assertion in JS_SealObject.  I made parallel changes to
xpcshell and ran it with the -P option on
js/src/xpconnect/tests/js/old/threads.js and passed once I had set
XPCOM_DEBUG_BREAK to warn in gdb (but when I don't run under gdb, I get
assertion botches in js/jsd/jsd_lock.c sometimes, in ASSERT_VALID_LOCK --
rginda, any ideas?).

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: diff -w of last patch

Comment 9

[Phil Schwartau]

 static int
 usage(void)
 {
     fprintf(gErrFile, "%s\n", JS_GetImplementationVersion());
-    fprintf(gErrFile, "usage: js [-s] [-w] [-W] [-b branchlimit]   etc.
+    fprintf(gErrFile, "usage: js [-PswW] [-b branchlimit]          etc.


The new -P option seems more esoteric than -s, -w, -W.
Shouldn't it get more of an explanation, e.g. like [-b branchlimit]?

Or would it be too ungainly?

Comment 10

[Brendan Eich [:brendan]]

-P is esoteric.  Unless we want to use gnu getopt_long, and support --superproto
or some such, I see no way to explain things.  The [-b branchlimit] usage
message part works because -b takes a parameter, which is what branchlimit
stands for.  No param for -P.  With gnu getopt_long, we could have
-P|--superproto.  File an RFE?

/be

Comment 11

[Robert Ginda]

Sorry, no idea on the jsd_lock assert botches.  jsd_lock.c is jband's original
code, he may have a clue.

Comment 12

[Brendan Eich [:brendan]]

Where's jband?  For that matter, where's shaver?  Saw him in another bug
recently, so I'm hopeful!

/be

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 135042 [details] [diff] [review]
diff -w of last patch

Brendan talked me through this on IRC, and I'm on-side. r=shaver

Comment 14

[Brendan Eich [:brendan]]

Fixed.

/be