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

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
13 years ago
3 years ago

People

(Reporter: mats, Unassigned)

Tracking

({testcase})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(5 attachments, 3 obsolete attachments)

Reporter

Description

13 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

13 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

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

Comment 3

13 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

13 years ago
The bug also occurs in fullscreen mode.
Reporter

Comment 5

13 years ago
Posted patch Patch rev. 1 (obsolete) — Splinter Review
Reporter

Updated

13 years ago
Assignee: events → mats.palmgren
Reporter

Comment 6

13 years ago
Posted patch Patch rev. 1 (diff -w) (obsolete) — Splinter Review
This also fixes bug 305085.
Attachment #254755 - Flags: review?(neil)
Reporter

Updated

13 years ago
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?
Reporter

Comment 8

13 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

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

Comment 11

13 years ago
Posted 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.
Reporter

Comment 14

13 years ago
Posted file Testcase #3
Reporter

Comment 15

13 years ago
Posted file Testcase #4
Reporter

Comment 16

13 years ago
Posted patch Patch rev. 2Splinter Review
Attachment #254754 - Attachment is obsolete: true
Attachment #254755 - Attachment is obsolete: true
Attachment #254958 - Attachment is obsolete: true
Reporter

Comment 17

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

Comment 19

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

Comment 20

9 years ago
Reproduce with Firefox 4.0.beta6 too.

Updated

9 years ago
See Also: → 531199

Updated

9 years ago
See Also: → 513566

Updated

9 years ago
See Also: → 448493
Reporter

Updated

8 years ago
Assignee: matspal → nobody

Comment 21

7 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

6 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

6 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

5 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

5 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
Closed: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.