241050 - M17rc1 and Trunk topcrash [@ nsJSContext::DOMBranchCallback]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See http://talkback-public.mozilla.org/reports/Trunk/smart-analysis.all and
http://talkback-public.mozilla.org/reports/M17b/smart-analysis.all

Stack:

nsJSContext::DOMBranchCallback
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp
 line 413] 
js_Interpret
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c 
line 2033] 
js_Invoke
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c 
line 1302] 
...etc

The line number for branch is 417; for trunk, 413.  lxr has:

409   nsJSContext *ctx = NS_STATIC_CAST(nsJSContext *, ::JS_GetContextPrivate(cx));
410 
411   // Filter out most of the calls to this callback
412   if (++ctx->mBranchCallbackCount & MAYBE_GC_BRANCH_COUNT_MASK)
413     return JS_TRUE;
414 
415   // Run the GC if we get this far.
416   JS_MaybeGC(cx);
417 
418   // Filter out even more of the calls to this callback

Could "ctx" be coming out null or bogus?

This is the #2 topcrash in 1.7b.

Comment 1

[Bernd]

Boris: you should read first bug 238446 and look for the dependencies :-)

Comment 2

[Bernd]

you will find there bug 238218

Comment 3

[Brendan Eich [:brendan]]

What bernd said.

/be

*** This bug has been marked as a duplicate of 238218 ***

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note the "and trunk" part of the summary and the first URL I cite in comment 0.
 If talkback is lying, fine; otherwise this crash is still happening (in fact
happened with builds from 2004-04-13-10 and 2004-04-10-09, which is way after
bug 238218 got checked in and marked fixed).

Today may be my day for filing dups, but we may still have a real bug here...

Comment 5

[Brendan Eich [:brendan]]

Sorry, didn't read the trunk part.

 nsJSContext::DOMBranchCallback
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp
 line 413] 
	 js_Interpret
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c 
line 2033] 
	 js_Invoke
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c 
line 1302] 
	 js_InternalInvoke
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c 
line 1379] 
	 js_InternalGetOrSet
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c 
line 1422] 
	 js_GetProperty
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c  line
2720] 
	 JS_GetProperty
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c  line
2473] 
	 nsXPCWrappedJSClass::CallMethod
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp
 line 1316] 
	 nsXPCWrappedJS::CallMethod
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp
 line 450] 
	 PrepareAndDispatch
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp
 line 119] 
	 SharedStub
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp
 line 147] 
	 MessageWindow::WindowProc
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp
 line 975] 
	 USER32.dll + 0x3a50 (0x77d43a50)  
	 USER32.dll + 0x3b1f (0x77d43b1f)  
	 USER32.dll + 0x44f5 (0x77d444f5)  
	 USER32.dll + 0x4525 (0x77d44525)  
	 ntdll.dll + 0x25da3 (0x77f75da3)  
	 USER32.dll + 0x58ce (0x77d458ce)  
	 nsWindow::DefaultWindowProc
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp
 line 1375] 
	 USER32.dll + 0x3a50 (0x77d43a50)  
	 USER32.dll + 0x3b1f (0x77d43b1f)  
	 USER32.dll + 0x5b2c (0x77d45b2c)  
	 USER32.dll + 0x5b4b (0x77d45b4b)  
	 nsWindow::WindowProc
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp
 line 1361] 
	 USER32.dll + 0x3a50 (0x77d43a50)  
	 USER32.dll + 0x3b1f (0x77d43b1f)  
	 USER32.dll + 0x44f5 (0x77d444f5)  
	 USER32.dll + 0x4525 (0x77d44525)  
	 ntdll.dll + 0x25da3 (0x77f75da3)  
	 USER32.dll + 0x3fd4 (0x77d43fd4)  
	 PeekKeyAndIMEMessage
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsAppShell.cpp
 line 91] 
	 nsAppShell::Run
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsAppShell.cpp
 line 138] 
	 nsAppShellService::Run
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/appshell/src/nsAppShellService.cpp
 line 524] 
	 main1
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
 line 1313] 
	 main
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
 line 1783] 
	 WinMain
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
 line 1809] 
	 WinMainCRTStartup()  
	 kernel32.dll + 0x214c7 (0x77e814c7)   

Looks like we're running on the safe context.  Why would its JSContext have null
private data?

/be

Comment 6

[Jay Patel [:jay]]

Bug 238218 is fixed, but I have been seeing similar crashes in the latest
Talkback data for the Trunk and Mozilla 1.7rc1.  So we should probably have a
look at these more recent crashes.

Here is a recent crash in the Mozilla 1.7 branch (from 1.7rc1):
Incident ID: 28665
Stack Signature	nsJSContext::DOMBranchCallback e1b894d7
Email Address	
Product ID	Mozilla17
Build ID	2004042109
Trigger Time	2004-04-23 11:21:11.0
Platform	Win32
Operating System	Windows NT 5.0 build 2195
Module	gklayout.dll + (0014ca14)
URL visited	
User Comments	
Since Last Crash	sec
Total Uptime	sec
Trigger Reason	Access violation
Source File Name
d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp
Trigger Line No.	417
Stack Trace 	
nsJSContext::DOMBranchCallback
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp,
line 417]
js_Interpret
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1671]
js_Invoke
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 959]
js_InternalInvoke
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1036]
js_InternalGetOrSet
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1079]
js_GetProperty
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 2679]
JS_GetProperty
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c, line 2490]
nsXPCWrappedJSClass::CallMethod
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,
line 1316]
nsXPCWrappedJS::CallMethod
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp,
line 450]
PrepareAndDispatch
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp,
line 119]
SharedStub
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp,
line 147]
MessageWindow::WindowProc
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp,
line 975]
USER32.dll + 0x1ef0 (0x77e01ef0)
USER32.dll + 0x3869 (0x77e03869)
USER32.dll + 0x38ab (0x77e038ab)
ntdll.dll + 0x1ff57 (0x7847ff57)
USER32.dll + 0x343f (0x77e0343f)
USER32.dll + 0x1ef0 (0x77e01ef0)
USER32.dll + 0x3d1e (0x77e03d1e)
USER32.dll + 0x6e9b (0x77e06e9b)
nsWindow::WindowProc
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1361]
USER32.dll + 0x1ef0 (0x77e01ef0)
USER32.dll + 0x3869 (0x77e03869)
USER32.dll + 0x38ab (0x77e038ab)
ntdll.dll + 0x1ff57 (0x7847ff57)
USER32.dll + 0x18ec (0x77e018ec)
PeekKeyAndIMEMessage
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsAppShell.cpp,
line 91]
nsAppShell::Run
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsAppShell.cpp,
line 138]
nsAppShellService::Run
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpfe/appshell/src/nsAppShellService.cpp,
line 524]
main1
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1313]
main
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1783]
WinMain
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1809]
WinMainCRTStartup()
KERNEL32.DLL + 0x11af6 (0x77e81af6)

Comment 7

[Brendan Eich [:brendan]]

So the callstack in comment 6 goes through the JS component that implements
-killAll:
http://lxr.mozilla.org/mozilla/source/xpfe/components/killAll/nsKillAll.js. 
This component has a chromeUrlForTask getter that implements the command-line
option by looping over DOM windows using the window mediator service, closing
each in turn.  That must be closing the hidden window whose JSContext we're
running on, because it is the "safe" JSContext that XPConnect uses when in
doubt, when C++ is calling into JS:
http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#976,
see also
http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpccallcontext.cpp#92
and the comment above it.

This says that GlobalWindowImpl::Close (domWindow.close() from nsKillAll.js
calls this) is not setting a termination function and returning early at
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsGlobalWindow.cpp#3641. 
Good thing, because XPConnect won't call the termination function
(http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#106,
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#1710,
note the PR_FALSE aTerminated parameter).  But how can that be?  Is the hidden
window's JSContext not hooked up to its nsJSContext?

jst, any ideas?  Certainly, if this is peculiar to nsKillAll.js, as it seems to
be from the talkback, then there's probably a specific fix.  Didn't we find last
time that nsJSContext's dtor needed to JS_SetBranchCallback(cx, nsnull) exactly
to handle a case we didn't quite grok, where XPConnect is using the JSContext
and the stack must unwind from the point where that cx's private data (its
nsJSContext) is being dtor'd?

That once again raises the possibility in DOMBranchCallback that the cx may have
null private data.  Before we throw in the towel and restore the null check that
we removed from DOMBranchCallback, I'd like to know why the termination function
is not used.  That, provided XPConnect were fixed to actually call it
(indirectly via ScriptExecuted => ScriptEvaluated(PR_TRUE)), would fix the bug
without adding a null check to DOMBranchCallback.

/be

Comment 8

[Brendan Eich [:brendan]]

> Good thing, because XPConnect won't call the termination function
>
(http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#106,
> http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#1710,
> note the PR_FALSE aTerminated parameter).

Second link should be
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#1819 --
lxr is pretty busted.  See
http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/nsXPConnect.cpp#1071.

Oh, and I recall now that XPConnect does its own kind of termination function,
based on XPCCallContext::mDestroyJSContextInDestructor.  We have redundant
mechanism in the DOM and XPConnect for modularity's sake, but it still seems to
me we could simplify this twisted maze a bit.

So what is really going on in this bug?  If the DOM called
JS_SetBranchCallback(cx, nsnull), and the JS engine reloaded cx->branchCallback,
then there'd be no problem.  Since the DOM calls JS_SetBranchCallback(cx,
nsnull) right after JS_SetContextPrivate(cx, nsnull), it must be the case that
js_Interpret has not reloaded cx->branchCallback, and is using its onbranch
local to call the old callback.

This suggests a fix: change js_Interpret to reload onbranch after any native
call-out (getter/setter or native function).  I'll attach a patch in a minute.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: proposed fix

Correction: reload onbranch from cx->branchCallback only after native (possibly
native, actually) function calls.  Restoring after getters, setters, and other
low-level hooks is hard, and not worth it.  The API users must clear the branch
callback, if it matters (as it does for the DOM, due to this deferred destroy
of the context on which one is running), only from a native function such as
domWindow.close().

/be

Comment 10

[Brendan Eich [:brendan]]

Please help test this patch by running -killAll or whatever it is, with and
without QuickStart if you're on Windows.

/be

Comment 11

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

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

r=shaver.  Go for 1.7!

Comment 12

[Brendan Eich [:brendan]]

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

Safe fix for topcrash bug, easy a=.

/be

Comment 13

[chris hofmann]

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

a=chofmann for 1.7

Comment 14

[Brendan Eich [:brendan]]

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

Marking chofmann's approval1.7+.

This patch already landed on the trunk, and I have just landed it on the 1.7
branch.  Thanks, great to have talkback stack analysis with line numbers!

/be

Comment 15

[alanjstr]

Marking fixed based on comment 14