355811 - [FIX]Crash in [@ nsJSContext::DOMBranchCallback] when download finishes without navigator window.

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Build ID: 2006-10-05-08 of SeaMonkey trunk on Windows XP, but I've seen this on the trunk for at least a week or more.

Summary: Crash in [@ nsJSContext::DOMBranchCallback] when download finishes without navigator window.

Steps to Reproduce:

1. Start a semi-large download (try BitComet 0.73)
2. Close all Navigator windows, but leave the Download progress window up
3. Wait until it finishes downloading

Actual Results:
Crash, here:

nsJSContext::DOMBranchCallback  [mozilla/dom/src/base/nsJSEnvironment.cpp, line 664]
js_Interpret  [mozilla/js/src/jsinterp.c, line 2386]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1392]
nsXPCWrappedJSClass::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1418]
nsXPCWrappedJS::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp, line 478]
SharedStub  [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 147]
nsDownload::OnStateChange  [mozilla/xpfe/components/download-manager/src/nsDownloadManager.cpp, line 1318]
nsDownloadProxy::OnStateChange  [mozilla/xpfe/components/build/../download-manager/src\nsDownloadProxy.h, line 59]
nsExternalAppHandler::ExecuteDesiredAction  [mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp, line 2168]
nsExternalAppHandler::OnStopRequest  [mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp, line 2116]
nsDocumentOpenInfo::OnStopRequest  [mozilla/uriloader/base/nsURILoader.cpp, line 378]
nsStreamListenerTee::OnStopRequest  [mozilla/netwerk/base/src/nsStreamListenerTee.cpp, line 65]
nsInputStreamPump::OnStateStop  [mozilla/netwerk/base/src/nsInputStreamPump.cpp, line 572]

Comment 1

[Boris Zbarsky [:bzbarsky]]

So we get into DOMBranchCallback with JSContext which has a null private.  This JSContext is on the top of the JSContext stack when XPConnect calls into the JS component in question.  Not sure what's put it there...  More interestingly, I'm not sure how it manages to have a null private but be calling this branch callback.  Checking on what's going on there:

(gdb) frame 1
#1  0xb7dddc7d in js_Interpret (cx=0x82af7a8, pc=0xb38ac09d "\005\205T", 
    result=0xbfffe6ac) at ../../../mozilla/js/src/jsinterp.c:2386
2386                CHECK_BRANCH(-1);
(gdb) p cx->branchCallback
$19 = 0
(gdb) p onbranch
$20 = 0xb7545434 <nsJSContext::DOMBranchCallback(JSContext*, JSScript*)>

Which looks wrong to me.

The branch callback on cx got nulled out when we did:

        /* If native, use caller varobj and scopeChain for eval. */
        frame.varobj = fp->varobj;
        frame.scopeChain = fp->scopeChain;
        ok = native(cx, frame.thisp, argc, frame.argv, &frame.rval);
        JS_RUNTIME_METER(cx->runtime, nativeCalls);

which in this case was a Close() call on a window, which called Quit on app-startup, which destroyed the hidden window and its nsJSContext.  This native() call was under JS_Invoke.

The comments when we set onbranch say:

2140     /*
2141      * Prepare to call a user-supplied branch handler, and abort the script
2142      * if it returns false.  We reload onbranch after calling out to native
2143      * functions (but not to getters, setters, or other native hooks).
2144      */

I don't know that that works for us, generally speaking.  Perhaps the DOM branch callback should null-check the nsJSContext it gets?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Might be nice to find out when this regressed too.  Since mid-August, apparently.

Comment 3

[Frank Wein [:mcsmurf]]

Perhaps it should be noted that it only crashes if the "Keep this window open after the download is complete." checkbox is not checked :) (but after reading the whole bug it was clear to me).
Regressed between 2006-09-11 09:00:00 and 2006-09-12 08:00:00
Bonsai link:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-11+08%3A00%3A00&maxdate=2006-09-12+09%3A00%3A00&cvsroot=%2Fcvsroot

Comment 4

[Boris Zbarsky [:bzbarsky]]

Several JS engine changes in there.  Over to engine, if nothing else to get my questions in comment 1 answered.

Comment 5

[Brendan Eich [:brendan]]

What question from comment 1?

The code that calls (not defines) LOAD_BRANCH_HANDLER in jsinterp.c is easy to inspect and has not changed.

The questions to answer are when and how Close was called and destroyed the hidden window's context.  If this call to Close was from the same js_Interpret in frame 1, then why didn't LOAD_BRANCH_HANDLER reload null into onbranch?

Is the JSContext on which all those top frames are running destroyed?  It would seem so, since the only JS_SetBranchCallback that passes null as the cb param in the entire tree that lxr finds is in ~nsJSContext.

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

> What question from comment 1?

Whether onbranch should be reloaded after calling out to |native|, and if so why it wasn't.

> The questions to answer are when and how Close was called

The |native| in question is window.close.

> If this call to Close was from the same js_Interpret
> in frame 1, then why didn't LOAD_BRANCH_HANDLER reload null into onbranch?

Yes, that's what I'm asking.

> Is the JSContext on which all those top frames are running destroyed?

I'm not sure how I tell this offhand, but it might well not be -- see the code in nsJSContext::~nsJSContext that defers the JS_DestroyContext call via XPConnect.

But yes, the ::~nsJSContext for the hidden window is where we null out the callback.

For what it's worth, this really is trivial to reproduce in a debug seamonkey build.  Breakpointing in JS_SetBranchCallback lets you catch when it gets set to null, and then you can examine the stack, step back up to js_Interpret, and so forth.

Comment 7

[Brendan Eich [:brendan]]

What is the script active in js_Interpret?  Print *script to see filename and starting line number.

I don't have any SeaMonkey build, so if you could pull that info out, it would be a big help.  Even better to unwind from close, watch onbranch be reloaded as null, and see why it might be non-null later, verifying that the same script is calling the DOM branch callback that dies.

Since nothing changed in js_Interpret in the regression window, I'm betting that this is not a JS engine bug.  If the regression window is accurate, I'd focus on DOM, embedding, and xpfe changes.

/be

Comment 8

[Brendan Eich [:brendan]]

I don't think it's fair to reassign the bug to get a question answered, especially if the code is plain enough (LOAD_BRANCH_CALLBACK is as plain as it gets).  If this isn't obviously a DOM bug either, then leave it in a general component.

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

So what happens here is that C++ calls into http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/embedding/components/ui/progressDlg/nsProgressDialog.js&rev=1.45#187 which sets this.completed, and the setter (defined at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/embedding/components/ui/progressDlg/nsProgressDialog.js&rev=1.45#691)  closes the window.  Since the branch callback is not reloaded after setter invocations, bad things happen.

Now the question is why bad things didn't happen before.  I'll try to do some pulls by date to see whether I can localize the regressing patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So it looks like the behavior here changed with bug 347523.  Presumably we are now no longer entraining something we used to entrain via XBL or some such, so more of shutdown happens while the JS is executing...

Talked to brendan, and we think a null-check in the DOM branch callback is the way to go.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Null-check

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 241786 [details] [diff] [review]
Null-check

r+sr=jst

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2006-10-15-06 of SeaMonkey trunk under Windows XP; no crash when finished downloading, as per testcase in comment 0.