Last Comment Bug 309581 - pagehide events don't fire on tab close
: pagehide events don't fire on tab close
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Brian Ryner (not reading)
: Hixie (not reading bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-21 23:47 PDT by Brian Ryner (not reading)
Modified: 2005-10-14 08:01 PDT (History)
4 users (show)
asa: blocking1.8rc1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.44 KB, patch)
2005-10-03 16:54 PDT, Brian Ryner (not reading)
no flags Details | Diff | Splinter Review
patch (3.14 KB, patch)
2005-10-10 15:07 PDT, Brian Ryner (not reading)
bzbarsky: review+
bzbarsky: superreview+
asa: approval1.8rc1+
Details | Diff | Splinter Review

Description Brian Ryner (not reading) 2005-09-21 23:47:00 PDT
When a tab is closed, the unload event is fired correctly, but pagehide is not.
Comment 1 Brian Ryner (not reading) 2005-10-03 16:54:13 PDT
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.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2005-10-03 17:06:01 PDT
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) 2005-10-03 17:14:15 PDT
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 [:bz] (still a bit busy) 2005-10-04 18:27:22 PDT
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) 2005-10-04 22:44:55 PDT
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 [:bz] (still a bit busy) 2005-10-05 09:01:46 PDT
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) 2005-10-05 13:25:04 PDT
Brian, you're making a new patch here, right?
Comment 8 Brian Ryner (not reading) 2005-10-05 13:56:34 PDT
Comment on attachment 198383 [details] [diff] [review]
patch

yep, i'll be posting a new patch.
Comment 9 Brian Ryner (not reading) 2005-10-10 15:07:10 PDT
Created attachment 199106 [details] [diff] [review]
patch

Don't rely on a PresContext.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2005-10-10 16:35:38 PDT
Comment on attachment 199106 [details] [diff] [review]
patch

Most excellent.
Comment 11 Brian Ryner (not reading) 2005-10-10 17:18:39 PDT
checked in
Comment 12 Brian Ryner (not reading) 2005-10-10 17:20:00 PDT
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] 2005-10-11 16:49:08 PDT
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) 2005-10-12 12:18:47 PDT
(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] 2005-10-14 08:01:45 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.