Closed Bug 484710 Opened 11 years ago Closed 10 years ago
add outer/inner-window-destroyed notifications for pages that are ejected from the bfcache (initially suggested a page
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).
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
No longer blocks: 449452
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.
Yes on all counts for comment 2. Smaug, want to implement this?
Adding event dispatching to nsDocument::Destroy doesn't work when tabs are closed. So need to find some other way.
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.
Yeah, probably. I was just trying find one place which could handle both cases.
"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.
Yeah, well... how does unload work? This should work the same, imo.
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?
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....
The test change shows how to use this.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #444921 - Flags: review?(jst)
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?
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
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.
> 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.
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?
For inner-window-destroyed I get: subject nsISupports topic "inner-window-destroyed" data null And similar for outer-window-destroyed.
From subject you get the window ID so that you know which window is destroyed. QueryInterface(Components.interfaces.nsISupportsPRUint64).data
> 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).
(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.
resummarizing to mention the notifications added instead of the event.
Summary: add pageIgnore event for pages that are ejected from the bfcache → add outer/inner-window-destroyed notifications for pages that are ejected from the bfcache (initially suggested a pageIgnore event)
You need to log in before you can comment on or make changes to this bug.