16115 - we're trying to delete a nsWebShellWindow twice...

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

[Scott MacGregor]

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.

Comment 1

[Suresh Duddi (gone)]

What is the test case. Always on shutdown or anything in particular.

Comment 2

[Scott MacGregor]

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.

Comment 3

[Scott MacGregor]

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.

Comment 4

[Scott MacGregor]

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.

Comment 5

[Scott MacGregor]

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.

Comment 6

[Scott MacGregor]

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.

Comment 7

[Scott MacGregor]

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?

Comment 8

[Scott MacGregor]

Attachment: patch for nsWebShellWindow.cpp

Comment 9

[Suresh Duddi (gone)]

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.

Comment 10

[Scott MacGregor]

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!

Comment 11

[sairuh (rarely reading bugmail)]

rubberstamp vrfy [old checkin].