pagehide events don't fire on tab close

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Brian Ryner (not reading))

Tracking

({fixed1.8})

Trunk
fixed1.8
Points:
---
Bug Flags:
blocking1.8rc1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
When a tab is closed, the unload event is fired correctly, but pagehide is not.
(Assignee)

Comment 1

12 years ago
Created attachment 198383 [details] [diff] [review]
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.
Attachment #198383 - Flags: superreview?(bzbarsky)
Attachment #198383 - Flags: review?(roc)
Won't HTML iframes (which can end up with no presentation at all well before the
docshell is destroyed) have the same problem?
(Assignee)

Comment 3

12 years ago
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.
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.
(Assignee)

Comment 5

12 years ago
There's another option, which isn't great but works and already exists... 
force creation of the DOMEvent up-front and use nsIPrivateDOMEvent::SetTarget
().
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.
Brian, you're making a new patch here, right?
(Assignee)

Comment 8

12 years ago
Comment on attachment 198383 [details] [diff] [review]
patch

yep, i'll be posting a new patch.
Attachment #198383 - Attachment is obsolete: true
Attachment #198383 - Flags: superreview?(bzbarsky)
Attachment #198383 - Flags: review?(roc)
(Assignee)

Comment 9

12 years ago
Created attachment 199106 [details] [diff] [review]
patch

Don't rely on a PresContext.
Attachment #199106 - Flags: superreview?(bzbarsky)
Attachment #199106 - Flags: review?(bzbarsky)
Comment on attachment 199106 [details] [diff] [review]
patch

Most excellent.
Attachment #199106 - Flags: superreview?(bzbarsky)
Attachment #199106 - Flags: superreview+
Attachment #199106 - Flags: review?(bzbarsky)
Attachment #199106 - Flags: review+
(Assignee)

Comment 11

12 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

12 years ago
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).
Attachment #199106 - Flags: approval1.8rc1?

Comment 13

12 years ago
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?

Updated

12 years ago
Flags: blocking1.8rc1?
(Assignee)

Comment 14

12 years ago
(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.

Updated

12 years ago
Attachment #199106 - Flags: approval1.8rc1? → approval1.8rc1+

Updated

12 years ago
Flags: blocking1.8rc1? → blocking1.8rc1+
(Assignee)

Updated

12 years ago
Keywords: fixed1.8

Comment 15

12 years ago
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.
You need to log in before you can comment on or make changes to this bug.