495710 - WindowDataMap doesn't remove data objects for closed windows

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus Stange [:mstange]]

Attachment: fix v1

The WindowDataMap in nsWindowMap.mm wants to remove the data object for a window when it closes, i.e. when the windowWillClose notification fires. However, that notification never fires because the nsCocoaWindow destructor releases the window without calling close on it.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 380731 [details] [diff] [review]
fix v1

[NSWindow close:] gets called on all the windows we create if you
choose Quit from the Dock menu.

Better (I think) to send a custom notification, and have
TopLevelWindowData register for that (in addition to the
WindowWillClose notification).

Comment 2

[Markus Stange [:mstange]]

You're right, with this patch it's indeed possible that close is called twice. But I don't understand why you think this might be harmful - the docs don't mention that anything bad could happen in this case.
FWIW, the unit tests passed on the try server build I made with this patch, but that might not indicate much.

I've checked the history of the code lines I'm touching. The close call has been missing since pink's initial implementation of nsCocoaWindow in 2001; in 2002 hyatt replaced "release" with "autorelease", and in bug 342142 Josh added a setDelegate:nil... so there's nothing suspicious to find there.

I also tried out your suggestion: If I remove the line [mWindow setReleasedWhenClosed:NO]; and also remove [mWindow autorelease];, I don't hit any cases where close gets called twice on the same object. Instead I get some log messages on shutdown:
"GrP not requesting _registerDragTypes because the window is dying already"
Google says they're harmless and have been removed in 10.5 (I'm currently testing on 10.4).
I also added some logging to dealloc and verified that the objects still are deallocated properly.

So I still think that my current patch here is fine, but I'll gladly make another one that follows your suggestion. What's your opinion?
Josh, maybe you can give us some insights?

Comment 3

[Josh Aas]

I'll take a look at this next week, probably Tuesday. Thanks for looking into this.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 380731 [details] [diff] [review]
fix v1

Markus, I've changed my mind about your patch, and think it's fine as
it stands.  But let's see what Josh has to say next week.

I've confirmed that (with your patch) [NSWindow close] does get called
twice on every ToolbarWindow (ordinary top-level browser window) when
you quit from the Dock menu.  But (as best I can tell from the docs
and my limited tests) this isn't a problem when isReleasedWhenClosed
is 'false'.

Apple's docs (on [NSWindow setReleasedWhenClosed:]) say that when
isReleasedWhenClosed is false, calling [NSWindow close] only results
in the window being "hidden".  If this is truly what happens (as
appears to be the case), then there shouldn't be any problem calling
[NSWindow close] more than once on the same window (when
isReleasedWhenClosed is false).

The nsCocoaWindow destructor (which (with your patch) now calls
[NSWindow close] on its mWindow object) is always called before
[NSWindow close] is called again (on the same object) from [NSApp
terminate:].  At first this puzzled me -- why should [NSApp
terminate:] feel the need to call [NSWindow close] again?  But then I
realized that a hidden window stays in the window list (what's
returned by NSWindowList()).  [NSApp terminate:], when it's called,
"closes" every window that's still in the window list.  Apple's docs
on [NSWindow close] obliquely document this:  "For example, when the
application terminates, it sends the close message to all windows in
its window list, even those that are not currently visible."

I don't think there's anything wrong with my original suggestion (to
add the following to your patch -- get rid of the calls to [mWindow
setReleasedWhenClosed:NO] and [mWindow autorelease]).  But clearly
someone (we and/or Apple) continues to "use" the window after it's
been closed.  And my original suggestion makes that more likely to
cause trouble.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> But clearly someone (we and/or Apple) continues to "use" the window
> after it's been closed.

We should eventually try to get to the bottom of this (in another
bug).  But I don't consider it urgent, and in any case we've got
plenty of other things to do that (I think) take higher priority.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Needless to say, every WindowWillClose handler will now need to be checked to make sure it can deal with being called more than once.

Comment 7

[Markus Stange [:mstange]]

(In reply to comment #4)
> [NSApp terminate:], when it's called,
> "closes" every window that's still in the window list.  Apple's docs
> on [NSWindow close] obliquely document this:  "For example, when the
> application terminates, it sends the close message to all windows in
> its window list, even those that are not currently visible."

OK, that makes sense.

> Needless to say, every WindowWillClose handler will now need to be checked to
> make sure it can deal with being called more than once.

That doesn't seem to be necessary. Even though close gets called twice, I haven't seen a WindowWillClose notification fire twice. In my tests they were only fired once.
But anyway, we only have two of these handlers, and they're both safe.

Comment 8

[Markus Stange [:mstange]]

Attachment: alternative patch

For convenience, this is the patch that implements Steven's original suggestion. It has the ultimate advantage that it removes more lines than it adds.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 382186 [details] [diff] [review]
alternative patch

putting this into josh's review queue so he doesn't forget about it

Comment 10

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/bb7c02ccd0c4