Closed Bug 369955 Opened 15 years ago Closed 6 years ago

onbeforeunload is fired twice on Quit (but not on Close)

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: mats, Unassigned)

References

()

Details

(Keywords: testcase)

Attachments

(5 files, 3 obsolete files)

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
OS: Linux → All
Hardware: PC → All
The bug also occurs in fullscreen mode.
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Assignee: events → mats.palmgren
Attached patch Patch rev. 1 (diff -w) (obsolete) — Splinter Review
This also fixes bug 305085.
Attachment #254755 - Flags: review?(neil)
Blocks: 305085
No longer depends on: 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

Attachment #254755 - Flags: review?(neil)
(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)
Attached patch wip (obsolete) — Splinter Review
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.
Attached file Testcase #3
Attached file Testcase #4
Attached patch Patch rev. 2Splinter Review
Attachment #254754 - Attachment is obsolete: true
Attachment #254755 - Attachment is obsolete: true
Attachment #254958 - Attachment is obsolete: true
This patch takes care of re-enabling the onbeforeunload for all involved
content viewers if the user veto'd the unload.
Attachment #255014 - Flags: review?(neil)
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.
Attachment #255014 - Flags: review?(neil) → review-
QA Contact: ian → events
Reproduced on Vista32 with Firefox 3.5.x and 3.6.x using Ctrl-W key.
Reproduce with Firefox 4.0.beta6 too.
See Also: → 531199
See Also: → 513566
See Also: → 448493
Assignee: matspal → nobody
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?
Flags: needinfo?(alexis_wilke)
(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.
Flags: needinfo?(alexis_wilke)
Because this seems to be working in Firefox, and should now also work in e10s mode ( bug 967873), closing as worksforme.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.