Closed Bug 305085 Opened 19 years ago Closed 10 years ago

onbeforeunload shows "Do you really want to close"-message twice

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 38
Iteration:
38.1 - 26 Jan
Tracking Flags:
Tracking Status
firefox38 --- verified

People

(Reporter: nospam.mozilla, Assigned: Gijs)

References

Details

Attachments

(3 files, 1 obsolete file)

Testcase #1
362 bytes, text/html
Details
MozReview Request: Bug 305085 - fix permitUnload usages in tab/window closing so we never show a permitUnload twice for the same page, r?jaws
39 bytes, text/x-review-board-request
jaws
: review+
Details
MozReview Request: Bug 305085 - add a test for beforeunload uses, r?jaws
39 bytes, text/x-review-board-request
jaws
: review+
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.10) Gecko/20050717 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.10) Gecko/20050717 Firefox/1.0.6

When window.onbeforeunload is set, the "Do you really want to close"-message is
shown twice, if more than one tab is opened and the close is initiated by
window.close().

If there is no second tab or if the tab is not closed by window.close(), only
one message is shown (as expected).

Reproducible: Always

Steps to Reproduce:
1. Open a html page containing the following code:
<html>
<body>
<a href="javascript: window.close()">Close</a>
<script language="javascript" type="text/javascript">
  <!--
  window.onbeforeunload = function() {
    return "Test";
  }
  -->
</script>
</body>
</html>

2. Open a second tab (may be empty)
3. Click on the "Close"-link

Actual Results:  
The "Do you really want to close"-message is shown twice.

Expected Results:  
The "Do you really want to close"-message should be shown once.
To reproduce this, the window must be opened by javascript. (Otherwise
window.close() is forbidden)

Example page that opens the test page:
<html>
<body>
<script language="javascript" type="text/javascript">
  <!--
  window.open("test.html", "_blank");
  -->
</script>
</body>
</html>

So the complete steps to Reproduce are:
1. Create the page shown here and name it "test2.html"
2. Create the page shown in the bug report and name it "test.html"
3. Open "test2.html" (This will open "test.html" in a new window)
4. Open a new tab in the "test.html"-window (may be empty)
5. Click on the "Close" link.
6. The "Do you really want to close"-message comes up. Press OK.
7. The "Do you really want to close"-message comes up a second time.
If you are really annoyed with this, disable JavaScript.  To do this, go to tools>options.  Then go to the content section and clear the checkbox marked "Enable JavaScript".
Blocks: 369955
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
No longer blocks: 369955
Depends on: 369955
Attached file Testcase #1 — 

    
    

    
  
Is this still an issue?  I recall some changes in the last year or so to prevent this exact sort of thing.

    
    

    
  
The problem still occurs in:  Mozilla/5.0 (X11; Linux x86_64; rv:2.0b6pre) Gecko/20100907 Firefox/4.0b6pre

    
    

    
  
I still experience this problem in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1.

However, it occurs in the following scenario:
1. Install the Tab Mix Plus add-on.
2. Enable the option to display a "Close tab" button on all tabs.
3. Open only one tab that has an "onbeforeunload" handler in place.
4. Click the X on that tab.

    
    

    
  
The testcase from comment 3 still applies even after Bug 636374.

    
    

    
  
This issue just started happening for me. Wrote the following code to test.

<!DOCTYPE html>
<html>
<head>
<script>

function confirmExit()
{
	return "Warning!!!.";
}

function InitApp()
{
	window.onbeforeunload = confirmExit;
}

</script>
</head>
<body onload="InitApp();">
<div id="myDiv"><h2>Unload test</h2></div>
</body>
</html>

    
    

    
  
Reproduced on the current Firefox 29.0.1 on Mac OS X 10.9, but not always.


When closing a single tab using Command+W or menu File, Close Tab:

- Apparently only occurs when NOT having another tab open in the same Firefox window. 

- Even having an empty tab open in the same window, does not give me the double confirmation. But having other Firefox windows open does not seem to affect this.


When closing the full current window using Shift+Command+W or menu File, Close Window:

- If the tab that uses "onbeforeunload" has focus: no problem, regardless of having other tabs open in the same window.

- If another tab has focus when closing the window, then the same double confirmation problem occurs.


When quitting Firefox using Command+Q:

- No problems, no matter which window or tab has focus.


(Tested with typing an answer on any Stack Exchange website, which currently does not have a workaround implemented for this bug. That might change: http://meta.stackexchange.com/questions/232177/add-workaround-for-old-firefox-beforeunload-bug-that-makes-it-prompt-twice)

    
    

    
  
This still reproduces in the latest Firefox 35 Nightly (BuildID=20141013030202) precisely as Arjan describes in comment 11:
- dialog displays twice (if "Leave" option is selected) when attempting to close the only remaining tab in the window
- dialog displays twice when focus is not on the tab that uses "onbeforeunload", and Ctrl+Shift+W is used to close the window
- dialog displays once as expected when attempting to close the window from the "x" button

To trigger the dialog, the following page can be used: http://dolske.net/mozilla/tests/prompt/onbeforeunload.html.

Confirmed this issue on Win 7 x64, Mac OS X 10.9.5, Ubuntu 13.04 x64.

A couple of notes regarding this issue:
- the issue has been there ever since Firefox 4 (to reproduce it: open Firefox -> keep only 1 tab -> load "http://dolske.net/mozilla/tests/prompt/onbeforeunload.html" -> press Ctrl+W -> click on "Leave Page")
- a side-effect of this issue is that you potentially can no longer close the tab (and window) using Ctrl+W, using the following scenario: open Firefox -> keep only 1 tab -> load "http://dolske.net/mozilla/tests/prompt/onbeforeunload.html" -> press Ctrl+W -> click on "Leave Page" -> click on "Stay on Page" ====> at this point the dialog is dismissed, however using Ctrl+W or the tab's "x" button will no longer do anything, as the dialog will not show, and the tab (and window) will not close (unles the window's "x" button is pressed, which works as expected).

    
  
Points: --- → 5
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+

    
    

    
  

      
      
      
      

        Attached file
          
        MozReview Request: bz://305085/Gijs (obsolete)
        — 
      

    


  

        Attachment #8532344 -
        Flags: review?(ttaubert)

    
    

    
  
/r/1159 - Bug 305085 - add a test for beforeunload uses, r?ttaubert
/r/1161 - Bug 305085 - fix permitUnload usages in tab/window closing so we never show a permitUnload twice for the same page, r?dao

Pull down these commits:

hg pull review -r 9166954611787163ec4fe31304603f5c9b5200e7

    
  
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 37.1
Flags: in-testsuite? → in-testsuite+
Iteration: 37.1 → 37.2

    
    

    
  
I can confirm the bug and BTW
The tab with "window.onbeforeunload" should be the last tab on the window and you should close the tab not the window.

With the test case below if I click on leave page on the first message but stay on page on the second one than I can't close any more that tab or other tabs opened after that. The only way is to restart firefox.

I tested the issue on Ubuntu and on Windows with firefox 34.

<html>
<head></head>
<body>
  <script  type="text/javascript">
  window.onbeforeunload = function() {
    return "Are you absolutely sure you want to leave?\nAny unsaved changes will be lost!";
  };
  </script>
</body>
</html>

To me it seams like the event is fired once becouse the tab is being closed and because it is the last tab it is being fired again becouse the window is going to be closed
QA Contact: florin.mezei

    
    

    
  
https://reviewboard.mozilla.org/r/1159/#review857

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +  Services.obs.removeObserver(onTabModalDialogLoaded, "tabmodal-dialog-loaded")

Nit: missing semicolon

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +  let deferred = Promise.defer();

Please use |new Promise(...|

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +function openAndLoadWindow(aOptions, aWaitForDelayedStartup=false) {

aOptions is always {} and aWaitForDelayedStartup is always true, maybe just remove those arguments?

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +var expectingDialog = false;

nit: let

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +  node.Dialog.ui.button0.click();

Can you please add a comment here that says which button is clicked, or which action expected?

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +  let deferred = Promise.defer();

Please use |new Promise(...|

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +    win.addEventListener("load", function onLoad() {

This branch isn't used.

::: browser/base/content/test/general/file_double_close_tab.html
(Diff revision 1)
> -    <title>Test for bug 1050638 - clicking tab close button twice should close tab even in beforeunload case</title>
> +    <title>Test for bug 1050638 and bug 305085 - clicking tab close button twice should close tab even in beforeunload case</title>

Maybe split the title in two? The description doesn't really fit for bug 305085.

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +  if (expectingDialog) {

Nit: Could do without the if ().

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +  win.addEventListener("unload", function onunload(e) {

This is probably from general/head.js but using the "unload" event feels wrong to me. How about using the "domwindowclosed" notification?

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/head.js#506

    
    

    
  
https://reviewboard.mozilla.org/r/1159/#review867

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +  ok(newWin.closed, "Window should be closed.");

Should add ok(!expectingDialog) to make sure there was a dialog.

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 1)
> +  ok(newWin.closed, "Window should be closed.");

Should add ok(!expectingDialog) to make sure there was a dialog.

    
    

    
  
https://reviewboard.mozilla.org/r/1161/#review865

::: browser/base/content/browser.js
(Diff revision 1)
> -    if (ds.contentViewer && !ds.contentViewer.permitUnload())
> +    if (ds.contentViewer && !ds.contentViewer.permitUnload(true)) {

Can you please add a comment saying what |true| means here? Maybe just say that all subsequent calls to permitUnload() for the current browser/document will be ignored when that returns true.

::: browser/base/content/browser.js
(Diff revision 1)
> +      window.getInterface(Ci.nsIDocShell).contentViewer.resetCloseWindow();

Can you please explain why we reset all "kids"? Might be good to mention that this basically re-allows onbeforeunload dialogs triggered by calling permitUnload() for all browsers/documents that were iterated before. Because we're not closing the window. Something like that :)

::: browser/base/content/tabbrowser.xml
(Diff revision 1)
> +            if (!aTab._pendingPermitUnload && !aTabWillBeMoved) {

Not sure I understand this change. By moving this after the block dealing with closing a window with the last tab we will always omit the onbeforeunload dialog, no?

        Attachment #8532344 -
        Flags: review?(ttaubert)

    
    

    
  
https://reviewboard.mozilla.org/r/1161/#review1011

::: browser/base/content/tabbrowser.xml
(Diff revision 1)
> +            if (!aTab._pendingPermitUnload && !aTabWillBeMoved) {

No, because closeWindow will itself call window.close(), which will call WindowIsClosing which calls permitUnload, as per the above. :-)

    
  

        Attachment #8532344 -
        Flags: review?(jaws)

    
    

    
  
/r/1159 - Bug 305085 - add a test for beforeunload uses, r?jaws
/r/1161 - Bug 305085 - fix permitUnload usages in tab/window closing so we never show a permitUnload twice for the same page, r?jaws

Pull down these commits:

hg pull review -r 29d6d43e3ee8d831b3debc97a1e12c422e5c0bb8

    
    

    
  
Jared, redirecting to you because Tim is on PTO until January. I've mostly made changes as requested, except that I've just removed the implementations of closing windows and/or opening them, as they existed in head.js already. I've not updated those because I imagine other consumers may or may not break when using the other Promise implementation (not unheard of). I can file a followup bug for it if required, but I actually thought we were sticking with the current implementation until DOM Promises were further improved in terms of debugging support and whatnot...

(FWIW, I don't understand how to look at the old comments wrt the new code in mozreview - they don't show up for me. I don't know why.)
Iteration: 37.2 → 37.3

        Attachment #8532344 -
        Flags: review?(jaws) → review+

    
    

    
  
https://reviewboard.mozilla.org/r/1157/#review1079

::: browser/base/content/browser.js
(Diff revision 2)
> -    if (ds.contentViewer && !ds.contentViewer.permitUnload())
> +    // Passing true to permitUnload indicate we plan on closing the window.

s/indicate/indicates/

::: browser/base/content/tabbrowser.xml
(Diff revision 2)
> +                // call before permitUnload() even returned.

s/even//

::: browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
(Diff revision 2)
> +const TEST_PAGE = "http://mochi.test:8888/browser/browser/base/content/test/general/file_double_close_tab.html";

This should have a license header.

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
> (Diff revision 2)
> > +const TEST_PAGE = "http://mochi.test:8888/browser/browser/base/content/test/general/file_double_close_tab.html";
> 
> This should have a license header.

I didn't think tests needed a license header, they are public domain by default, right?
Flags: needinfo?(jaws)

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #25)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> > browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
> > (Diff revision 2)
> > > +const TEST_PAGE = "http://mochi.test:8888/browser/browser/base/content/test/general/file_double_close_tab.html";
> > 
> > This should have a license header.
> 
> I didn't think tests needed a license header, they are public domain by
> default, right?

There is a mix of tests that are missing license headers, then those that have Public License or MPL headers (small minority).

https://bugzilla.mozilla.org/show_bug.cgi?id=665580#c15 for reference if it helps ;)
Flags: needinfo?(jaws)

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> (In reply to :Gijs Kruitbosch from comment #25)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> > > browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
> > > (Diff revision 2)
> > > > +const TEST_PAGE = "http://mochi.test:8888/browser/browser/base/content/test/general/file_double_close_tab.html";
> > > 
> > > This should have a license header.
> > 
> > I didn't think tests needed a license header, they are public domain by
> > default, right?
> 
> There is a mix of tests that are missing license headers, then those that
> have Public License or MPL headers (small minority).
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=665580#c15 for reference if it
> helps ;)

But this policy change was much more recent:

http://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/

Did I miss something else?
Flags: needinfo?(jaws)

    
    

    
  
Fair enough, carry on without the header then :)
Flags: needinfo?(jaws)

    
    

    
  
With the comments fixed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/c8f1dab5c9e5
remote:   https://hg.mozilla.org/integration/fx-team/rev/618a0ea01af9

For QA: AIUI this might also fix bug 369955, but I'm not sure. Please check. :-)
Whiteboard: [fixed-in-fx-team]

    
    

    
  
Backed out for m-bc1 orange in the test this adds.

remote:   https://hg.mozilla.org/integration/fx-team/rev/ea461c27bb0f
remote:   https://hg.mozilla.org/integration/fx-team/rev/cb25573de7e7


It's odd because I know I've run the test locally in the past and it passed, but (a) it doesn't now and (b) it doesn't on infra. I'll have to figure out how to fix it in the new year.
Whiteboard: [fixed-in-fx-team]
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan

    
    

    
  
Stupid, stupid, stupid Gijs. Test issue because the window close utility promise function I used in response to the review comments (from head.js) actually calls window.close, and this test wants a promise that the window *will be closed* - but wants to do it itself.

With some test adjustments (mostly per comment #16) which make it pass locally:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=b68da8162bca

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/e0c6c4e136ee
https://hg.mozilla.org/mozilla-central/rev/458431092bd3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38

    
    

    
  
The issue reproduced as stated in comment 12, using older versions like Firefox 35.0.1 or 36, on Windows 7x64, Mac OS X 10.9.5, Ubuntu 12.04 x86. The original issue (message showing twice) no longer reproduces with the latest Nightly 38 build (BuildID=20150128101733) on either of the 3 operating systems. All scenarios from comment 12 work fine now.

There are still 2 issues that appear:
1. The message does NOT show at all when e10s is enabled.
2. When there is only one tab with http://dolske.net/mozilla/tests/prompt/onbeforeunload.html open, if you choose to close that tab ("x" button or Ctrl+W) and choose to "Stay on Page", then you can no longer close that tab (clicking on "x" or using "Ctrl+W" will do nothing). Closing the window still works.

Gijs, i did not find anything filed for these two issues. Should I file them separately and close this bug?

Note that I've also tried the scenarios from bug 369955, but I could not reproduce the original issue with Firefox 35.0.1 or Firefox 36. Alexis Wilke reported a couple of weeks ago that he still sees this, so I've asked him to verify the fix on Nightly.
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #34)
> The issue reproduced as stated in comment 12, using older versions like
> Firefox 35.0.1 or 36, on Windows 7x64, Mac OS X 10.9.5, Ubuntu 12.04 x86.
> The original issue (message showing twice) no longer reproduces with the
> latest Nightly 38 build (BuildID=20150128101733) on either of the 3
> operating systems. All scenarios from comment 12 work fine now.
> 
> There are still 2 issues that appear:
> 1. The message does NOT show at all when e10s is enabled.

This is because of bug 967873. I've changed the summary to make it easier to find.

> 2. When there is only one tab with
> http://dolske.net/mozilla/tests/prompt/onbeforeunload.html open, if you
> choose to close that tab ("x" button or Ctrl+W) and choose to "Stay on
> Page", then you can no longer close that tab (clicking on "x" or using
> "Ctrl+W" will do nothing). Closing the window still works.

This sounds serious. Does this work on 35/36/37? Either way, new bug blocking this one, please, and CC me (and let me know there if this works correctly on 35/36/37). Also, when closing the window, does that cause the dialog to popup again or no?

> Gijs, i did not find anything filed for these two issues. Should I file them
> separately and close this bug?
> 
> Note that I've also tried the scenarios from bug 369955, but I could not
> reproduce the original issue with Firefox 35.0.1 or Firefox 36. Alexis Wilke
> reported a couple of weeks ago that he still sees this, so I've asked him to
> verify the fix on Nightly.

Great, I'll comment there to ensure he checks on non-e10s.
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #35)
> This sounds serious. Does this work on 35/36/37? Either way, new bug
> blocking this one, please, and CC me (and let me know there if this works
> correctly on 35/36/37).

Filed new bug 1127394. This scenario works correctly in 35/36/37, but only if you choose "Stay on Page" the first time... if you choose "Leave" on the first prompt and then "Stay" on the second one, then the issue also shows there (as reported in comment 12). So basically it's the same issue, it's just initially I thought it was just a side-effect of the dialog showing twice, but it seems it's something else.

> Also, when closing the window, does that cause the
> dialog to popup again or no?

Yes, closing or quitting both cause the dialog to show up (once) as expected.

Closing this bug since remaining issues are tracked separately.
Status: RESOLVED → VERIFIED

          status-firefox38:
          --- → verified

    
    

    
  

        Attachment #8532344 -
        Attachment is obsolete: true

        Attachment #8617974 -
        Flags: review+

        Attachment #8617975 -
        Flags: review+

    
  
Depends on: 1260594

    
  
Depends on: 1238032

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: