917371 - Fire a notification when a window is well and truly closed

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Dave Townsend [:mossop]]

Very frequently in tests we have to close a window and wait for it to go away. As our platform becomes more and more async that becomes harder. There are a number of events and notifications currently but all seem to fire during the closing of a window, not after, or don't include information like which window is closing. At the moment we have lots of test code using things like executeSoon or focus detection etc. to figure out when the window is finally gone. This is a recipe for intermittent failures. It would be better if we just had a reliable notification for this.

I think the goals would be that when you receive the notification you can rely on the window no longer being visible, not appearing in the windowmediator APIs or generally being reachable through any API.

It might be nice if this notification also only happens after the next window has received focus as that is often what we care about but that's open to debate.

Comment 1

[Bobby Holley (:bholley)]

Dave, Gavin, Dolske - can you guys decide what you want this notification to look like? The actual patch to do it should be trivial. It's mostly just an issue of writing tests, which we can probably get for free, because Dolske has volunteered to update the password manager tests to use whatever we make here.

Comment 2

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

A "chrome-window-closed" notification that is passed an outer window ID, perhaps?

Comment 3

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

Being passed the actual window would make things easier, but if it's not a useful object at that point (e.g. its JS has been torn down etc.) that might be confusing to people.

Comment 4

[Bobby Holley (:bholley)]

I hate  to introduce new near-redundant notifications, and xul-window-destroyed _almost_ fits the bill. It's used very sparingly in the tree and not at all in addons. So I think we could get away with firing it async and passing the outer window ID as aData.

Gavin, how does that sound? I assume the outer window ID I should pass should be that of the top-level chrome docshell?

Comment 5

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

Yeah, sounds reasonable.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Introduce the ability to asynchronously notify observers. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Notify xul-window-destroyed asynchonously. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Deliver the outer window ID (in string form) as aData for xul-window-destroyed. v1

Comment 9

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=02f7b9388eea

Comment 10

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

Hmm, wait, there's already also a outer-window-destroyed notification.

Comment 11

[Bobby Holley (:bholley)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> Hmm, wait, there's already also a outer-window-destroyed notification.

So there is!

I wonder what ordering guarantees this has, if any, with the destruction of the native XULWindow.

Comment 12

[Benjamin Smedberg]

It appears that I should hold off reviews, given comment 10-11

Comment 13

[Bobby Holley (:bholley)]

Mossop, is the given notification sufficent?

Comment 14

[Dave Townsend [:mossop]]

(In reply to Bobby Holley (:bholley) from comment #13)
> Mossop, is the given notification sufficent?

Looks like this happens after the window is removed from the window tracker? Then I think probably yes

Comment 15

[Bobby Holley (:bholley)]

And for you, dolske?

Comment 16

[Dave Townsend [:mossop]]

(In reply to Dave Townsend (:Mossop) from comment #14)
> (In reply to Bobby Holley (:bholley) from comment #13)
> > Mossop, is the given notification sufficent?
> 
> Looks like this happens after the window is removed from the window tracker?
> Then I think probably yes

I attached a change to bug 924500 that reverts the change from bug 874502 that started this and switches to outer-window-destroyed and it seems work well.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 811237 [details] [diff] [review]
Part 1 - Introduce the ability to asynchronously notify observers. v1

Ok, sounds like we can mostly WONTFIX this bug.

I think it might still be worth landing my async observer notification machinery though, since it lets consumers avoid rolling custom runnables. Flagging bsmedberg for review on that.

Comment 18

[Benjamin Smedberg]

Comment on attachment 811237 [details] [diff] [review]
Part 1 - Introduce the ability to asynchronously notify observers. v1

I don't like nsIObserverService for its hard-coded wstring and nsISupports params, so unless we really need this I'd prefer to avoid it for now. Just topic/subject would be better, for lack of a nice non-JS variant.

Comment 19

[Justin Dolske [:Dolske]]

I don't have any opinion on this.

Comment 20

[Bobby Holley (:bholley)]

Alright, resolving WFM.