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

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: MikeM, Assigned: igor)

Tracking

({assertion, regression})

Trunk
x86
Windows 2000
assertion, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v1b
3.89 KB, patch
igor
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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

11 years ago
Priority: -- → P1
(Reporter)

Comment 1

11 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

11 years ago
Per request from igor:

The expression: regs.sp - fp->spbase 
as the value 0x00000004
(Reporter)

Comment 3

11 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

11 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

11 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

11 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

11 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

11 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

11 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

11 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

11 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

11 years ago
asking for 1.9 blocking: the bug makes SpiderMonkey unusable in multithreading environment.
Flags: blocking1.9?
(Assignee)

Comment 12

11 years ago
Created attachment 312809 [details] [diff] [review]
v1

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)
(Assignee)

Updated

11 years ago
Blocks: 365851
(Assignee)

Updated

11 years ago
Keywords: assertion, regression
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

11 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

11 years ago
Created attachment 313053 [details] [diff] [review]
v1b

The new version fixes English grammar in comments.
Attachment #312809 - Attachment is obsolete: true
Attachment #313053 - Flags: review+
(Assignee)

Comment 16

11 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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-
(Reporter)

Comment 17

11 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!
(Assignee)

Updated

10 years ago
Blocks: 481380
You need to log in before you can comment on or make changes to this bug.