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)
Firefox
Tabbed Browser
Tracking
()
Tracking | Status | |
---|---|---|
firefox38 | --- | verified |
People
(Reporter: nospam.mozilla, Assigned: Gijs)
References
Details
Attachments
(3 files, 1 obsolete file)
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.
Comment 2•19 years ago
|
||
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".
Updated•18 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Updated•18 years ago
|
Comment 3•18 years ago
|
||
![]() |
||
Comment 4•14 years ago
|
||
Is this still an issue? I recall some changes in the last year or so to prevent this exact sort of thing.
Comment 5•14 years ago
|
||
The problem still occurs in: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b6pre) Gecko/20100907 Firefox/4.0b6pre
Comment 6•13 years ago
|
||
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.
Comment 10•11 years ago
|
||
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>
Comment 11•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
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).
Assignee | ||
Updated•10 years ago
|
Points: --- → 5
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+
Assignee | ||
Comment 14•10 years ago
|
||
/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 | ||
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 37.1
Flags: in-testsuite? → in-testsuite+
Updated•10 years ago
|
Iteration: 37.1 → 37.2
Comment 15•10 years ago
|
||
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
Updated•10 years ago
|
QA Contact: florin.mezei
Comment 16•10 years ago
|
||
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
Comment 17•10 years ago
|
||
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.
Comment 18•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8532344 -
Flags: review?(ttaubert)
Assignee | ||
Comment 20•10 years ago
|
||
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. :-)
Assignee | ||
Updated•10 years ago
|
Attachment #8532344 -
Flags: review?(jaws)
Assignee | ||
Comment 21•10 years ago
|
||
/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
Assignee | ||
Comment 22•10 years ago
|
||
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.)
Assignee | ||
Comment 23•10 years ago
|
||
remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d64d54355f44
Updated•10 years ago
|
Iteration: 37.2 → 37.3
Updated•10 years ago
|
Attachment #8532344 -
Flags: review?(jaws) → review+
Comment 24•10 years ago
|
||
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.
Assignee | ||
Comment 25•10 years ago
|
||
(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)
Comment 26•10 years ago
|
||
(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)
Assignee | ||
Comment 27•10 years ago
|
||
(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)
Assignee | ||
Comment 29•10 years ago
|
||
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]
Assignee | ||
Comment 30•10 years ago
|
||
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]
Updated•10 years ago
|
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
Assignee | ||
Comment 31•10 years ago
|
||
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
Assignee | ||
Comment 32•10 years ago
|
||
take #2: remote: https://hg.mozilla.org/integration/fx-team/rev/e0c6c4e136ee remote: https://hg.mozilla.org/integration/fx-team/rev/458431092bd3
Comment 33•10 years ago
|
||
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
Comment 34•10 years ago
|
||
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)
Assignee | ||
Comment 35•10 years ago
|
||
(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)
Comment 36•10 years ago
|
||
(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
Assignee | ||
Comment 37•9 years ago
|
||
Attachment #8532344 -
Attachment is obsolete: true
Attachment #8617974 -
Flags: review+
Attachment #8617975 -
Flags: review+
Assignee | ||
Comment 38•9 years ago
|
||
Assignee | ||
Comment 39•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•