onbeforeunload is fired twice on Quit (but not on Close). This may be related to bug 305085, so marking dependent for now. STEPS TO REPRODUCE 1. load attachment 176135 [details] (testcase from bug 284596) 2. quit the application using CTRL+Q or File->Quit ACTUAL RESULT onbeforeunload is fired twice EXPECTED RESULT onbeforeunload is fired once PLATFORMS AND BUILDS TESTED Bug occurs in SeaMonkey 20070209 Linux Bug occurs in Firefox 20070206 Linux Bug occurs in Firefox 2.0.0.1 Linux Bug occurs in Firefox 1.5.0.9 Linux ADDITIONAL INFORMATION Clicking the 'x' in the upper left corner of the window or using CTRL+W or File->Close works as expected - onbeforeunload is fired once.

WFM Seamonkey 1.1 Linux(X11; U; Linux i686; en-US; rv:1.8.1.2pre) Gecko/20070121 Dialog does NOT appear twice here. GTK 1.2 0.9.1 Linux 2.4.32, buildconfig: target i686-pc-linux-gnu Build tools Compiler Version Compiler flags gcc gcc version 2.95.3 20010315 (release) -Wall -W -Wno-unused -Wpointer-arith -Wcast-align -Wno-long-long -pedantic -pthread -pipe c++ gcc version 2.95.3 20010315 (release) -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -pthread -pipe -I/usr/X11R6/include Configure arguments --with-system-jpeg=/usr/lib/ --with-system-zlib=/usr/lib/ --with-system-png=/usr/lib/ --enable-application=suite --enable-default-toolkit=gtk --disable-xprint --enable-crypto --disable-jsd --disable-activex --disable-activex-scripting --disable-tests --enable-optimize=-O2 --disable-debug --disable-venkman --disable-xprint --disable-gnomevfs --with-gtk-prefix=/usr/local/lib --enable-image-decoders=all --disable-gtktest --disable-glibtest --disable-xft --disable-freetype2 --enable-reorder --enable-strip --enable-elf-dynstr-gc --enable-svg --enable-svg-renderer-cairo

Sorry for comment #1, NOT WFM on Quit, but works for close. Should have read the description thorougly...

Bug also occurs in SeaMonkey 2007020305 on MacOSX Bug also occurs in SeaMonkey 2007020909 on Windows Vista

The bug also occurs in fullscreen mode.

This also fixes bug 305085.

(In reply to comment #6) >This also fixes bug 305085. I don't see how; that bug is caused by the original close() calling PermitUnload followed by removeTab() calling PermitUnload. Perhaps PermitUnload should be written to only pose the dialog once per load?

(In reply to comment #7) > (In reply to comment #6) > >This also fixes bug 305085. > I don't see how; that bug is caused by the original close() calling > PermitUnload followed by removeTab() calling PermitUnload. Right, I was testing in SeaMonkey and what I saw was a third problem (that was fixed by the patch). I've attached the corrected testcase in bug 305085 now and it shows 3 dialogs in SeaMonkey! (the problem being that SM loads about:blank in removeTab) > Perhaps PermitUnload should be written to only pose the dialog once per load? Yeah, that's probably better, if it matches what IE does. http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeunload.asp

(In reply to comment #8) >Yeah, that's probably better, if it matches what IE does. Unfortunately it doesn't, I wrote a quick test.

Comment on attachment 254755 [details] [diff] [review] Patch rev. 1 (diff -w) >+ const CI = Components.interfaces; I'm not a CI fan but I see it's already crept in elsewhere :-/ >+ // We did permitUnload() in window.tryToClose() so we need to suppress Perhaps it might make more sense for the close handler to call permitUnload rather than suppressing depending on whether tryToClose exists or not? >+ if (mPermitUnloadSuppressed > 0 || !mDocument) { Don't need to check for > 0 as it's unsigned i.e. if (mPermitUnloadSuppressed || !mDocument)

Here's a rough sketch of a different approach that seems to work for the testcases I have. The idea is to "arm" the 'onbeforeunload' dispatch in DocumentViewerImpl::LoadStart and keep the current hooks we have. It needs more work but do you think it could work? BTW, do you have more testcases you can attach?

Comment on attachment 254958 [details] [diff] [review] wip >+ // Hmm, should we really fire onbeforeunload for subwindows if it was veto'ed above? >+ >+ nsCOMPtr<nsIDocShellTreeNode> docShellNode(do_QueryReferent(mContainer)); > > if (docShellNode) { > PRInt32 childCount; > docShellNode->GetChildCount(&childCount); > > for (PRInt32 i = 0; i < childCount && *aPermitUnload; ++i) { Note the extra loop termination test here: ^^^ My idea about arming the event doesn't match IE; IE always askes all the beforeunloadhandlers again even if you OKed them the first time around.

This patch takes care of re-enabling the onbeforeunload for all involved content viewers if the user veto'd the unload.

Comment on attachment 255014 [details] [diff] [review] Patch rev. 2 (diff -w) >+ attribute boolean enableOnbeforeunloadEvent; This is an internal variable, not only does it not need to be exposed as an attribute but you should not be calling the methods internally. >+ void setEnableOnbeforeunloadEventRecursive(in boolean aEnable); Since you only ever pass in PR_TRUE perhaps this should be named void reenableOnBeforeUnloadEvents(); > NS_IMETHODIMP >+DocumentViewerImpl::SetEnableOnbeforeunloadEventRecursive(PRBool aEnable) >+{ >+ SetEnableOnbeforeunloadEvent(aEnable); >+ >+ nsCOMPtr<nsIDocShellTreeNode> docShellNode(do_QueryReferent(mContainer)); >+ if (docShellNode) { >+ PRInt32 childCount; >+ docShellNode->GetChildCount(&childCount); >+ for (PRInt32 i = 0; i < childCount; ++i) { >+ nsCOMPtr<nsIDocShellTreeItem> item; >+ docShellNode->GetChildAt(i, getter_AddRefs(item)); >+ nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(item)); >+ if (docShell) { >+ nsCOMPtr<nsIContentViewer> cv; >+ docShell->GetContentViewer(getter_AddRefs(cv)); >+ if (cv) { >+ cv->SetEnableOnbeforeunloadEventRecursive(aEnable); >+ } >+ } >+ } >+ } >+ return NS_OK; >+} I think we can optimise this. If you called permitUnload on a document viewer, and one of its child document viewer's onbeforeunload event is enabled, then all of its descendants must be enabled too. This means that on entry you can check to see if you are enabled in which case you can assume that nobody got around to calling permitUnload on you or your children. >+ // Unload was veto'd by the user => re-enable the 'onbeforeunload' >+ // event for future PermitUnload() calls. >+ SetEnableOnbeforeunloadEvent(PR_TRUE); Perhaps the caller should be doing this. Thoughts? >- for (var i = 0; reallyClose && i < numtabs; ++i) { >+ var i = 0; >+ for ( ; reallyClose && i < numtabs; ++i) { > var ds = browser.getBrowserForTab(cn[i]).docShell; > > if (ds.contentViewer && !ds.contentViewer.permitUnload()) > reallyClose = false; > } >+ if (!reallyClose) { >+ for (var j = 0; j < i; ++j) { >+ // The user has veto'd the unload in an 'onbeforeunload' handler. >+ // Re-enable 'onbeforeunload' for the content viewers that did >+ // not veto the unload. >+ var ds = browser.getBrowserForTab(cn[j]).docShell; >+ if (ds.contentViewer) >+ ds.contentViewer.setEnableOnbeforeunloadEventRecursive(true); >+ } >+ } It occurs to me we can optimise this by calling permitUnload on the top XUL content viewer which will then automatically recurse through all tabs.

Reproduced on Vista32 with Firefox 3.5.x and 3.6.x using Ctrl-W key.

Reproduce with Firefox 4.0.beta6 too.

The event is fired twice on Windows 7 only if Aero is disabled. If Aero is enabled, the event fires only once. Tested on Firefox 16 and 17.

I'm not seeing this on Windows 8 with Nightly from 2013-07-15 (25.0a1). Can anyone confirm that this is still an issue?

I confirm, that the error still exists in Firefox 22.0 on Windows 7 x64. Note that you have to disable aero to see the bug.

I use both browsers, SeaMonkey and FireFox, and it still happens to me too. I'm on Ubuntu 13.04, although SeaMonkey is always the latest.

This should be fixed in the latest Nightly 38 with bug 305085. However I was unable to reproduce the original issue using Firefox 35.0.1 or 36. Alexis, could you try the latest Firefox Nightly 38 and see if it still reproduces for you?

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #25) > This should be fixed in the latest Nightly 38 with bug 305085. However I was > unable to reproduce the original issue using Firefox 35.0.1 or 36. > > Alexis, could you try the latest Firefox Nightly 38 and see if it still > reproduces for you? Note that if you test this, you'll need to turn off e10s when testing (it's the top option in the preferences' general pane). Thanks for checking!

That worked, with E10S turned off as mentioned by Gijs. With E10S turned on, it does not ask anything. I tested my code on SeaMonkey 2.25 and that fails as expected with the old code. Thank you for finally seriously addressing this problem.

Because this seems to be working in Firefox, and should now also work in e10s mode ( bug 967873), closing as worksforme.