424636 - Assertion failure: !JS_IS_RUNTIME_LOCKED(rt), at js_GC (js/src/jsgc.c:2997)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5/Array/regress-350256-03.js;language=type;text/javascript>

Linux (32bit), Mac ppc|Intel trunk builds assert Assertion failure: !JS_IS_RUNTIME_LOCKED(rt) at js_GC (js/src/jsgc.c:2997) in debug browser builds. opt browser builds appear to hang. Linux (64bit) does not show the problem.

Probably regressed between 2008-03-19 10:00PM and 2008-03-20 00:00AM. I'm guessing bug 420869.

Comment 1

[Bob Clary [:bc] (inactive)]

Schrep asked that I comment on why I think this should block b5.

I don't know for sure if this should block beta 5 however my gut tells me that:

We shouldn't take regressions at this stage.

I think this may be responsible for some other issues such as time outs on other tests although I haven't had the time to run tests with bug 420869 backed out to make absolutely sure.

I was hoping Igor could assign a severity to this so we could have a better idea if it should block or not.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> I think this may be responsible for some other issues such as time outs on
> other tests although I haven't had the time to run tests with bug 420869 backed
> out to make absolutely sure.

In theory this sounds like a bad bug, but to judge it at least a stack trace is required. I will check it later today.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Igor: P1 = beta blocker, P2 = final release blocker, let us know what you think.

Comment 4

[Igor Bukanov]

This is a regression from bug 365851. The stack trace leading to the bug looks like:

#3  0x002006f0 in JS_Assert (s=0x217a86 "!JS_IS_RUNTIME_LOCKED(rt)", file=0x216f71 "/home/igor/m/ff/mozilla/js/src/jsgc.c", ln=2997) at /home/igor/m/ff/mozilla/js/src/jsutil.c:63
#4  0x001ae38c in js_GC (cx=0xb7b7ec0, gckind=GC_NORMAL) at /home/igor/m/ff/mozilla/js/src/jsgc.c:2997
#5  0x001c4b4a in js_GenerateShape (cx=0xb7b7ec0) at /home/igor/m/ff/mozilla/js/src/jsinterp.c:93
#6  0x001f6e09 in GetPropertyTreeChild (cx=0xb7b7ec0, parent=0x40fbed90, child=0xbfc94030) at /home/igor/m/ff/mozilla/js/src/jsscope.c:896
#7  0x001f7484 in js_AddScopeProperty (cx=0xb7b7ec0, scope=0xc25c500, id=33472345, getter=0x1a8ce4 <args_getProperty>, setter=0x1a8c23 <args_setProperty>, slot=16736181, attrs=0, flags=<value optimized out>, shortid=0) at /home/igor/m/ff/mozilla/js/src/jsscope.c:1260
#8  0x001d08f1 in js_DefineNativeProperty (cx=0xb7b7ec0, obj=0xbdc1480, id=33472345, value=-2147483647, getter=0x1a8ce4 <args_getProperty>, setter=0x1a8c23 <args_setProperty>, attrs=<value optimized out>, flags=0, shortid=0, propp=0x0) at /home/igor/m/ff/mozilla/js/src/jsobj.c:3114

Here js_GenerateShape calls JS_GC with a locked scope leading to this bug. the changes from the bug 420869 just exposed this as the test now runs sufficiently long to trigger the bug. The worst result of the bug is a deadlock. Note also that on 64bit platform the bug is invisible is takes 2**32 times longer to trigger the bug.

As such I think this can wait RC1.

Comment 5

[Damon Sicore (:damons)]

Any progress on this?

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I don't think we should block on this: by my understanding it's unlikely to cause real problems for release users, and seems like it's amenable to a dot-release fix.  (We should continue to prosecute such a fix once the other blockers are done, to be sure.)

Minusing, please renom if you believe that I have understated the severity of the issue, or the invasiveness of the fix.

Comment 7

[Brendan Eich [:brendan]]

I'm about to patch. This is a hard hang after a bounded number of operations that correlates to JS usage, not to uptime. It will bite users in the field.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 9

[Igor Bukanov]

Comment on attachment 318235 [details] [diff] [review]
proposed fix

>Index: jsinterp.c
> uint32
>-js_GenerateShape(JSContext *cx)
>+js_GenerateShape(JSContext *cx, JSBool gcLocked)
> {
...
>-        js_GC(cx, GC_NORMAL);
>+        js_GC(cx, gcLocked ? GC_LOCK_HELD : GC_NORMAL);

An alternative to JSBool gclocked is to use JSGCInvocationKind gckind and pass GC_LOCK_HELD or GC_NORMAL directly to js_GenerateShape together with an assert that only these two gc kinds are accepted. But this a matter of taste.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> (From update of attachment 318235 [details] [diff] [review])
> >Index: jsinterp.c
> > uint32
> >-js_GenerateShape(JSContext *cx)
> >+js_GenerateShape(JSContext *cx, JSBool gcLocked)
> > {
> ...
> >-        js_GC(cx, GC_NORMAL);
> >+        js_GC(cx, gcLocked ? GC_LOCK_HELD : GC_NORMAL);
> 
> An alternative to JSBool gclocked is to use JSGCInvocationKind gckind and pass
> GC_LOCK_HELD or GC_NORMAL directly to js_GenerateShape together with an assert
> that only these two gc kinds are accepted.

I considered that but did not like the unwanted degree of freedom that requires an assertion to "type check", and did not want to drag in the enum from jsgc.h to all #includes of jsinterp.h. Taste-wise it was a closer call, I agree, but these "operational" considerations made me go with a boolean. Thanks,

/be

Comment 11

[Damon Sicore (:damons)]

Noting brendan's reply (comment 7) to shaver, +'ing.  

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 318235 [details] [diff] [review]
proposed fix

Tests well, safe fix with small number of callers to js_GenerateShape (not exported, so we can analyze all call sites).

/be

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 318235 [details] [diff] [review]
proposed fix

a1.9=beltzner

Comment 14

[Bob Clary [:bc] (inactive)]

attachment 318235 [details] [diff] [review] appears to fix the assertion for me on 32bit linux and mac os x. I'll kick off a full run of tests with this patch shortly.

Comment 15

[Brendan Eich [:brendan]]

Fixed:

js/src/jsinterp.c 3.494
js/src/jsinterp.h 3.92
js/src/jsscope.c 3.87
js/src/jsscope.h 3.59
js/src/jsstr.h 3.61

/be

Comment 16

[Bob Clary [:bc] (inactive)]

v 1.9.0