Open Bug 1016952 Opened 8 years ago Updated 3 years ago

Dispatch a DOMWindowDestroyed event in nsGlobalWindow

Categories

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

defect
Not set
normal

Tracking

()

People

(Reporter: pbro, Unassigned)

Details

In the FirefoxDevTools we need to track all windows that exist in a given debuggee tab and react to when new windows get created or removed.

Right now, we use the progressListener defined here: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#1390

I'm working on this code in bug 1007021 as it doesn't notify its consumers when windows get destroyed.

What I'm doing now involves observing the "inner-window-destroyed" event, but it's a little bit awkward because I need to track the windows that I know about in my debuggee tab to then compare their IDs to the ID of the window I receive with this event, so as to filter out all events that aren't for my windows.

While it's definitely doable, it would make it a lot easier if a DOMWindowDestroyed event was dispatched by nsGlobalWindow, exactly like we now have DOMWindowCreated.
If we had this event, we could then do, with the debuggee tab's docShell, something like this:

let handler = docShell.chromeEventHandler;
handler.addEventListener("DOMWindowDestroyed", this._onWindowDestroyed, true);

and in _onWindowDestroyed(evt), we wouldn't have to worry about checking if the evt.target.defaultView is of interest to us.
What do you expect the event target to be, exactly?
Flags: needinfo?(pbrosset)
For DOMWindowCreated the event target is the HTMLDocument. This target would also be fine for DOMWindowDestroyed.
In my use case, I only really care about the Window object.
Flags: needinfo?(pbrosset)
There is no HTMLDocument to be had by the time the window is destroyed, since the document keeps the wndow alive.

There is no Window to be had either, since it's been destroyed.

The only thing we still have is the window ID of the now-gone window, which is what we pass to the observer notification...
In that case, would there be a way to dispatch a "DOMWindowWillBeDestroyed" kind of event. Something that will be sent before the HTMLDocument and Window get destroyed, similarly to how the DOMWindowCreated gets sent *after* the HTMLDocument and Window get created?
Hmm.  I suppose we could fire an event at the point at which we dispatch the current "dom-window-destroyed" observer notification.  We've been trying to avoid this because the window is about to enter a completely bogus state and we don't want people referencing it after that point, but the observer notification sort of hands it out anyway...

Olli, thoughts?
Flags: needinfo?(bugs)
So how does the setup actually work from JS point of view. dom-window-destroyed gives you nsISupports
pointing to an inner window. The event would be similar. Does JS get then access to the
outer window (which then uses the current inner, not the about-to-be-destroyed window)?

(it feels to me the safest option is to still use IDs for window destroying.)
Flags: needinfo?(peterv)
> Does JS get then access to the outer window (which then uses the current inner, not the
> about-to-be-destroyed window)

Yes, that's exactly what happens.  So if you actually examine the window in any way, it's not the same as the one you used to have.

> (it feels to me the safest option is to still use IDs for window destroying.)

That's why we introduced ids...
Flags: needinfo?(peterv)
(In reply to Boris Zbarsky [:bz] from comment #6)
> Hmm.  I suppose we could fire an event at the point at which we dispatch the
> current "dom-window-destroyed" observer notification.  We've been trying to
> avoid this because the window is about to enter a completely bogus state and
> we don't want people referencing it after that point, but the observer
> notification sort of hands it out anyway...
> 
> Olli, thoughts?
Oops. I just meant to cc, but let me say something about this quote.

Here you are describing what we are already doing in devtools codebase:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/storage.js#1507
And something similar is going to land in bug 1007021.
But instead of listening for dom-window-destroyed, we are using inner-window-destroyed and a Map() to get a reference to this somewhat bogus window.

I can easily agree we have to be careful about what we are doing with this window, and I think storage usage over here may be wrong:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/storage.js#277
  let host = this.getHostName(window.location);
I imagine window.location may race and already be set to the new location being loaded, or it may throw as being in middle of location change?

I'm starting to wonder if we should base all our code around inner window id.
So far the two usage I know about is storage. We want to remove data related to the document, here we can easily switch to an id instead of window or location.
The second usage in bug 1007021 is for sanity cleanup. We register some listener on documents via docShell.addWeakReflowObserver and we are aiming to unregister them when the document is destroyed.
May be we should just expect listeners to be freed and do nothing? May be we should listen for webnavigation-destroy that tells us when a docshell is destroyed?
Hmm, I thought I cleared the needinfo.

I don't really see a sane way to pass a window object to JS when window is about to be destroyed.
In fact, I think we should not pass any window with dom-window-destroyed, but only ID, similar to
inner-window-destroyed / outer-window-destroyed.
(But changing how dom-window-destroyed behaves might break quite some addons)
Flags: needinfo?(bugs)
> I imagine window.location may race and already be set to the new location being loaded,

I'm 99.9% sure that for a navigation the location is set to the new thing being loaded at this point, yes.  It's not even a race: it's deterministically the new thing.

> May be we should just expect listeners to be freed and do nothing?

Generally speaking, yes.

One issue, though, is that addWeakReflowObserver adds the observer to the docshell, not the document.  So normally it would stick around on the docshell as you navigate.  If you don't want that behavior, then you do need to remove the observers at the point when you want to stop observing.

If you want the observer to stick around while the docshell is alive, you can just not remove it; the docshell holds a weak ref to the observer, so there are no memory cycles involved or anything.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.