Closed Bug 130719 Opened 18 years ago Closed 17 years ago
Unload doesn't permit alerts()'s when window is closed
Browser, not engine ---> Event Handling. Summary sounds like bug 6354, "onunload doesn't fire when a window is closed"
Assignee: rogerl → joki
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...
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
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
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...
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. firstname.lastname@example.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.
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? email@example.com /be
Attachment #95961 - Flags: superreview+
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: 17 years ago
Resolution: --- → FIXED
right, 1.0 branch. posthumus adt1.0.+. Replacing "mozilla1.0.1+" with "fixed1.0.1" per Comment #30 From firstname.lastname@example.org.
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
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.