660778 - Stack Overflow Crash [@ js::gc::Cell::arenaHeader() | js::gc::Cell::address()] | ASSERTION: JSEventListener has wrong script context?: 'stack && NS_SUCCEEDED(stack- >Peek(&cx)) && cx && GetScriptContextFromJSContext(cx) == mContext'

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

1. http://www.justin.tv/steven_bonnell_ii/b/286904322
2. Stack Overflow Crash WinXP debug nightly

Associated Socorro Signature UnmarkGrayChildren

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=UnmarkGrayChildren&reason_type=contains&date=05%2F31%2F2011%2003%3A38%3A02&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=UnmarkGrayChildren

which shows mostly windows but one mac.

in vs2005 debugger:

inline uintptr_t
Cell::address() const
{
    uintptr_t addr = uintptr_t(this);
    JS_ASSERT(addr % Cell::CellSize == 0);
=>    JS_ASSERT(Chunk::withinArenasRange(addr));
    return addr;
}

		addr	118147992	unsigned int
		this	0x070acb98	const js::gc::Cell * const

>	mozjs.dll!js::gc::Cell::address()  Line 442 + 0x4 bytes	C++
 	mozjs.dll!js::gc::Cell::arenaHeader()  Line 449 + 0x8 bytes	C++
 	mozjs.dll!js::gc::AssertValidColor(const void * thing=0x070acb98, unsigned int color=1)  Line 508 + 0x8 bytes	C++
 	mozjs.dll!js_GCThingIsMarked(void * thing=0x070acb98, unsigned int color=1)  Line 565 + 0xd bytes	C++
 	xul.dll!xpc_IsGrayGCThing(void * thing=0x070acb98)  Line 155 + 0xc bytes	C++
 	xul.dll!UnmarkGrayChildren(JSTracer * trc=0x0012b564, void * thing=0x070acb98, unsigned int kind=0)  Line 583 + 0x15 bytes	C++
 	mozjs.dll!js::gc::Mark<JSObject>(JSTracer * trc=0x0012b564, JSObject * thing=0x070acb98)  Line 124 + 0x1d bytes	C++
 	mozjs.dll!js::gc::MarkKind(JSTracer * trc=0x0012b564, void * thing=0x070acb98, unsigned int kind=0)  Line 317 + 0xd bytes	C++
 	mozjs.dll!js::gc::MarkValueRaw(JSTracer * trc=0x0012b564, const js::Value & v={...})  Line 341 + 0x1b bytes	C++
 	mozjs.dll!js::gc::MarkValueRange(JSTracer * trc=0x0012b564, js::Value * beg=0x050a26f8, js::Value * end=0x050a2700, const char * name=0x00a843ec)  Line 357 + 0xd bytes	C++
 	mozjs.dll!js::gc::MarkValueRange(JSTracer * trc=0x0012b564, unsigned int len=1, js::Value * vec=0x050a26f8, const char * name=0x00a843ec)  Line 364 + 0x1b bytes	C++
 	mozjs.dll!fun_trace(JSTracer * trc=0x0012b564, JSObject * obj=0x070acc40)  Line 1974 + 0x2d bytes	C++
 	mozjs.dll!js::gc::MarkChildren(JSTracer * trc=0x0012b564, JSObject * obj=0x070acc40)  Line 665 + 0x10 bytes	C++
 	mozjs.dll!JS_TraceChildren(JSTracer * trc=0x0012b564, void * thing=0x070acc40, unsigned int kind=0)  Line 753 + 0xd bytes	C++
 	xul.dll!UnmarkGrayChildren(JSTracer * trc=0x0012b564, void * thing=0x070acc40, unsigned int kind=0)  Line 590 + 0x12 bytes	C++
...


Note that automation shows this same signature in aurora WinXP/Win7 for other justin.tv urls though not this one. 

For aurora, try http://www.justin.tv/the_jesus_chatline/b/286335617 and maybe reload to trigger the crash


Note we also get

###!!! ASSERTION: JSEventListener has wrong script context?: 'stack && NS_SUCCEEDED(stack-
>Peek(&cx)) && cx && GetScriptContextFromJSContext(cx) == mContext', file c:/work/mozilla/
builds/nightly/mozilla/dom/src/events/nsJSEventListener.cpp, line 220

###!!! ASSERTION: JSEventListener has wrong script context?: 'stack && NS_SUCCEEDED(stack-
>Peek(&cx)) && cx && GetScriptContextFromJSContext(cx) == mContext', file c:/work/mozilla/
builds/aurora/mozilla/dom/src/events/nsJSEventListener.cpp, line 220

See also bug 650519

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: crash report xp

Comment 2

[Gregor Wagner [:gwagner]]

Seems like custom JSTracers have problems not with over-recursion.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

I haven't been able to reproduce the problem on this site, but I think this patch should fix the problem. It just checks for a stack overflow in UnmarkGray, and if it runs out of stack, it requires a GC to run before the next CC.

Comment 4

[Andreas Gal :gal]

Comment on attachment 536674 [details] [diff] [review]
patch

This is a great fix! I didn't think of just forcing a GC instead of dealing with a delayed marking stack.

Comment 5

[Gregor Wagner [:gwagner]]

 static void
 UnmarkGrayChildren(JSTracer *trc, void *thing, uint32 kind)
 {
+    int stackDummy;
+    if (!JS_CHECK_STACK_SIZE(trc->context->stackLimit, &stackDummy)) {

Maybe a stupid question but are we sure that stackLimit corresponds to the current thread?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to comment #5)
> Maybe a stupid question but are we sure that stackLimit corresponds to the
> current thread?

Good question. The context that we're using is somehow derived from some kind of per-thread data in XPConnect, so I think it should be for the current thread. Whether it's actually been set properly is another matter. I see a bunch of callers to JS_SetNativeStackQuota, so I think we're probably safe.

Unfortunately, the patch is leaking on tryserver, so I'll need to sort this out.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/tracemonkey/rev/85083c2d1f32

Comment 9

[Andrew McCreight [:mccr8]]

I believe that before my patch for Bug 663532 landed that the cycle collector never actually invoked the garbage collector, so we should make sure that that patch (and its followup that fixes a leak that my patch introduced...) ends up in the same release as this one.

Comment 10

[Bob Clary [:bc] (inactive)]

This is fixed on mozilla-central right? Merged on June 6 by cdleary ?
http://hg.mozilla.org/mozilla-central/rev/85083c2d1f32

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> This is fixed on mozilla-central right? Merged on June 6 by cdleary ?
> http://hg.mozilla.org/mozilla-central/rev/85083c2d1f32

It looks to me like it landed and this should probably be marked fixed.

Given comment 9, is it too much for this to be pushed forward onto the Gecko 6 branch? or could it be possible? I'm asking this as it appears to be a top crasher for Thunderbird at the moment (#2).

Comment 12

[Andrew McCreight [:mccr8]]

As I say above, note that you probably need Bug 663532 and Bug 664506, too, as the cycle collector wasn't working quite right, in a way that this bug depends on, as far as I can tell.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/mozilla-central/rev/85083c2d1f32

Comment 15

[Scoobidiver (away)]

It is #9 top crasher in 6.0.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Should we try to get this into 6?

Comment 17

[Sheila Mooney]

We don't need to track this but request approval for 6 and it will get triaged.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 536674 [details] [diff] [review]
patch

Please land on both Aurora and Beta and please, in the future, please include a risk assessment with approval requests. Thanks.

Comment 19

[Andrew McCreight [:mccr8]]

Comment on attachment 536674 [details] [diff] [review]
patch

This patch is already in Aurora, so I'm removing the approval flag.

Comment 20

[Andrew McCreight [:mccr8]]

http://hg.mozilla.org/releases/mozilla-beta/rev/61bff9064c10

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> http://hg.mozilla.org/releases/mozilla-beta/rev/61bff9064c10

You're supposed to update the status flags when landing stuff on branches.

Comment 22

[Andrew McCreight [:mccr8]]

I thought I was supposed to wait until the landing actually stuck, but thanks, I'll set them immediately in the future.

Comment 23

[AndreiD[QA]]

Verified on:
Mac: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:6.0) Gecko/20100101 Firefox/6.0
Windows 7: Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0
Windows XP: Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0
Linux: Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

There are no errors/crashes while playing the videos / link provided in the description

Comment 24

[Scoobidiver (away)]

(In reply to comment #23)
> Verified on:
> Mac: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:6.0) Gecko/20100101
> Firefox/6.0
> Windows 7: Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0
> Windows XP: Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0
> Linux: Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0
Which browser.startup.homepage_override.buildID? Firefox 6.0b1 or b2?

Comment 25

[AndreiD[QA]]

> Which browser.startup.homepage_override.buildID? Firefox 6.0b1 or b2?
Build 2

Comment 26

[(no longer active)]

It was not clear whether this landed before the last Aurora merge or not, so now we don't exactly know whether this exists on Beta or not.  Somebody from the js team should confirm.

Comment 27

[Andrew McCreight [:mccr8]]

I think it is did.  It is present in Aurora as of this morning.  I don't know if that is before or after the merge. http://hg.mozilla.org/releases/mozilla-aurora/file/37edafab8b33/xpcom/base/nsCycleCollector.cpp

If xpcom/base/nsCycleCollector.cpp has a call to NeedCollect(), then this patch has been applied.

Comment 28

[Andrew McCreight [:mccr8]]

"I think it is did. "   This should be "I think it is."

Comment 29

[LegNeato]

mNeedGCBeforeCC is on mozilla-beta, marking this as fixed