If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED

Status

SeaMonkey
UI Design
P3
normal
VERIFIED FIXED
18 years ago
13 years ago

People

(Reporter: Scott MacGregor, Assigned: Scott MacGregor)

Tracking

Trunk
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

18 years ago
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

18 years ago
What is the test case. Always on shutdown or anything in particular.
(Assignee)

Updated

18 years ago
Assignee: dp → mscott
(Assignee)

Comment 2

18 years ago
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.
(Assignee)

Comment 3

18 years ago
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.
(Assignee)

Comment 4

18 years ago
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.
(Assignee)

Comment 5

18 years ago
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.
(Assignee)

Comment 6

18 years ago
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.
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Whiteboard: Fix ready...pending code review
(Assignee)

Comment 7

18 years ago
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?
(Assignee)

Comment 8

18 years ago
Created attachment 2092 [details] [diff] [review]
patch for nsWebShellWindow.cpp

Comment 9

18 years ago
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.
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Whiteboard: Fix ready...pending code review
(Assignee)

Comment 10

18 years ago
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!

Updated

17 years ago
QA Contact: don → sairuh
rubberstamp vrfy [old checkin].
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.