528706 - nsIWindowMediator::getMostRecentWindow should not return closed windows

Status: NEW

(Core :: Widget, defect, P3)

Description

[Dão Gottwald [:dao]]

I don't know if there's a legitimate use for getMostRecentWindow returning a window that is already closed, but in browser code it's probably never what we need. See bug 528440 where we worked around this.

Comment 1

[Marco Bonardo [:mak]]

or eventually could provide getMostRecentOpenWindow if the behavior is expected.

Comment 2

[Marco Bonardo [:mak]]

or an optional aOnlyOpenWindows param.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
> I don't know if there's a legitimate use for getMostRecentWindow returning a
> window that is already closed

I doubt it. It looks to me like it can only happen if you call window.close() from JS and then call getMostRecentWindow() before returning to the event loop (i.e. before the nsCloseEvent fires).

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> It looks to me like it can only happen if you call window.close()
> from JS and then call getMostRecentWindow() before returning to the event loop
> (i.e. before the nsCloseEvent fires).

Doesn't that mean that http://hg.mozilla.org/mozilla-central/rev/9eeafcb0fc7d wouldn't have helped for the "Only one window should exist after cleanup - Got 2, expected 1" case in bug 527074, since executeSoon is used between tests?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I don't know. There may well be other cases where it can occur, since the code isn't super easy to follow.

Are you confident that that change helped?

Comment 6

[Dão Gottwald [:dao]]

Yes, that failure is the first one that I managed to get rid of by simply not having the windows counting function count closed windows.

Comment 7

[Dão Gottwald [:dao]]

I should note that I got that failure locally quite constantly, which is why I was so fast pushing the fix. See bug 527074 comment 29.

Comment 8

[Marco Bonardo [:mak]]

This bug alone just made me lose HOURS of debugging to figure why on a yield my objects were dying when with the old synchronous API they were not.

Considered we are moving more and more to async, this may now have a much larger impact.

I think we should just change it to not return a window if .closed is true.
Anything against that?