Stephen Donner [:stephend] Not actively reading bugmail Reporter
	Description • 19 years ago

*Might* be fallout from bug 297561, but I'm not sure.

Build ID: 2005-08-14-05, Windows XP SeaMonkey trunk.

Summary: "Opening [filename.ext]" dialog won't close (GDI leak?)

Steps to Reproduce:

1. In a current SeaMonkey build, load
http://www.yolinux.com/TUTORIALS/LinuxTutorialMimeTypesAndApplications.html
2. Click on http://www.yolinux.com/MIME-EXAMPLES/launch.mpg (or any file linked)
3. After opening/saving the file, try to close the window

Expected Results:

Should be able to close the window.

Actual Results:

Window is unable to be closed, and appears that GDI resources leak (screen
paints through).

See screenshot.

	Stephen Donner [:stephend] Not actively reading bugmail Reporter
	Comment 2 • 19 years ago

Builds up to and including 2005-08-13-05 do NOT have this bug, builds later
(i.e. 2005-08-14-05) do.

	Hermann Schwab
	Comment 3 • 19 years ago

can't reproduce, BuildID 2005081405 from zip package
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050814 SeaMonkey/1.0a

1. http://www.yolinux.com/TUTORIALS/LinuxTutorialMimeTypesAndApplications.html
Didn't find the mpg link on that page

2. http://www.yolinux.com/MIME-EXAMPLES/launch.mpg
left-clicked that URL here, and download started automatically, afterwards
Windows MediaPlayer came up.

saved other files by right-click, file box closed automatically or on abort.

	Hermann Schwab
	Comment 4 • 19 years ago

wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050814 SeaMonkey/1.0a

found that link by scrolling just a tad to the right, didn't see the last column
of the table in my first comment. Left click, right click, all is working fine.

	Stephen Donner [:stephend] Not actively reading bugmail Reporter
	Comment 5 • 19 years ago

Okay, sorry, better steps:

1. Click http://www.yolinux.com/MIME-EXAMPLES/launch.mpg, but DO NOT click OK
(i.e. just let the dialog sit there in the "choose" state)
2. Now click on http://www.yolinux.com/MIME-EXAMPLES/video.avi, but BEFORE you
click OK to "Open it with the current application (Winamp)", position it in the
bottom right of the other dialog. (Make sure both dialogs are visible on screen,
but the video.avi (which you will perform step #3 on) is on TOP of the
launch.mpg dialog.)
3. Now, click OK on the first dialog (launch.mpg), and watch the fun begin.

	Martijn Wargers (dead)
	Comment 6 • 19 years ago

Also wfm with 20050814 trunk SeaMonkey build. I've tried to reproduce with the
steps in comment 5.

	Ere Maijala (slow)
	Comment 7 • 19 years ago

I'm able to reproduce. Investigating..

	Ere Maijala (slow)
	Comment 8 • 19 years ago

I was able to reproduce this a couple of times. Couldn't find anything
supporting the theory that bug 297561 has something to do with this. For some
reason window.close() just destroys the XUL stuff but doesn't close the window.
Now I'm not able to reproduce again. Anyway, this is a bit out of my scope
unless someone can say something definite about how bug 297561 is related.

	Ere Maijala (slow)
	Comment 9 • 19 years ago

For what it's worth, it seems after trying to reproduce this I'm often getting
a crash upon shutdown. It could be related as download manager is involved.

	OstGote!
	Comment 10 • 19 years ago

(In reply to comment #9)
> For what it's worth, it seems after trying to reproduce this I'm often getting
> a crash upon shutdown. It could be related as download manager is involved.

Same for me, I crash only at shutdown after reproduce comment 5. See Talkback
TB8406759M.

	Ere Maijala (slow)
	Comment 11 • 19 years ago

I can't reproduce this one anymore nor do I crash upon shutdown.

	Ere Maijala (slow)
	Comment 12 • 19 years ago

Oops, I do crash, but can't reproduce the dialog bug.

	Ere Maijala (slow)
	Comment 13 • 19 years ago

The crash happens because DownloadManager dies before Download, but Download
calls DownloadManager in the destructor. Anyone interested in fixing this?

	Stephen Donner [:stephend] Not actively reading bugmail Reporter
	Comment 14 • 19 years ago

(In reply to comment #12)
> Oops, I do crash, but can't reproduce the dialog bug.

Yeah, I can't reproduce any longer with build 2005-08-22-05 on Windows XP
SeaMonkey trunk either.

As for the crash, I'm totally fine with morphing this bug to cover it, instead.

	Andrew Schultz Assignee
	Comment 15 • 19 years ago

worksforme with linux seamonkey trunk 2005082703
so this is a download manager bug?

	Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
	Comment 16 • 19 years ago

(In reply to comment #13)
> The crash happens because DownloadManager dies before Download, but Download
> calls DownloadManager in the destructor. Anyone interested in fixing this?

Is it as simple as checking the validity of mDownloadManager before calling its
assert?  (And maybe having ~nsDownloadManager clear mDownloadManager in all the
nsDownloads?)

http://lxr.mozilla.org/seamonkey/source/xpfe/components/download-manager/src/nsDownloadManager.cpp#913

	Samuel Sidler (old account; do not CC)
	Comment 17 • 19 years ago

Tested on a recent Mac branch nightly and couldn't reproduce.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050907
SeaMonkey/1.0a

	Ere Maijala (slow)
	Comment 18 • 19 years ago

(In reply to comment #16)
> Is it as simple as checking the validity of mDownloadManager before calling its
> assert?  (And maybe having ~nsDownloadManager clear mDownloadManager in all the
> nsDownloads?)

I'm not sure. Of course that would prevent this symptom, but the cause might be
that the downloads should have been destroyed already earlier or something.

	Andrew Schultz Assignee
	Comment 19 • 19 years ago

I can reproduce this bug pretty easily with my gtk2 branch builds (I think... I
can't get a full stack, and it also occurs using progress dialogs).

	Stephen Donner [:stephend] Not actively reading bugmail Reporter
	Comment 20 • 19 years ago

*** Bug 308646 has been marked as a duplicate of this bug. ***

	Ian Neal
	Comment 21 • 19 years ago

I only get this on trunk builds BuildID 2005091007 and later (see duped bug for
details)

	Andrew Schultz Assignee
	Comment 22 • 19 years ago

I finally got a reasonable stack out of my branch builds, and it's the same as
attachment 192753 [details], even if I have both progress dialogs and download manager
disabled.  Also, I verified with valgrind that my other builds are also seeing
this bug (access the download manager after it's been deleted), but they just
don't crash.

	Ian Neal
	Comment 23 • 19 years ago

(In reply to comment #16)
> (In reply to comment #13)
> > The crash happens because DownloadManager dies before Download, but Download
> > calls DownloadManager in the destructor. Anyone interested in fixing this?
> 
> Is it as simple as checking the validity of mDownloadManager before calling its
> assert?  (And maybe having ~nsDownloadManager clear mDownloadManager in all the
> nsDownloads?)
> 
>
http://lxr.mozilla.org/seamonkey/source/xpfe/components/download-manager/src/nsDownloadManager.cpp#913

Adding if (mDownloadManager) into there does not seem to resolve the crashing.

	Stephen Donner [:stephend] Not actively reading bugmail Reporter
	Comment 24 • 19 years ago

Check out dbaron's comments here:
https://bugzilla.mozilla.org/show_bug.cgi?id=237736#c9

	Ere Maijala (slow)
	Comment 25 • 19 years ago

>
http://lxr.mozilla.org/seamonkey/source/xpfe/components/download-manager/src/nsDownloadManager.cpp#913
> 
> Adding if (mDownloadManager) into there does not seem to resolve the crashing.

Which is to be expected because there's nothing that would nullify it when the
download manager is destroyed. 

	Ere Maijala (slow)
	Comment 26 • 19 years ago

The patch for bug 304563 might fix this too. It probably caused the download
window not to close completely.

	Bruno 'Aqualon' Escherl
	Comment 27 • 19 years ago

(In reply to comment #26)
> The patch for bug 304563 might fix this too. It probably caused the download
> window not to close completely.
You meant bug 307563, adding autoScroll shouldn't have any effect on this crasher ;)

Can this bug still be seen with builds containing the patch for bug 307563? That
patch was checked in on 2005-10-15 11:18.

	Andrew Schultz Assignee
	Comment 28 • 19 years ago

Bug 307563 certainly wasn't causing the crash for me on linux.  I'm not seeing the crash with current trunk and branch builds, but I also don't see the bug with older builds that did crash.  Last I checked the talkback URL (the server seems to be down now), it still had current crashes.

	Robert Kaiser
	Comment 29 • 19 years ago

SeaMonkey 1.0 Beta is frozen, and we still have no clue what's happening, also this seems to be happening very rarely and not easily reproducable, so not blocking beta for this.

	timeless
	Comment 30 • 19 years ago

kairo@kairo.at: downloadmanager should observe xpcom shutdown and drop everything then. not later. (actually, so should nsExternalAppHandler).

	Adam Guthrie
	Comment 31 • 18 years ago

*** Bug 340115 has been marked as a duplicate of this bug. ***

	Andrew Schultz Assignee
	Comment 32 • 18 years ago

This keeps the download manager alive so long as there is a download.  nsDownloadManager removes its refs to the nsDownloads via ::DownloadEnded

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 33 • 18 years ago

this is actually strange come to think of it, because the externalapphandler should not hold a ref to the nsDownload by the time it gets destructed...

	Andrew Schultz Assignee
	Comment 34 • 18 years ago

Right, one might expect nsExternalAppHandler::CloseWindow, ::Cancel or ::OnStopRequest to be called before ~nsExternalAppHandler, but that isn't happening.  It's hard to tell from the stack how it got into this situation.

Anyway, the attached patch should a) fix the crash and b) is correct since nsDownload depends on nsDownloadManager existing as long as the nsDownload does.

	timeless
	Comment 35 • 18 years ago

so, the problem is that there's a js object which held a reference to exthandler

	xpcom_core.dll!PL_DHashTableOperate(PLDHashTable * table=0x0451b760, const void * key=0x0013f7a4, PLDHashOperator op=PL_DHASH_LOOKUP)  Line 490 + 0xd	C
  	docshell.dll!nsExternalAppHandler::~nsExternalAppHandler()  Line 1390 + 0x2a	C++
 	docshell.dll!nsExternalAppHandler::`scalar deleting destructor'()  + 0xf	C++
 	docshell.dll!nsExternalAppHandler::Release()  Line 1343 + 0x8e	C++

This object is being GCd because some js held a reference to it:
 	xpc3250.dll!XPCJSRuntime::GCCallback(JSContext * cx=0x0439c840, JSGCStatus status=JSGC_END)  Line 562 + 0x12	C++

That js world is being destroyed:
 	js3250.dll!JS_DestroyContext(JSContext * cx=0x0439c840)  Line 943 + 0xb	C

The js was in the global scope for a xul document, which goes away:
 	gklayout.dll!nsXULPDGlobalObject::SetContext(nsIScriptContext * aContext=0x00000000)  Line 811	C++

 	gklayout.dll!nsXULPrototypeDocument::~nsXULPrototypeDocument()  Line 271	C++
because the xul document is going away:
 	gklayout.dll!nsXULDocument::~nsXULDocument()  Line 444 + 0x46	C++
 	gklayout.dll!nsXULDocument::Release()  Line 476 + 0xc	C++

because the last js reference to the xul window was GC'd:
 	js3250.dll!js_GC(JSContext * cx=0x00aa3ba0, unsigned int gcflags=0)  Line 1947 + 0xf	C

because a js xpcom component is going away:
 	js3250.dll!JS_DestroyContext(JSContext * cx=0x00aa3ba0)  Line 943 + 0xb	C

because xpconnect is shutting down
 	xpc3250.dll!mozJSComponentLoader::UnloadAll(int aWhen=3)  Line 1007 + 0xd	C++

because everything's unloading
 	xpcom_core.dll!nsComponentManagerImpl::UnloadLibraries(nsIServiceManager * serviceMgr=0x00000000, int aWhen=3)  Line 3114 + 0x28	C++

because xpcom is shutting down
 	seamonkey.exe!NS_ShutdownXPCOM(nsIServiceManager * servMgr=0x00000000)  Line 171 + 0xa	C++

The root problem in a way is that some js xpcom component is holding a reference to a xul window which it probably shouldn't be doing. that could be your download manager component i suppose. this is why you don't see closewindow on the stack, the close window call would have happened, but the js reference kept the window alive. some weak references might be a good idea.

	Robert Kaiser
	Comment 36 • 18 years ago

Should this go into SeaMonkey 1.1 (on 1.8 branch)? If so, please nominate it for that by requesting approval-seamonkey1.1b? once it has reviews and fixes the crash on trunk.

This is no really major problem though, it's just annoying, so we won't push back a beta for it, we can still take such stability fixes (without L10n changes) after Beta and still have a more stable final.

	neil@parkwaycc.co.uk
	Comment 37 • 18 years ago

I still haven't found out which window (tricky to do in destructors), but I did find two references to button.xml in my latest stack.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 38 • 18 years ago

(In reply to comment #35)
> so, the problem is that there's a js object which held a reference to
> exthandler

That's not the entire answer. exthandler should still have dropped its reference to the download earlier. It seems kind of odd that the reference cycle here was broken (or these two objects couldn't be destructed) but the exthandler->download reference wasn't dropped earlier.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 39 • 18 years ago

I'd still like to know how this was possible though...

	neil@parkwaycc.co.uk
	Comment 40 • 18 years ago

(In reply to comment #39)
>I'd still like to know how this was possible though...
Something is definitely holding on to nsHelperAppDlg.xul - as well as causing the shutdown crash it's also triggering local store assertions (trying to flush the local store after the profile has been shut down).

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 41 • 18 years ago

fine, but necko should call OnStopRequest at some point, which releases the reference to mWebProgressListener...

I suppose it's possible that this happened before the user chose an action for the file. Urg, wish I remembered this code better...

	Andrew Schultz Assignee
	Comment 42 • 18 years ago

Comment on attachment 241802 [details] [diff] [review]
give nsDownload an owning ref to nsDownloadManager

preliminary talkback data suggests this fixed the crash on trunk.

	Robert Kaiser
	Comment 43 • 18 years ago

Comment on attachment 241802 [details] [diff] [review]
give nsDownload an owning ref to nsDownloadManager

In this case, a=me for 1.1b

	Andrew Schultz Assignee
	Comment 44 • 18 years ago

I landed the crash fix on the branch.  I filed bug 356848 for what's described in comment 33 and following.

	Stephen Donner [:stephend] Not actively reading bugmail Reporter
	Comment 45 • 17 years ago

Verified FIXED on trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/2007072004 Minefield/3.0a7pre, and my Windows Vista trunk build of Minefield, too.