Closed Bug 425828 Opened 16 years ago Closed 16 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+
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: 16 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: