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

Status: RESOLVED WORKSFORME

(Core :: DOM: Events, defect)

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[beanladen]

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

Comment 2

[beanladen]

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

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Mats Palmgren (inactive)]

The bug also occurs in fullscreen mode.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -w)

This also fixes bug 305085.

Comment 7

[[e-mail]]

(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?

Comment 8

[Mats Palmgren (inactive)]

(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

Comment 9

[[e-mail]]

(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 10

[[e-mail]]

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)

Comment 11

[Mats Palmgren (inactive)]

Attachment: wip

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 12

[Mats Palmgren (inactive)]

Attachment: Testcase #2, child IFRAME w. onbeforeunload

Comment 13

[[e-mail]]

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.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 15

[Mats Palmgren (inactive)]

Attachment: Testcase #4

Comment 16

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 17

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

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

Comment 18

[[e-mail]]

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.

Comment 19

[Marek Sykora]

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

Comment 20

[Marek Sykora]

Reproduce with Firefox 4.0.beta6 too.

Comment 21

[pawlaktomek]

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.

Comment 22

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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?

Comment 23

[pawlaktomek]

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.

Comment 24

[Alexis Wilke]

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.

Comment 25

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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?

Comment 26

[:Gijs (he/him)]

(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!

Comment 27

[Alexis Wilke]

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.

Comment 28

[:Gijs (he/him)]

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