Closed Bug 424636 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect, P2, major)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: bc, Assigned: brendan)

References

()

Details

(4 keywords)

Attachments

(1 file)

<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.
Flags: in-testsuite+
Flags: in-litmus-
Flags: blocking1.9?
Assignee: general → igor
Keywords: hang
Target Milestone: --- → mozilla1.9beta5
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.
(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.
Igor: P1 = beta blocker, P2 = final release blocker, let us know what you think.
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.
Assignee: igor → brendan
Blocks: js-propcache
No longer blocks: 420869
Priority: -- → P2
Flags: blocking1.9? → blocking1.9+
Target Milestone: mozilla1.9beta5 → mozilla1.9
Any progress on this?
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.
Flags: blocking1.9+ → blocking1.9-
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
Flags: blocking1.9- → blocking1.9?
Attached patch proposed fixSplinter Review
Attachment #318235 - Flags: review?(igor)
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.
Attachment #318235 - Flags: review?(igor) → review+
(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
Status: NEW → ASSIGNED
Hardware: PC → All
Noting brendan's reply (comment 7) to shaver, +'ing.  
Flags: blocking1.9? → blocking1.9+
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
Attachment #318235 - Flags: approval1.9?
Comment on attachment 318235 [details] [diff] [review]
proposed fix

a1.9=beltzner
Attachment #318235 - Flags: approval1.9? → approval1.9+
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.
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
v 1.9.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.