1127394 - Cannot close single tab after choosing to remain on page (from onbeforeunload dialog)

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reproducible with: 
- Firefox 38 Nightly latest - BuildID: 20150128101733

NOT Reproducible with:
- Firefox 35.0.1
- Firefox 36 Beta latest
- Firefox 37 Aurora latest

Reproducible on: Windows 7 x64, Mac OS X 10.9.5, Ubuntu 12.04 x86

Steps to reproduce:
1. Open Firefox and go to http://dolske.net/mozilla/tests/prompt/onbeforeunload.html (make sure it's the only tab).
2. Chose to close the tab (click "x" or Ctrl+W).
3. In the prompt that appears click "Stay on Page".
4. After the dialog is dismissed, attempt to once again close the tab (click "x" or Ctrl+W).

Expected results:
The onbeforeunload dialog should once again display asking whether to leave or remain on the page.

Actual results:
Nothing happens.

Notes:
- this scenario used to work for previous versions (where bug 305085 is not yet fixed). However, there was a variation of this scenario where this issue was showing also for older versions: prior to the fix for 305085, in the scenario above we display the dialog 2 times, and if you click "Leave" and then "Stay" on the second prompt, then this bug also reproduces there.
- choosing to close the window still works as expected, as the dialog displays and works correctly
- trying this scenario with more tabs works fine... it's just the one remaining tab that causes this issue

Comment 1

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Note that currently this only works with e10s disabled (the dialog does not show when e10s is enabled, because of bug 967873).

Comment 2

[:Gijs (he/him)]

I can reproduce this. It's because _beginRemoveTab calls closeWindow calls window.close, and then *always* returns true - including if the close is cancelled. Unfortunately window.close doesn't tell you if that is the case... I'll look into whether we can determine that some other way or are just going to have to call WindowIsClosing() manually from the function we pass to closeWindow from _beginRemoveTab. The latter has the downside that we wouldn't catch add-ons that register close event handlers...

I *think* we might be able to figure this out by at least one of:
- checking window.closed
- adding a system event close listener that checks whether preventDefault has been called on the event
- looking if we can make window.close() return whether it worked or not (might not be spec-compatible...)

Comment 3

[:Gijs (he/him)]

I have a test for this, but I'd like Florin and/or QE to check if there aren't edgecases I've forgotten about.

Comment 4

[:Gijs (he/him)]

Attachment: should be able to close single tab through tabclose button even if we said 'stay on page' initially,

Jared, seeing as you did the 305085 reviews... also, sorry for the kludgy test, but... yeah. :-\

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8557272 [details] [diff] [review]
should be able to close single tab through tabclose button even if we said 'stay on page' initially,

Review of attachment 8557272 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/globalOverlay.js
@@ +32,2 @@
>      window.close();
> +    return window.closed;

Ok, so _windowIsClosing gets the wrong state in there and then any future attempts to close the window bail out early because tabbrowser thinks the window is closing already.

@@ +32,5 @@
>      window.close();
> +    return window.closed;
> +  }
> +
> +

nit, only one blank line here.

Comment 6

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Ok, so _windowIsClosing gets the wrong state in there and then any future
> attempts to close the window bail out early because tabbrowser thinks the
> window is closing already.

Yes, sorry for not doing a thorough explanation to go with the patch, but that's exactly right.

> @@ +32,5 @@
> >      window.close();
> > +    return window.closed;
> > +  }
> > +
> > +
> 
> nit, only one blank line here.

Done.

remote:   https://hg.mozilla.org/integration/fx-team/rev/0718406be39a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0718406be39a

Comment 8

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I can no longer reproduce the issue on Windows 7 x64, Mac OS X 10.8.5, Ubuntu 14.04 x64, using the latest Firefox 38 Nightly (BuildID=20150203062848). Everything works fine now when choosing to leave or stay.