484710 - add outer/inner-window-destroyed notifications for pages that are ejected from the bfcache (initially suggested a pageIgnore event)

Status: RESOLVED FIXED

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

Description

[John J. Barton]

Firebug creates a side table of information for every page it debugs. It needs to delete this side table in a timely fashion to avoid bloating the memory. In the case that a page is in the bfcache the information must be held indefinately because there is no event to signal that the page is ejected.  A 'pageIgnore' event would complement the current pageHide (write cache) and pageShow (read from cache).

(Attaching to bug 449452 as they both relate to improvements for firebug).

Comment 1

[John J. Barton]

Copying some info from the newsgroup to give context for this bug

John J. Barton wrote:
> When a user clicks a link that replaces the view in the current tab with a new page, what is the fate of the nsIDOMWindow supporting the view they click on?  Or: what happens when they hit the 'back' button?

The outer nsIDOMWindow (the |window| object in content JS, aka the <browser>'s contentWindow object in the parent document) stays right where it is.  It just has its inner window and document replaced.

The inner nsIDOMWindow (the global scope object in content JS) is either thrown away when GC happens) or frozen and placed in the bfcache.  When the user hits the 'back' button, either the window in bfcache is thawed or a new inner window is created, depending on whether there's a window in the bfcache.

> Is the page reloaded? loaded from cache? or is the DOM saved in some way as if the window was there but invisible? Are scripts recompiled? Where they live while the page was invisible?

In the non-bfcache case, the page is reloaded in the sense of reparsing from the original HTML source.  That might come from the HTTP cache or not, depending on whether it's been evicted; typically it does come from the HTTP cache.  The DOM is recreated during the parsing process, scripts are recompiled (and also reloaded, probably from the HTTP cache).

In the bfcache case, freezing the window just marks it as frozen; the DOM is preserved, as are compiled script objects.  Thawing marks it as thawed.  Freezing suspends timeouts and interval timers on the frozen window, so they won't execute.  The script that's running when the window is frozen runs to completion, as it would if it were being closed, for example.  After that, only scripts running on other windows can touch this window; if they modify the DOM the cached DOM and window is thrown away.

-Boris

Boris Zbarsky wrote:
> John J. Barton wrote:
>> And to detect this we use pagehide/pageshow correct?
>
> That's correct.  The pagehide event tells you whether the page is going into bfcache; the pageshow tells you whether it's coming from bfcache. Note that just because a page went into bfcache doesn't mean it'll ever come out again, like any other cache.

...
> Hmm, so what event tells me "you'll never get a pageshow so you can drop the megabytes of info you've saved in Firebug side table for that page?"

Pagehide if the page is not going into bfcache.  If it is, I don't know that there's a notification that fires.  Might be worth filing a bug asking for one.

-Boris

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

pageIgnore could and should be handled only in chrome.

I'm not sure the event name 'pageIgnore' is the best possible.
Maybe 'pageUnloaded' or 'pageFinalized'? And the event should be probably fired
also for documents which aren't bfcached.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes on all counts for comment 2.

Smaug, want to implement this?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Adding event dispatching to nsDocument::Destroy doesn't work when
tabs are closed. So need to find some other way.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I don't think Destroy() is the right place for it.  The right place is right after unload if we're firing unload, or when we evict from bfcache if we do that.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Yeah, probably. I was just trying find one place which could handle both
cases.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

"The close tab" case is interesting anyway. Where should the event be dispatched?
I guess to chrome event handler, but it (<browser>) isn't necessarily in document anymore.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, well... how does unload work?  This should work the same, imo.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Found during triage. This bug seems to have stalled. Olli, any plans on picking this up again or have you moved on? Any better thoughts on where to implement this since you've looked at it last?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the existing dom-window-destroyed notification sort of does this, but hands inner windows out.

jst, would it make sense to dispatch the window-id notification when we send dom-window-destroyed?  I'd been planning to do it in nsGlobalWindow::Cleanup instead....

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

The test change shows how to use this.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 444921 [details] [diff] [review]
Like so

It is possible that the notification fires at very unsafe times, like in middle of cycle collection?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm...  The one from SetDocShell maybe, but we have an existing notification there too (which passes out the window pointer at that!).

ReallyClearScope can happen in several ways.  One is off a thread nsIRunnable (so completely safe).  Another is via a window state holder destructor.  Another is under SetNewDocument.  Another is under SetDocShell on the outer.

So maybe I should just do this notification async.  That shouldn't be a problem.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Do it off an event

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/2661c7866ca1

Comment 16

[Eric Shepherd [:sheppy]]

Looking at this to see what if any documentation work needs doing. Looks like the final implementation differs substantially from the summary, as there doesn't appear to be a new pageIgnore event added here.

Given that, is there actually anything in need of documenting here? Looks like there are new outer-window-destroyed and inner-window-destroyed notifications, but I'm unclear on whether these are "interesting" to anyone outside the deep guts of the code.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Given that, is there actually anything in need of documenting here? 

The new observer service notifications.

> but I'm unclear on whether these are "interesting" to anyone outside the deep
> guts of the code.

They're of no interest to the deep guts.  They're provided to enable extensions to know when a window has gone away so they can drop whatever information they were storing for that window, if any.

Comment 18

[Eric Shepherd [:sheppy]]

documented: https://developer.mozilla.org/en/Observer_Notifications#Windows

Comment 19

[John J. Barton]

Sorry I can't work out what the docs are telling me. 

In 
https://developer.mozilla.org/En/Working_with_BFCache
I'm good until the very end:
-----
The pagehide event tells you whether the page is going into bfcache; the pageshow tells you whether it's coming from bfcache. ....

Q: Hmm, so what event tells me "you'll never get a pageshow so you can drop the megabytes of info you've saved in Firebug side table for that page?"

A: Pagehide if the page is not going into bfcache.  If it is, I don't know that there's a notification that fires. 
-----
The last answer seems to contradict the previous info, and the last sentence is now out of date because of this bug fixed correct?

So inner-window-destroyed / outer-window-destroyed means we'll never get a pageshow right?

Isn't inner-window-destroyed/outer-window-destroyed really independent of the bfcache?  That is we can get

pagehide ... inner-window-destroyed : we went in to the bfcache, then later died
pagehide ... pageshow...pagehide...inner-window-destroyed: in and out cache, then die
inner-window-destroyed: never cached, just died.

or?

Comment 20

[John J. Barton]

For inner-window-destroyed I get:
   subject  nsISupports
   topic    "inner-window-destroyed"
   data     null
And similar for outer-window-destroyed.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

From subject you get the window ID so that you know which window is destroyed.

QueryInterface(Components.interfaces.nsISupportsPRUint64).data

Comment 22

[Boris Zbarsky [:bzbarsky]]

> In  https://developer.mozilla.org/En/Working_with_BFCache

That just never got updated.  I've fixed that page.  It needs a complete rewrite anyway; it was just a brain-dump that never got prettied up...

> The last answer seems to contradict the previous info

Not that I can see.

> and the last sentence is now out of date because of this bug fixed correct?

Yes.

> So inner-window-destroyed / outer-window-destroyed means we'll never get a
> pageshow right?

inner-window-destroyed for an inner window means that page has gone away and won't be shown again, yes.

> Isn't inner-window-destroyed/outer-window-destroyed really independent of the
> bfcache?

Yes.  The idea is that for cases like Firebug you can just listen to inner-window-destroyed and not worry about bfcache at all, if desired.

> pagehide ... inner-window-destroyed : we went in to the bfcache, then later
> died

Or didn't go into bfcache and died.  pagehide fires whenever a page is unloaded, whether it's going into bfcache or not.  The event has a boolean property that tells you whether it's being cached.

> inner-window-destroyed: never cached, just died.

This will never happen on its own; it'll always be accompanied by a pagehide.  See above.

Smaug covered what the subject of the notification is, and the bfcache page you linked to documents it too, as does the actual documentation added for this bug (see comment 18).

Comment 23

[John J. Barton]

(In reply to comment #21)
> From subject you get the window ID so that you know which window is destroyed.
> 
> QueryInterface(Components.interfaces.nsISupportsPRUint64).data

So the 'data' field is null but the subject has a secret field called 'data' that is actually the window id. Why didn't I think of that?

I corrected the documentation page from comment 18, please check it.

Comment 24

[Nickolay_Ponomarev]

resummarizing to mention the notifications added instead of the event.