Closed
Bug 224306
Opened 21 years ago
Closed 21 years ago
can't mutate empty object delegating to sealed prototype object
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.6beta
People
(Reporter: brendan, Assigned: brendan)
Details
(Keywords: js1.5)
Attachments
(2 files, 2 obsolete files)
48.46 KB,
patch
|
Details | Diff | Splinter Review | |
48.07 KB,
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•21 years ago
|
Assignee | ||
Comment 1•21 years ago
|
||
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
Assignee | ||
Comment 2•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #134947 -
Flags: review?(shaver)
Assignee | ||
Comment 3•21 years ago
|
||
Patch shaves 100 bytes of text off my -g -O libmozjs.so build. Testcase coming up. /be
Comment 4•21 years ago
|
||
> 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?
Assignee | ||
Comment 5•21 years ago
|
||
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
Assignee | ||
Comment 6•21 years ago
|
||
> 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
Assignee | ||
Updated•21 years ago
|
Attachment #134947 -
Flags: review?(shaver)
Assignee | ||
Comment 7•21 years ago
|
||
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
Assignee | ||
Comment 8•21 years ago
|
||
Attachment #134946 -
Attachment is obsolete: true
Attachment #134947 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #135042 -
Flags: review?(shaver)
Comment 9•21 years ago
|
||
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?
Assignee | ||
Comment 10•21 years ago
|
||
-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•21 years ago
|
||
Sorry, no idea on the jsd_lock assert botches. jsd_lock.c is jband's original code, he may have a clue.
Assignee | ||
Comment 12•21 years ago
|
||
Where's jband? For that matter, where's shaver? Saw him in another bug recently, so I'm hopeful! /be
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
Attachment #135042 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 14•21 years ago
|
||
Fixed. /be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Flags: testcase-
You need to log in
before you can comment on or make changes to this bug.
Description
•