In a current pull from 10/11, I'm starting to see assertions in the release method for nsWebShellWindow about us deleting the window twice. I'm assigning this to dp and cc'ing waterson because I'm guessing it may be related to fixing the webshell leak that dp tracked down last week. cc'ing waterson because the solution may involve changes he's going to be making to the tree. Here's the stack trace with comments below: nsDebug::PreCondition(const char * 0x01402380, const char * 0x01402370, const char * 0x0140233c, int 257) line 262 + 13 bytes nsWebShellWindow::Release(nsWebShellWindow * const 0x011abad0) line 257 + 41 bytes nsWebShell::SetDocLoaderObserver(nsWebShell * const 0x011aacd0, nsIDocumentLoaderObserver * 0x00000000) line 1439 + 27 bytes nsWebShell::Destroy(nsWebShell * const 0x011aacd0) line 1146 nsWebShellWindow::~nsWebShellWindow() line 236 nsWebShellWindow::`scalar deleting destructor'() + 15 bytes nsWebShellWindow::Release(nsWebShellWindow * const 0x011abad0) line 257 + 151 bytes nsWebShell::SetDocLoaderObserver(nsWebShell * const 0x011aacd0, nsIDocumentLoaderObserver * 0x00000000) line 1439 + 27 bytes nsWebShell::Destroy(nsWebShell * const 0x011aacd0) line 1146 nsWebShellWindow::Close(nsWebShellWindow * const 0x011abad0) line 439 nsAppShellService::~nsAppShellService() line 145 nsAppShellService::`scalar deleting destructor'(unsigned int 1) + 15 bytes nsAppShellService::Release(nsAppShellService * const 0x0112d5d0) line 153 + 147 bytes When closing a webshell shell window, we are destroying a webshell. Destroying the webshell is causing us to release the last reference on the webshell window so we call into it's dtor which causes us to call Destroy on the webshell again! (all on the same stack trace) This causes a re-entrancy problem which leads to the problems I'm seeing.
What is the test case. Always on shutdown or anything in particular.
Upon further review, I'll go look into fixing this myself so dp doesn't have to worry about it =). At the very least, we should make nsWebshell::Destroy handle the re-entrancy case nicely.
dp, to answer your question, I see this about 50% of the time on shut down on my machine. I can't remember if I've ever seen it if I just run the browser. I know I've seen it when running mailnews and the browser together.
A couple other strange things in here... This is all getting started because the app shell service is getting destroyed. In its dtor, it's closing the hidden window. In closing the hidden window (which is a nsWebShellWidow), we attempt to create the app shell service and unregiser ourself). Of course we're in the dtor of the appshell service so this kind of behavior can't be good either.
I'm now drifting outside of the scope of this bug report. But it looks to me like NS_ShutdownXPCOM is manually deleting all the services (which makes sense since we're trying to cleanup leaks). But shouldn't we be shutting down the services before deleting them? (That sounds vague I know). But in the case of the appshell service, the dtor doesn't do anything involving closing all the open windows. It does have a method on it nsAppShellService::Quit which enumerates over all known windows and asks them to close and shut down. There's also a nsAppShellService::Shutdown which shuts down all the app shell components. It almost seems like we want to be pulling these two methods into the loop before we actually delete the service.
Please ignore my comments about nsAppShellService::Shutdown and nsAppShellService::Quit not being called. I'm sure they are getting called long before we go to destroy the app shell service.
Here's a test case I used to reproduce this crash every time: Start up with -mail. Open a browser window. Select File --> Quit. I'm attaching a one line patch to nsWebShellWindow::Close which just does this at the beginning of the method: nsCOMPtr<nsIWebShellWindow> placeHolder = this; This ensures that the web shell window can't get deleted out from under the Close call if the webshell the window destroys is holidng the last reference on the window. dp can you review this for me?
Patch looks good. Wouldn't the right fix be eliminate the cause for the webshell to turn around and delete the webshellwindow, like removing the webshellwindow from the observer list. What do you think ? If you are convinced that is a longer, tedious approach then fine. Just wanted to make sure you thought of it.
Hey dp, I just checked in the patch. About your comments. The problem this patch is fixing is the window's close method is destorying a webshell. The webshell has the last outstanding reference to the window so in closing the webshell, we end up destorying the webshell window too. But we're in the middle of the webshell window's close method on the stack. And that's bad. So I don't think there is a more involved solution that would fix this. Thanks for the review!
rubberstamp vrfy [old checkin].