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

VERIFIED FIXED

Status

SeaMonkey
Download & File Handling
--
critical
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: stephend, Assigned: Andrew Schultz)

Tracking

({crash, fixed-seamonkey1.1b, topcrash})

Trunk
x86
All
crash, fixed-seamonkey1.1b, topcrash
Dependency tree / graph
Bug Flags:
blocking-seamonkey1.0b -

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(3 attachments)

(Reporter)

Description

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

Comment 1

13 years ago
Created attachment 192719 [details]
Screenshot
(Reporter)

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

13 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.
Also wfm with 20050814 trunk SeaMonkey build. I've tried to reproduce with the
steps in comment 5.
Blocks: 297561

Comment 7

13 years ago
I'm able to reproduce. Investigating..

Comment 8

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

Comment 9

13 years ago
Created attachment 192753 [details]
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

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

Comment 11

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

Comment 12

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

Comment 13

13 years ago
The crash happens because DownloadManager dies before Download, but Download
calls DownloadManager in the destructor. Anyone interested in fixing this?
(Reporter)

Comment 14

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

Updated

13 years ago
Severity: major → critical
Keywords: crash
Summary: "Opening [filename.ext]" dialog won't close (GDI leak?) → Crash on shutdown in [@ PL_DHashTableOperate] after downloading using Download Manager
(Assignee)

Comment 15

13 years ago
worksforme with linux seamonkey trunk 2005082703
so this is a download manager bug?
(Reporter)

Updated

13 years ago
Component: Widget: Win32 → Download Manager
Product: Core → Mozilla Application Suite
(Assignee)

Updated

13 years ago
Assignee: win32 → download-manager
QA Contact: ian
(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
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

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

Comment 19

12 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).
Flags: blocking-seamonkey1.0b?
(Reporter)

Comment 20

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

Comment 21

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

Comment 22

12 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.
OS: Windows XP → All
Summary: Crash on shutdown in [@ PL_DHashTableOperate] after downloading using Download Manager → Crash on shutdown in [@ PL_DHashTableOperate] after downloading

Comment 23

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

Comment 24

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

Comment 25

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

Comment 26

12 years ago
The patch for bug 304563 might fix this too. It probably caused the download
window not to close completely.
Depends on: 307563
(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.
(Reporter)

Updated

12 years ago

Updated

12 years ago
Summary: Crash on shutdown in [@ PL_DHashTableOperate] after downloading → Crash on shutdown in [@ PL_DHashTableOperate] (nsDownload::~nsDownload) after downloading
(Assignee)

Comment 28

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

Comment 29

12 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.
Flags: blocking-seamonkey1.0b? → blocking-seamonkey1.0b-

Comment 30

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

Comment 31

11 years ago
*** Bug 340115 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

11 years ago
Keywords: topcrash
Summary: Crash on shutdown in [@ PL_DHashTableOperate] (nsDownload::~nsDownload) after downloading → Crash on shutdown in [@ SearchTable] [@ PL_DHashTableOperate] (nsDownload::~nsDownload) after downloading
(Assignee)

Comment 32

11 years ago
Created attachment 241802 [details] [diff] [review]
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
Assignee: download-manager → ajschult
Status: NEW → ASSIGNED
Attachment #241802 - Flags: review?(cbiesinger)
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...
(Assignee)

Comment 34

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

Comment 35

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

Comment 36

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

Comment 37

11 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.
(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.
Attachment #241802 - Flags: review?(cbiesinger) → review+
I'd still like to know how this was possible though...

Comment 40

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

Comment 42

11 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.
Attachment #241802 - Flags: approval-seamonkey1.1b?

Comment 43

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

In this case, a=me for 1.1b
Attachment #241802 - Flags: approval-seamonkey1.1b? → approval-seamonkey1.1b+
(Assignee)

Comment 44

11 years ago
I landed the crash fix on the branch.  I filed bug 356848 for what's described in comment 33 and following.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed-seamonkey1.1b
Resolution: --- → FIXED
(Reporter)

Comment 45

11 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.
Status: RESOLVED → VERIFIED
Crash Signature: [@ SearchTable] [@ PL_DHashTableOperate]
You need to log in before you can comment on or make changes to this bug.