Closed Bug 130719 Opened 22 years ago Closed 22 years ago

onUnload doesn't permit alerts()'s when window is closed

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: OliverKitzing, Assigned: danm.moz)

References

(
URL
)

Details

(Whiteboard: [adt1 rtm] [ETA 08/20])

Attachments

(1 file, 2 obsolete files)

allow windows to open new windows while being destroyed
5.68 KB, patch
pavlov
: review+
brendan
: superreview+
chofmann
: approval+
Details | Diff | Splinter Review 
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.8) Gecko/20020204
BuildID:    2002020406

The onUnload-event doesn't fire when closing a window. It fires
if you leave the actual page via a link though.
(bug seen in Mozilla 0.98 and 0.99 on Windows2000 and Windows XP)

Every other known browser behaves as expected, including Netscape 6.2

(Really a bug or a "feature"?)



Reproducible: Always
Steps to Reproduce:
Just try the URL. Closing the child window doesn't fire an event as you
would expect.

Expected Results:  Fire the onUnload event. It's hard to write JavaScript-Code
depending
on events if they don't work properly...... obviously!
Browser, not engine ---> Event Handling. 

Summary sounds like bug 6354,                    
"onunload doesn't fire when a window is closed"
Assignee: rogerl → joki
Component: JavaScript Engine → Event Handling
QA Contact: pschwartau → madhur
Here is the HTML of the child window that comes up,
http://oliverkitzing.bei.t-online.de/diverses/window_OnUnloadBugTest.htm:


<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head><title>OnUnload-Bug-Test</title>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>

<body onUnload="alert('OnUnload Event has been fired')">
If you close this window, an alert should appear....
</body>

</html>
Confirming bug with Mozilla trunk builds 20020311xx on WinNT, Linux.
OS: WinXP ---> All.

Note we get this error in the JS Console when we close the child window:

Error: uncaught exception: [Exception... "Component returned failure code: 
0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowInternal.alert]"  nsresult: 
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: <unknown filename> :: 
onunload :: line 0"  data: no]


Don't know if this is Event Handling or not. This is a bad bug.
cc'ing jst, hyatt, sfraser -
Status: UNCONFIRMED → NEW
Ever confirmed: true
Is the problem that the unload handler doesn't fire, or is the problem that
alert() throws an exception when called from the unload handler? Someone could
test by using dump() in stead (you must turn on the pref that enables dump()
output to show up on the console, and you must run with -console to see the
dump() output).
Oops, could this be a feature that prevents unwanted alerts coming up?
Or does that require the user to set something in Edit > Preferences?

I have not blocked anything as a user in my builds...
OS: Windows XP → All
I'm thinking this could be due to us not letting the browser open new windows
from the onunload handler when a window is closed, and technically, an alert()
box is a window in mozilla, so that could be blocked. I'm not sure about this
though, someone would need to test.
jst looked at this with me and explained this is indeed a feature.
We prevent any window from being opened from the onUnload handler,
IF the onUnload event is fired from dismissing the window.

Note the onUnload event is also fired when you navigate from 
one site to another. In this case, we ALLOW windows to be opened
by the onUnload handler.


TO SEE THIS:

1. Load the testcase for this bug
2. A child window comes up saying,
  "If you close this window, an alert should appear....
3. DON'T close the child window!
4. Go to the parent window instead and bring up, say, http://www.mozilla.org
5. Drag a link from mozilla.org to the child window
6. This causes the child window to navigate to a new site
7. This causes the onUnload event of the child window to fire
8  The alert appears successfully: 'OnUnload Event has been fired'


Reassigning to danm, who will know more about this. The only question
here is, do we want to allow alert()'s as a special case? Again, this
pertains to onUnload events caused by dismissing a window.

Also cc'ing rginda for his work on bug 92955,
"Disable window.open from onload and onunload"
Assignee: joki → danm
Changing summary:

from     "onUnload doesn't fire when window is closed"
to       "onUnload doesn't permit alerts()'s when window is closed"


When I used dump() in the onUnload handler instead of alert(),
as jst suggested in Comment #4, I found that the onUnload event
was firing successfully in all cases.
Summary: onUnload doesn't fire when window is closed → onUnload doesn't permit alerts()'s when window is closed
We only prevent window.open from working on unload (or onload, setTimeout,
setInterval, top-level script, and N milliseconds after a mouse click) if the
user has enabled popup blocking.  This will also affect window.confirm and
window.prompt.

Do we need to special case chrome: urls, allowing them to open always?  Can
content open a chrome: url?

Some might consider blocking prompt, confirm, and alert an intended effect of
the pref, maybe this is a WONTFIX?
On second thought; Phil indicated that he didn't change any prefs, and the code
works when onload fires due to a navigation.  Maybe alert() is failing for
another reason.  I'll check in gdb.
I think this can't be WONTFIX, and I bet it's 4xp.  I'm not sure how important
it is to allow alert/confirm/prompt from onunload, though.

/be
Keywords: 4xp
I'd only hint at WONTFIX if this were related to the
dom.disable_open_during_load pref (which 4x doesn't have), but from what Phil
says, it doesn't appear to be.
I talked to danm about this and we had a quick look at the code. I'm fine with
mozilla not showing alert's from the unload handler when closing a window, but
currently calling alert() from the unload handler throws an exception which
causes the execution of the unload to fail. IMO that's a bug, and it's easy to
fix too. Danm knows how.
  Besides the pref to disallow opening windows from script Robert mentions in
comment 9, we also flat out disallow a dying window to spawn more windows as it
goes down. This legacy capability is badly abused by so many website authors and
besides, we don't handle it well (see bug 115969).
  Myself, I'm inclined to not fix this, despite the fact that legacy browsers
allowed it. Is there an unannoying use for posing alerts in an onunload handler
besides testing purposes? (A fair enough use, I have to admit...) We could
perhaps save ourselves the exception mentioned in comment 3 by not returning an
error from the window.alert function, as Johnny mentions just above. What say,
Brendan?
Yes, suppress the exception.  If you're going to fail, try to do it stealthily
so scripts, if not their authors, won't notice.  Sneak sneak sneak....

/be
You're right. This is a useful feature, but it should
be accessible via Edit->Preferences->Advanced->Script & Windows.

I noticed this behaviour for another reason... which has something
to do with access on variables in other windows via "opener. " and
included js-Files, where Mozilla behaves a little strange,
but i will file another report for this later on if it isn't included
in bugzilla already!

Bye,

Oliver Kitzing
QA Contact: madhur → rakeshmishra
Target Milestone: --- → mozilla1.2alpha
As mentioned above, this is a problem caused by the fix for bug 115969.
That's a crash when exiting the app while viewing a page that has an onunload
handler that wants to open a window. The app is muchly torn down by the time it
realizes it wants to open a new window and the ensuing crash is deep in the
bowels of Mozilla.
  Further ad-hoc testing indicates that this is a problem really only if the
app is shutting down. This patch modifies the 115969 patch to refuse to open a
new window only if we're the last window (and thus the app is in the midst of
trying to quit). Otherwise a dying window is now allowed to open a popup. Note
this is still under pref control! The end user can disallow a site from opening
popups from a window being closed (bug 75371). Without this patch that
capability couldn't actually be enabled despite the pref.
  Note this behaves a little strangely on a Mac OS9 build, where closing the
last window isn't a signal to quit the app. We'll still refuse; the last/only
window just has some unique behaviour on that platform.
  I think this is a decent solution, though I'm not sure how much help it gives
in practice, since most people seem to use just one window at a time. I think
my only chance to allow popups while closing a lone window would necessitate
firing the unonload handler earlier during window teardown so the new window
request could be dealt with before the app is quite so committed to quitting.
That's scary, but I'm considering it...
Keywords: nsbeta1+
Whiteboard: [adt1 rtm]
Blocks: 143047
Severity: normal → major
Whiteboard: [adt1 rtm] → [adt1 rtm] [ETA 08/20]
Taking QA Contact to myself, since I'm taking care of Events right now.
QA Contact: rakeshmishra → desale
Comment on attachment 95658 [details] [diff] [review]
reduce scope of refusal to open a new window

Dan, your patch seems very reasonable.	r=caillon if you move |PRBool more;|
into the if block where it's actually used (windowList).
Attachment #95658 - Flags: review+
Comment on attachment 95658 [details] [diff] [review]
reduce scope of refusal to open a new window

what caillon said, plus, the double newlines after opening braces for "then"
clauses seem a bit too spacey to me.  sr=brendan@mozilla.org with nits picked.

/be
Attachment #95658 - Flags: superreview+
After reflection, I don't much like the above patch. I'm working on something
better. But in the meantime, something worse:
Independent of the above patch, this patch puts control of whether to
disallow a window being destroyed from creating a new window under control of a
pref. Note that the other pref, the one with the UI (under Advanced -> Script &
Plugins) also continues to control this. Without this patch however, regardless
of the setting of that pref, a window being destroyed will never open a new
window. With this patch, and both prefs set false, a window being destroyed can
now open a new window.
  Note that the patch does not set the pref false; in fact assumes it's true.
Note that it's also only on the 1.0 branch. Note that with both prefs false,
bug 115969 is reinstated and Mozilla will crash.
Comment on attachment 95933 [details] [diff] [review]
put new-window-creation-during-destruction control under a pref

Eek (about the possible crash this can cause if the prefs are set "correctly"),
but sr=jst on the code change.
Attachment #95933 - Flags: superreview+
Comment on attachment 95933 [details] [diff] [review]
put new-window-creation-during-destruction control under a pref

r=saari, it is a desired behavior difference. That it crashes in one case is
really another bug.
Attachment #95933 - Flags: review+
Argh.

What are the chances of a real fix, danm?

/be
Brendan: pretty good.
  This patch removes the check disallowing a window being destroyed from
opening a new window (say in an onunload handler) from bug 115969, replacing it
with code that allows us to survive the new window. That is, the 115969 crash
is fixed without disallowing the situation. It does this by reordering things
that happen during shutdown and window closure so that the app is no longer
committed to exiting prematurely.
Attachment #95658 - Attachment is obsolete: true
Attachment #95933 - Attachment is obsolete: true
Comment on attachment 95961 [details] [diff] [review]
allow windows to open new windows while being destroyed

Could the comment you added to nsXULWindow::Destroy say something about
onunload and where (above) it is called indirectly?

sr=brendan@mozilla.org

/be
Attachment #95961 - Flags: superreview+
Keywords: mozilla1.0.1+
Comment on attachment 95961 [details] [diff] [review]
allow windows to open new windows while being destroyed

conditional a= chofmann for checkin on the 1.0.1 branch tonight assuming the
reviews go ok.
Attachment #95961 - Flags: approval+
Comment on attachment 95961 [details] [diff] [review]
allow windows to open new windows while being destroyed

r=pavlov
Attachment #95961 - Flags: review+
patch 3 is in, trunk and 1.0 branch. (1.0 branch, right?)
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
right, 1.0 branch. posthumus adt1.0.+.

Replacing "mozilla1.0.1+" with "fixed1.0.1" per Comment #30 From danm@netscape.com.
Keywords: mozilla1.0.1+ → adt1.0.1+, fixed1.0.1
I'm worried about this causing appservice->Quit() to not work...  That's a
problem for people who really want to quit; either via selecting it from the
menu; hotkey, or in kiosk/embedded uses such as ours.  It so happens that I
think wgate is mostly isolated from the issue (because the main user Exit button
is handled outside of the browser, and because we force confirmation on all
window.open's), but I think it might be an issue for other embedders.
Indeed, this fix caused the regression described by bug 163718 (or so it 
appears): QuickLaunch is broken.
re comment 32: yep, it caused bug 163710, where the quit command needs to be
issued twice on Mac OSes.
Also the cause of topcrash bug 163918?
Hmm, we backed out of this fix for 1.0.1 branch.  I wonder if we should remove
fixed1.0.1 keyword.

I do not know of the situation for the trunk.
The onunload event fires when closing the View Selection Source window for the
popup window at
http://oliverkitzing.bei.t-online.de/diverses/mozilla_onUnload_bug.htm though.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1b) Gecko/20020823
Keywords: fixed1.0.1
Whimper. Most cases work.
1-launch, load Page With Unload Handler That Opens New Window, close window.
2-launch, load PWUHTONW, quit.
3-launch, open new window, load PWUHTONW, quit from PWUHTONW window.
- but -
4-launch, load PWUHTONW, open new window, quit from new window -- doesn't work;
the new window flashes up and the app quits immediately. You'd think this
wouldn't be so hard.
*** Bug 166654 has been marked as a duplicate of this bug. ***
*** Bug 189770 has been marked as a duplicate of this bug. ***
See also bug 391834, which seeks to reverse this change (approximately) for security/annoyance reasons.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: