309581 - pagehide events don't fire on tab close

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Brian Ryner (not reading)]

When a tab is closed, the unload event is fired correctly, but pagehide is not.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

This makes us destroy the frameloader before destroying the content viewer. 
Frameloader destruction tears down the docshell, which dispatches the
pagehide/unload events.  If we've already destroyed the content viewer, the
document can't get a prescontext to use for the event and bails.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Won't HTML iframes (which can end up with no presentation at all well before the
docshell is destroyed) have the same problem?

Comment 3

[Brian Ryner (not reading)]

Probably... but I don't know what a good solution is in that case.  We could 
use a null nsPresContext for the event, I suppose, but I'm wary because of the 
weird way that the prescontext is used to determine the event target.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is this something for 1.9, or for 1.8?  For 1.9 we should definitely just fix
that aspect of event targeting.

For 1.8, I would be tempted to add a |target| member to nsEvent that can be used
as a last fallback if non-null by nsDOMEvent::GetTarget.  And set it when we
fire pagehide/pageshow events.  And then use null prescontexts.

Or maybe even not as a last fallback -- if I read this code right the target of
a pagehide can be some random content node if it happens to be the ESM's
currentTarget when we fire the PAGE_HIDE.

Comment 5

[Brian Ryner (not reading)]

There's another option, which isn't great but works and already exists... 
force creation of the DOMEvent up-front and use nsIPrivateDOMEvent::SetTarget
().

Comment 6

[Boris Zbarsky [:bzbarsky]]

That doesn't sound so bad -- we end up creating the event anyway as soon as we
have an event listener for it, and I think we always do here.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Brian, you're making a new patch here, right?

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 198383 [details] [diff] [review]
patch

yep, i'll be posting a new patch.

Comment 9

[Brian Ryner (not reading)]

Attachment: patch

Don't rely on a PresContext.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 199106 [details] [diff] [review]
patch

Most excellent.

Comment 11

[Brian Ryner (not reading)]

checked in

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 199106 [details] [diff] [review]
patch

I'd like to land this fix on the branch.  It's important for extensions that
want to be notified that a page is no longer visible, and should be safe
(doesn't affect other codepaths).

Comment 13

[Asa Dotzler [:asa]]

bryner, is there any chance this would fix the problem at bug 312027. If not,
can you look into that and tell us what you think?

Comment 14

[Brian Ryner (not reading)]

(In reply to comment #13)
> bryner, is there any chance this would fix the problem at bug 312027. If not,
> can you look into that and tell us what you think?

It's unlikely that this patch will address that problem.

Comment 15

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

Although nsDOMEvent::DuplicatePrivateData is no-op atm,
nsDocument::DispatchEventToWindow should call it if there are more than one
references to domEvt after sgo->HandleDOMEvent. I noticed this while doing some
fixes to event dispatching (in trunk).
The problem is that DOMEvent doesn't own nsEvent (because nsEvent is in stack)
when events are dispatched in this strange way.

But this is probably not a problem in branch because 
DuplicatePrivateData is anyway no-op.