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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: MikeM, Assigned: igor)
References
Details
(Keywords: assertion, regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
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);
--------------------
Reporter | ||
Updated•17 years ago
|
Priority: -- → P1
Reporter | ||
Comment 1•17 years ago
|
||
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 }");
--------------------------
Reporter | ||
Comment 2•17 years ago
|
||
Per request from igor:
The expression: regs.sp - fp->spbase
as the value 0x00000004
Reporter | ||
Comment 3•17 years ago
|
||
More values from stack:
regs.sp[-1] 0x03b134bc long
regs.sp[-2] 0x03b134b4 long
regs.sp[-(regs.sp - fp->spbase)] 0x04263f50 long
Reporter | ||
Comment 4•17 years ago
|
||
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.
};
Reporter | ||
Comment 5•17 years ago
|
||
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?
Reporter | ||
Comment 6•17 years ago
|
||
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 *
-------------------------
Reporter | ||
Comment 7•17 years ago
|
||
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.
Reporter | ||
Comment 8•17 years ago
|
||
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!
Assignee | ||
Comment 9•17 years ago
|
||
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.
Assignee | ||
Comment 10•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Summary: Crash in JSOP_CALL with VALUE_IS_FUNCTION → Property cache must be cleared when JSThread goes from 0 to 1 context.
Assignee | ||
Comment 11•17 years ago
|
||
asking for 1.9 blocking: the bug makes SpiderMonkey unusable in multithreading environment.
Flags: blocking1.9?
Assignee | ||
Comment 12•17 years ago
|
||
The patch makes sure that caches are cleared on each JSThread transition from 0 to 1 thread.
Assignee | ||
Updated•17 years ago
|
Blocks: js-propcache
Assignee | ||
Updated•17 years ago
|
Keywords: assertion,
regression
Comment 13•17 years ago
|
||
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+
Assignee | ||
Comment 14•17 years ago
|
||
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?
Assignee | ||
Comment 15•17 years ago
|
||
The new version fixes English grammar in comments.
Attachment #312809 -
Attachment is obsolete: true
Attachment #313053 -
Flags: review+
Assignee | ||
Comment 16•17 years ago
|
||
I checked in the patch from the comment 15 to the trunk:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1207127607&maxdate=1207127642&who=igor%25mir2.org
Mike: can you verify that this indeed fixed the bug?
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
Reporter | ||
Comment 17•17 years ago
|
||
After burning it in for awhile...yes the patch seems to be doing the job.
Thanks for your help on this Igor!
Well done!
You need to log in
before you can comment on or make changes to this bug.
Description
•