Closed Bug 425828 Opened 17 years ago Closed 17 years ago

Property cache must be cleared when JSThread goes from 0 to 1 context.

Categories

(Core :: JavaScript Engine, defect, P1)

x86
Windows 2000
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: MikeM, Assigned: igor)

References

Details

(Keywords: assertion, regression)

Attachments

(1 file, 1 obsolete file)

I'm getting a crash with the lastest trunk. I beleive it might have something to do with the work Igor has been doing with functions. Here is the call stack for the crash: -------------------- >js_Interpret(JSContext * cx=0x0335f168) Line 4621 + 0x1b bytes C js_Execute(JSContext * cx=0x0335f168, JSObject * chain=0x03b11000, JSScript * script=0x02fab0d8, JSStackFrame * down=0x00000000, unsigned int flags=2048, long * result=0x03c2f11c) Line 1527 C JS_ExecuteScript(JSContext * cx=0x0335f168, JSObject * obj=0x03b11000, JSScript * script=0x02fab0d8, long * rval=0x03c2f11c) Line 4876 + 0x1f bytes C -------------------- The code is below: BEGIN_CASE(JSOP_CALL) BEGIN_CASE(JSOP_EVAL) argc = GET_ARGC(regs.pc); vp = regs.sp - (argc + 2); lval = *vp; if (VALUE_IS_FUNCTION(cx, lval)) { <- DIES ON THIS LINE. JSNativeFunction *nfun; obj = JSVAL_TO_OBJECT(lval); fun = OBJ_TO_FUNCTION(obj); --------------------
Priority: -- → P1
The script I'm executing is below: It crashes on line 5: Which is: response.setHeader("Cache-Control", "no-cache, must-revalidate"); ----------------------------------- response.write(" "); var session = request.getSession(false, false); if(!session) { response.write("{t:-999}"); response.end(); } response.setHeader("Cache-Control", "no-cache, must-revalidate"); response.setHeader("Pragma", "no-cache"); var WORK_QUEUE_APP_ID = 1; var WORK_QUEUE_TYPE=2; if(session.global.DEBUG_WORK_QUEUE && session.global.DEBUG_WORK_QUEUE[WORK_QUEUE_APP_ID] && session.global.DEBUG_WORK_QUEUE[WORK_QUEUE_APP_ID][WORK_QUEUE_TYPE] && session.global.DEBUG_WORK_QUEUE[WORK_QUEUE_APP_ID][WORK_QUEUE_TYPE].length) { response.clear(); var obj = session.global.DEBUG_WORK_QUEUE[WORK_QUEUE_APP_ID][WORK_QUEUE_TYPE].shift(); if(obj.status!=200) { response.status=obj.status; response.end(); } response.write(obj.json) response.end(); } response.write("\r\n{ \"t\" : 1, \"o\" : null }"); --------------------------
Per request from igor: The expression: regs.sp - fp->spbase as the value 0x00000004
More values from stack: regs.sp[-1] 0x03b134bc long regs.sp[-2] 0x03b134b4 long regs.sp[-(regs.sp - fp->spbase)] 0x04263f50 long
Per request from igor here is the JSFunctionSpec from the "response" object. static JSFunctionSpec response_methods[] = { {"write", MWResponseMethod, 1, 0, 0}, //Takes 1 argument.. {"flush", MWResponseMethod, 0, 0, 0}, {"clear", MWResponseMethod, 0, 0, 0}, {"writeBinary", MWResponseMethod, 3, 0, 0}, //Takes bytes, start position and # of bytes. {"sendRedirect", MWResponseMethod, 1, 0, 0}, //Takes URL for redirection. {"setHeader", MWResponseMethod, 2, 0, 0}, //Takes name value pair (two strings). {"getBufferUsedSize", MWResponseMethod, 0, 0, 0}, {"containsHeader", MWResponseMethod, 1, 0, 0}, {"end", MWResponseMethod, 0, 0, 0}, //Used to stop script processing. {"addCookie", MWResponseMethod, 1, 0, 0}, //Add a cookie to the response. {"encodeRedirectURL", MWResponseMethod, 1, 0, 0}, //Add session token to a redirect. {"encodeURL", MWResponseMethod, 1, 0, 0}, //Add session token to a link. {"setContentType", MWResponseMethod, 1, 0, 0}, {0,0,0,0,0}, //Terminates the table of functions. REQUIRED. };
After messing around trying to reproduce the crash under different scenarios I now got a crash in a slightly different place. However this one is much more revealing... The crash is still in jsinterp.c. Here's the offending line: --------------------- /* Function is inlined, all other classes use object ops. */ ops = funobj->map->ops; --------------------- The map member of funobj is NULL which explains the crash. Here is the value of funobj as well as a few other local variables. -funobj 0x042f0f50 {map=0x00000000 fslots=0x042f0f54 dslots=0x00000000 } JSObject * + map 0x00000000 {nrefs=??? ops=??? freeslot=??? } JSObjectMap * + fslots 0x042f0f54 long [6] + dslots 0x00000000 long * + parent 0x00000000 {map=??? fslots=0x00000004 dslots=??? } JSObject * + clasp 0x00000000 {name=??? flags=??? addProperty=??? ...} JSClass * Call stack to crash: ---------------------- js_Invoke(JSContext * cx=0x031c2220, unsigned int argc=0x00000002, long * vp=0x03670548, unsigned int flags=0x00000000) Line 1046 + 0x5 bytes C js_Interpret(JSContext * cx=0x031c2220) Line 4841 + 0x16 bytes C js_Execute(JSContext * cx=0x031c2220, JSObject * chain=0x03d11000, JSScript * script=0x035d94f0, JSStackFrame * down=0x00000000, unsigned int flags=0x00000800, long * result=0x03d0f114) Line 1526 + 0x9 bytes C JS_ExecuteScript(JSContext * cx=0x031c2220, JSObject * obj=0x03d11000, JSScript * script=0x035d94f0, long * rval=0x03d0f114) Line 4876 + 0x19 bytes C -------------------- Igor, Could any of your function work cause this?
I just pulled latest trunk. Crash still exists bug has morphed a bit. Seems bug# 424376 is not to blame (at least not 100%). Now the crash has moved down 2 more lines and locals are slightly different. ----------------- /* Function is inlined, all other classes use object ops. */ ops = funobj->map->ops; /* * XXX this makes no sense -- why convert to function if clasp->call? * XXX better to call that hook without converting * XXX the only thing that needs fixing is liveconnect * * Try converting to function, for closure and API compatibility. * We attempt the conversion under all circumstances for 1.2, but * only if there is a call op defined otherwise. */ if ((ops == &js_ObjectOps) ? clasp->call : ops->call) { ok = clasp->convert(cx, funobj, JSTYPE_FUNCTION, &v); <- DIES HERE. ----------------- Values that were NULL before latest trunk now contain a value of 0xdadadada which looks like garbage memory pattern/dangling pointer... ----------------------------------- - funobj 0x03d21ae0 {map=0x03d21b00 fslots=0x03d21ae4 dslots=0xdadadada } JSObject * + map 0x03d21b00 {nrefs=0x03d21b20 ops=0x03d21f97 freeslot=0xdadadada } JSObjectMap * - fslots 0x03d21ae4 long [6] [0x0] 0x03d21f98 long [0x1] 0xdadadada long [0x2] 0xdadadada long [0x3] 0xdadadada long [0x4] 0xdadadada long [0x5] 0xdadadada long + dslots 0xdadadada long * + parent 0xdadadad8 {map=??? fslots=0xdadadadc dslots=??? } JSObject * + clasp 0xdadadad8 {name=??? flags=??? addProperty=??? ...} JSClass * -------------------------
Just found out it still crashes in the original location as described in 1st comment as well as location in comment #5. When crashes in location described in comment #5 funobj->map has a value of 0xdadadada too now.
FYI. Ran with JS_SetGCZeal() value of 2 per sugestion of Brendan. No rooting problems to speak of. Amen. However I did hit bug# 419091 before this bug crashed again. Arg!
Short summary of the debugging session: 1. adding early return to js_GC() to disable the GC prevents the bug. 2. The bug is related to the property cache. Due to the nature of the scripts that triggers the bug in JSOP_CALL, JSOP_CALLPROP should always store a function value on the stack. So at the end of JSOP_CALLPROP regs.sp[-2] must be a function. Adding a code to force a debug break when this is not the case has shown that the bug happens after a property cache hit on the following code path: BEGIN_CASE(JSOP_CALLPROP) { ... aobj = OBJ_IS_DENSE_ARRAY(cx, obj) ? OBJ_GET_PROTO(cx, obj) : obj; if (JS_LIKELY(aobj->map->ops->getProperty == js_GetProperty)) { PROPERTY_CACHE_TEST(cx, regs.pc, aobj, obj2, entry, atom); if (!atom) { ASSERT_VALID_PROPERTY_CACHE_HIT(0, aobj, obj2, entry); if (PCVAL_IS_OBJECT(entry->vword)) { rval = PCVAL_OBJECT_TO_JSVAL(entry->vword); Here JSVAL_IS_OBJECT(rval) but (JSObject *) rval shows that the object was GC-ed.
The bug is the repeat of the story from bug 351602 but with property cache. The property cache must be cleared on each transition from 0 to 1 context, not during thread initialization. Otherwise for alive threads without JSContext the GC will not flush the cache leading to storage of GC-ed values in the cache.
Summary: Crash in JSOP_CALL with VALUE_IS_FUNCTION → Property cache must be cleared when JSThread goes from 0 to 1 context.
asking for 1.9 blocking: the bug makes SpiderMonkey unusable in multithreading environment.
Flags: blocking1.9?
Attached patch v1 (obsolete) — Splinter Review
The patch makes sure that caches are cleared on each JSThread transition from 0 to 1 thread.
Assignee: general → igor
Status: NEW → ASSIGNED
Attachment #312809 - Flags: review?(brendan)
Blocks: js-propcache
Comment on attachment 312809 [details] [diff] [review] v1 Thanks, should have seen this one coming. /be
Attachment #312809 - Flags: review?(brendan)
Attachment #312809 - Flags: review+
Attachment #312809 - Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
Comment on attachment 312809 [details] [diff] [review] v1 No need for an approval as the bug has gained the blocking1.9 flag.
Attachment #312809 - Flags: approval1.9?
Attached patch v1bSplinter Review
The new version fixes English grammar in comments.
Attachment #312809 - Attachment is obsolete: true
Attachment #313053 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
After burning it in for awhile...yes the patch seems to be doing the job. Thanks for your help on this Igor! Well done!
Blocks: 481380
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: