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

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
12 years ago
3 years ago

People

(Reporter: mats, Unassigned)

Tracking

({testcase})

Trunk
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
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

12 years ago
Sorry for comment #1, NOT WFM on Quit, but works for close. Should have read
the description thorougly...
(Reporter)

Comment 3

12 years ago
Bug also occurs in SeaMonkey 2007020305 on MacOSX
Bug also occurs in SeaMonkey 2007020909 on Windows Vista
OS: Linux → All
Hardware: PC → All
(Reporter)

Comment 4

12 years ago
The bug also occurs in fullscreen mode.
(Reporter)

Comment 5

12 years ago
Created attachment 254754 [details] [diff] [review]
Patch rev. 1
(Reporter)

Updated

12 years ago
Assignee: events → mats.palmgren
(Reporter)

Comment 6

12 years ago
Created attachment 254755 [details] [diff] [review]
Patch rev. 1 (diff -w)

This also fixes bug 305085.
Attachment #254755 - Flags: review?(neil)
(Reporter)

Updated

12 years ago
Blocks: 305085
No longer depends on: 305085

Comment 7

12 years ago
(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?
(Reporter)

Comment 8

12 years ago
(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

(Reporter)

Updated

12 years ago
Attachment #254755 - Flags: review?(neil)

Comment 9

12 years ago
(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

12 years ago
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)
(Reporter)

Comment 11

12 years ago
Created attachment 254958 [details] [diff] [review]
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?
(Reporter)

Comment 12

12 years ago
Created attachment 254962 [details]
Testcase #2, child IFRAME w. onbeforeunload

Comment 13

12 years ago
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.
(Reporter)

Comment 14

12 years ago
Created attachment 254986 [details]
Testcase #3
(Reporter)

Comment 15

12 years ago
Created attachment 254989 [details]
Testcase #4
(Reporter)

Comment 16

12 years ago
Created attachment 255013 [details] [diff] [review]
Patch rev. 2
Attachment #254754 - Attachment is obsolete: true
Attachment #254755 - Attachment is obsolete: true
Attachment #254958 - Attachment is obsolete: true
(Reporter)

Comment 17

12 years ago
Created attachment 255014 [details] [diff] [review]
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.
Attachment #255014 - Flags: review?(neil)

Comment 18

12 years ago
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

Comment 19

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

Comment 20

8 years ago
Reproduce with Firefox 4.0.beta6 too.

Updated

8 years ago
See Also: → bug 531199

Updated

8 years ago
See Also: → bug 513566

Updated

8 years ago
See Also: → bug 448493
(Reporter)

Updated

8 years ago
Assignee: matspal → nobody

Comment 21

6 years ago
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?

Comment 23

5 years ago
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

5 years ago
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)

Comment 26

4 years ago
(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

4 years ago
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)

Comment 28

3 years ago
Because this seems to be working in Firefox, and should now also work in e10s mode ( bug 967873), closing as worksforme.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.