Closed Bug 1162860 Opened 10 years ago Closed 10 years ago

On Firefox <38, tabs end up closing other tabs if they call window.close from onbeforeunload

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Version:
37 Branch
Type:
defect
Priority:
Not set
Severity:
normal
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 41
Iteration:
41.1 - May 25
Tracking Flags:
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 + fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: weiss, Assigned: Gijs)

References

Details

Attachments

(2 files, 1 obsolete file)

index.html and tab.html
624 bytes, application/zip
Details
MozReview Request: Bug 1162860 - ensure closing tabs really don't get closed twice, r?ttaubert
39 bytes, text/x-review-board-request
ttaubert
: review+
Details
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0 Build ID: 20150415140819 Steps to reproduce: i have 2 HTML files: "index.html" code: <!DOCTYPE html> <html> <head> <title>INDEX.HTML</title> </head> <body> <a href="./tab.html" target="_blank">new Tab</a> </body> </html> "tab.html" code: <!DOCTYPE html> <html> <head> <title>TAB.HTML</title> <script type="text/javascript"> function doFinish(){ window.close(); } </script> </head> <body onunload="/*doFinish();*/" onbeforeunload="doFinish();"> TEST Tab </body> </html> 1. I close all tabs. 2. I start only index.html - its sinlge tab. 3. I click on "new Tab": new tab opened. 4. I close new opened tab. Actual results: Both tabs closed. Chrome and IE dont have this BUG. Expected results: Only 1 Tab closed. "INDEX.HTML" dont be closed.
As master in these cases can you look on this issue?
Component: Untriaged → Document Navigation
Flags: needinfo?(gijskruitbosch+bugs)
I can't reproduce this on Nightly. How are you closing the tab? Are you doing this with a clean Firefox profile? Is the tab in the background or the foreground?
Flags: needinfo?(weiss)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(BernesB)
1. I start local WebServer with my 2 files from attached ZIP. 2. I open only 1 Tab(dont happend if any other tabs opened) and write adress http://127.0.0.1/fftest/ and press return. 3. Click on "new Tab" 4. new tab is in foreground and i click small "close tab" button. may be its bug only on FF 37?
Flags: needinfo?(weiss)
its happend too if i install actual Firefox on clear Windows XP on Virtualbox.
I can't reproduce it too. @ Juri Hahn - you can try testing this using latest Nightly ( https://nightly.mozilla.org/ )
Flags: needinfo?(BernesB) → needinfo?(weiss)
its dont happend on Nightly, but its BUG on FF 37.0.2
Flags: needinfo?(weiss)
I can reproduce on 37, and it looks like the reason it's fixed later may be somewhat coincidental... (I see JS errors in the console), so I'm going to poke at this some more. Boris... strange question perhaps, but is window.close meant to work from onbeforeunload?
status-firefox38: --- → unaffected
status-firefox38.0.5: --- → unaffected
status-firefox39: --- → unaffected
status-firefox40: --- → unaffected
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bzbarsky)
Summary: If opened only 2 Tabs, i can colse both if closing only second → On Firefox <38, tabs end up closing other tabs if they call window.close from onbeforeunload
If I have both of the tabs open and try to close Nightly, I see JS infinite loops in tabbrowser.xml... unless Core is allowing things to happen that it shouldn't be, this begins to seem like a frontend issue to me... Tim, CC'ing you per your previous work on some of this. If you have ideas, that'd be helpful.
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Iteration: --- → 40.3 - 11 May
Points: --- → 3
Component: Document Navigation → Tabbed Browser
Ever confirmed: true
Flags: needinfo?(gijskruitbosch+bugs) → firefox-backlog+
Product: Core → Firefox
From IRC: 09:55 Gijs aaaaaaaaargh 09:56 Gijs goodness do I hate XBL sometimes 09:56 Gijs ttaubert: so, wanna hear something fun? 09:56 Gijs ttaubert: remember how we do some magic to keep track of whether a tab is closed by keeping track of it in a "closing" field? :) 09:57 Gijs specifically http://mxr.mozilla.org/mozilla-cen...ase/content/tabbrowser.xml#2144 and various other checks 09:58 Gijs ttaubert: so "closing" is a XBL field 09:58 Gijs ttaubert: guess what happens when the tab gets removed from the DOM? That's right, we kill off the XBL field. I have a patch, but I want a test, too.
Attached file MozReview Request: bz://1162860/Gijs (obsolete) —
/r/8473 - Bug 1162860 - ensure closing tabs really don't get closed twice, r?ttaubert Pull down this commit: hg pull -r 95a77dd5fc11addab9e5c8a5666e36fdde074d99 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8604078 - Flags: review?(ttaubert)
> is window.close meant to work from onbeforeunload? Per spec, yes. Per implementations, who knows...
Flags: needinfo?(bzbarsky)
Comment on attachment 8604078 [details] MozReview Request: bz://1162860/Gijs /r/8473 - Bug 1162860 - ensure closing tabs really don't get closed twice, r?ttaubert Pull down this commit: hg pull -r cdbeb3132c4ab7908f4d1739ac6a0d755fdc24b1 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8604078 [details] MozReview Request: bz://1162860/Gijs https://reviewboard.mozilla.org/r/8471/#review7153 r=me with a better solution for waiting until the tab opened by window.load() has finished loading. ::: browser/base/content/tabbrowser.xml:2144 (Diff revision 1) > + debugger; Nit: leftover ::: browser/base/content/tabbrowser.xml:2178 (Diff revision 1) > + debugger; Nit: leftover ::: browser/base/content/tabbrowser.xml:5625 (Diff revision 1) > + Nit: please remove white space ::: browser/base/content/test/general/browser_tabs_close_beforeunload.js:12 (Diff revision 1) > + Nit: please remove white space ::: browser/base/content/test/general/browser_tabs_close_beforeunload.js:4 (Diff revision 1) > +add_task(function*() { Please use strict mode. ::: browser/base/content/test/general/browser_tabs_close_beforeunload.js:13 (Diff revision 1) > + if (secondTab.linkedBrowser.contentDocument.readyState !== "complete" || Can we access contentDocument.readyState without a CPOW? Can't we just call window.open() from the content task and listen for load events there to trigger when the task resolves? ::: browser/base/content/test/general/browser_tabs_close_beforeunload.js:22 (Diff revision 1) > + closeBtn.click(); Just to make sure, maybe add a check the .closing=true right after clicking the close button.
Attachment #8604078 - Flags: review?(ttaubert) → review+
(In reply to Not doing reviews right now from comment #11) > > is window.close meant to work from onbeforeunload? > > Per spec, yes. Per implementations, who knows... IE asks ("This website is trying to close the tab"). Funnily enough, IE asks even if you close the tab yourself... Chrome keeps hanging if I try to navigate away from the tab that gets opened by script while the devtools are opened. If I don't open the devtools, it doesn't let you close the tab. Seems to me like we should not let beforeunload close the tab if you navigate the page to somewhere else... I'll file a separate bug.
Flags: qe-verify?
Comment on attachment 8604078 [details] MozReview Request: bz://1162860/Gijs /r/8473 - Bug 1162860 - ensure closing tabs really don't get closed twice, r?ttaubert Pull down this commit: hg pull -r 18683889ca2c79cf05c3e7aceda407bb4d92f6c8 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8604078 - Flags: review+ → review?(ttaubert)
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite+
Comment on attachment 8604078 [details] MozReview Request: bz://1162860/Gijs https://reviewboard.mozilla.org/r/8471/#review7159 Ship It!
Attachment #8604078 - Flags: review?(ttaubert) → review+
Iteration: 40.3 - 11 May → 41.1 - May 25
https://hg.mozilla.org/mozilla-central/rev/544b8089044a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
FF 38 have problem with my example, if i close new tab first time: its ok. But if i open and close new tab second time its crashed...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Juri Hahn from comment #19) > FF 38 have problem with my example, if i close new tab first time: its ok. > But if i open and close new tab second time its crashed... This fix was checked in for Firefox 41, which is what "FIXED" means here, so please don't reopen the bug based on the behaviour in 38. I'll investigate and see if it makes sense to uplift this.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
status-firefox38: unaffected → ?
status-firefox38.0.5: unaffected → ?
status-firefox39: unaffected → ?
status-firefox40: unaffected → ?
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: --- → FIXED
Also, if the browser crashed, can you go to about:crashes and give us the link of the crashreport corresponding to that crash?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(weiss)
hm... Crashed... its not work for any time and than showing message: Script: chrome://browser/content/tabbrowser.xml:2216 its stoped to work and i can select: ignore and run, break this script or debug it...
Flags: needinfo?(weiss)
and if i select break(stop) script, both tabs are closed.
I can reproduce the issue in comment #19 if I unapply my patch. I'm going to request it be uplifted to Firefox 39 and 40; I don't think it's safe enough to uplift to 38.0.5. Firefox 39 is currently in beta ( https://beta.mozilla.org/ ) and should be released in a little over a month from now.
Comment on attachment 8604078 [details] MozReview Request: bz://1162860/Gijs Approval Request Comment [Feature/regressing bug #]: n/a -- issue with the browser not dealing well with tabs that close themselves while they are being closed [User impact if declined]: browser hangs / internal bookkeeping of tabbrowser gets very confused [Describe test coverage new/current, TreeHerder]: has a test! [Risks and why]: low. The fix is a correctness fix which really really shouldn't be breaking anything else, only fixing our keeping track of which tabs have already closed, so it's pretty safe. Just don't want this on 38.0.5 ("release") because there's almost 0 time to shake out any issues. It *has* baked on Nightly for a bit, and I went through issues filed in the last week that mention "tab", and saw nothing relevant to tabbrowser behaviour / this bug. [String/UUID change made/needed]: no.
Attachment #8604078 - Flags: approval-mozilla-beta?
Attachment #8604078 - Flags: approval-mozilla-aurora?
Comment on attachment 8604078 [details] MozReview Request: bz://1162860/Gijs Approved for uplift to aurora (40) and beta (39). This has had some time on Nightly without obvious problems.
Attachment #8604078 - Flags: approval-mozilla-beta?
Attachment #8604078 - Flags: approval-mozilla-beta+
Attachment #8604078 - Flags: approval-mozilla-aurora?
Attachment #8604078 - Flags: approval-mozilla-aurora+
Andrei, this is one I'd love to see tested for the test build of 39.0b1 build 1 which should show up for your team tomorrow. If you have time from your testing of 38.0.5! Thanks.
Flags: needinfo?(andrei.vaida)
Backed out for browser_tabs_close_beforeunload.js timeouts. https://hg.mozilla.org/releases/mozilla-beta/rev/10093acd851e This initially hit a lot of problems due to Beta39 not having all the necessary BrowserTestUtils functionality the new tests use. I backported what I could and got those cleared up, but in the end, it was still timing out and I had to wave the white flag :(
status-firefox39: fixed → affected
Flags: needinfo?(gijskruitbosch+bugs)
(Clearing Andrei's needinfo because this didn't make b1) Tim, it seems like sessionstore:update never "isFinal" on beta because beta doesn't have bug 1109875. Is that safe to uplift at all? And if not, I guess we should remove removeTab from BrowserTestUtils, and reimplement 'wait for the tab to close' some other way in the test?
Flags: needinfo?(ttaubert)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(andrei.vaida)
I think it would be fine to just replace the |yield removeTab()| with simple gBrowser.removeTab() calls. Closing tabs is sync on Beta so that should just work.
Flags: needinfo?(ttaubert)
Tracking in case you end up trying to uplift again.
tracking-firefox39: --- → +
Yes, I need to make a branch patch and land it. Not hard, need to just not forget. Will try to get to this Tuesday (not here tomorrow or Monday)
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1167896
Attachment #8604078 - Attachment is obsolete: true
Attachment #8620253 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: