304680 - Crash on shutdown in [@ SearchTable] [@ PL_DHashTableOperate] (nsDownload::~nsDownload) after downloading

Status: VERIFIED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

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

*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.

Comment 1

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

Attachment: Screenshot

Comment 2

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

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

Comment 3

[Hermann Schwab]

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.

Comment 4

[Hermann Schwab]

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.

Comment 5

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

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.

Comment 6

[Martijn Wargers (dead)]

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

Comment 7

[Ere Maijala (slow)]

I'm able to reproduce. Investigating..

Comment 8

[Ere Maijala (slow)]

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.

Comment 9

[Ere Maijala (slow)]

Attachment: Crash stack

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.

Comment 10

[OstGote!]

(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.

Comment 11

[Ere Maijala (slow)]

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

Comment 12

[Ere Maijala (slow)]

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

Comment 13

[Ere Maijala (slow)]

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

Comment 14

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

(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.

Comment 15

[Andrew Schultz]

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

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(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

Comment 17

[Samuel Sidler (old account; do not CC)]

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

Comment 18

[Ere Maijala (slow)]

(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.

Comment 19

[Andrew Schultz]

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).

Comment 20

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

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

Comment 21

[Ian Neal]

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

Comment 22

[Andrew Schultz]

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.

Comment 23

[Ian Neal]

(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.

Comment 24

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

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

Comment 25

[Ere Maijala (slow)]

>
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. 

Comment 26

[Ere Maijala (slow)]

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

Comment 27

[Bruno 'Aqualon' Escherl]

(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.

Comment 28

[Andrew Schultz]

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.

Comment 29

[Robert Kaiser]

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.

Comment 30

[timeless]

[e-mail]: downloadmanager should observe xpcom shutdown and drop everything then. not later. (actually, so should nsExternalAppHandler).

Comment 31

[Adam Guthrie]

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

Comment 32

[Andrew Schultz]

Attachment: give nsDownload an owning ref to nsDownloadManager

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

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

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...

Comment 34

[Andrew Schultz]

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.

Comment 35

[timeless]

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.

Comment 36

[Robert Kaiser]

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.

Comment 37

[[e-mail]]

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.

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

(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.

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 40

[[e-mail]]

(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).

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

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...

Comment 42

[Andrew Schultz]

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

preliminary talkback data suggests this fixed the crash on trunk.

Comment 43

[Robert Kaiser]

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

In this case, a=me for 1.1b

Comment 44

[Andrew Schultz]

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

Comment 45

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

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.