Closed Bug 1234936 Opened 4 years ago Closed 4 years ago

It's not obvious why browser doesn't close (aka It's unclear which tab with onbeforeunlad prevented browser from closing)

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: arni2033, Assigned: Gijs)

References

()

Details

(Keywords: regression)

Attachments

(4 files)

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20151223030323
STR:
1.   Open attached "testcase 1" or the "data:" url from the form above in a new tab
2.   Repeat {Step 1} 4 more times   [you'll get 5 offending tabs total]
3.A) Pin tabs opened in Steps 1 and 2
3.B) Open many new tabs to make tabs opened in Step 1 and 2 invisible in tabstoolbar
4.   Click close button in firefox window OR click   (≡) -> Exit Nightly
5.   Repeat {Step 4} 4 more times

Result:       
 Firefox window remains open. There's no indication of what is happening

Expectations: 
 1) It must be clear which tab contains my unfinished work
 2) It must be clear why the browser doesn't close

It was regressed by bug 332195
> pushlog_url:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f1f6a7ae585eb6ed1eb6caebdf4e491fbfa87fd4&tochange=66d59599465c708d4dc985c774c99ccc26aca69d
See Also: → 1234937
Correction!
In Steps 1 and 2 you must click on content area after you open a tab because of bug 636905.
This is a little annoying because I'd hoped to avoid having to select tabs that have beforeunload dialogs, because of the large amount of abuse that web "feature" gets from trap pages and so on.

Unfortunately, I tested just scrolling the tab into view when it's highlighted, but I think the experience is too weird that way. You click the close button, and some tabs light up, but otherwise nothing happens, and your selected tab might get scrolled out of view? Ordinary users would be non-plussed, I think. Better to just select the tab in question outright.

The first part addresses scenario "A" here - there was a simple oversight in the selector we use for displaying the "attention" and "titlechanged" highlight styles on pinned tabs that broke it for tabs with no favicon. That's a trivial one-line CSS fix.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
The other annoying thing is that this would need uplift. The reason that's annoying is that bug 967873 missed 44, and so now our close-window architecture looks pretty different. I *think* we can just use gInCanCloseWindow by setting it in WindowIsClosing ( http://hg.mozilla.org/releases/mozilla-beta/file/tip/browser/base/content/browser.js#l6563 ) but I'm not 100% sure.

We can pref off bug 332195 on beta, but that would make me quite sad. I guess the compromise would be missing out the gInCanCloseWindow check, which means that closing non-focused tabs would also re-focus the tabs automatically again. I could live with that for 44.
Comment on attachment 8702655 [details]
MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm

https://reviewboard.mozilla.org/r/29163/#review25983

::: browser/base/content/browser.js:6273
(Diff revision 1)
> -    let {permitUnload, timedOut} = browser.permitUnload();
> +    let {permitUnload, timedOut} = browser.permitUnload(true);

What's |true| for?

::: browser/base/content/browser.js:6282
(Diff revision 1)
> -  return true;
> +  gInCanCloseWindow = false;

I think it would be a little cleaner to use try...finally here.

::: browser/base/content/tabbrowser.xml:4376
(Diff revision 1)
> +              !(gInCanCloseWindow && event.detail.inPermitUnload) &&

As far as I know we only call permitUnload from tab closing or from CanCloseWindow. I guess we can close tabs that aren't currently selected. If the closing tab has an onbeforeunload handler, do we actually not want to select the tab? It seems a little strange not to. (Also, we could get rid of this ugly global :-).
Attachment #8702655 - Flags: review?(wmccloskey) → review+
Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

https://reviewboard.mozilla.org/r/29161/#review25985

Someone else should review this. I don't know anything about CSS.
Attachment #8702654 - Flags: review?(wmccloskey)
Oh, I see, we highlight the tab title in that case. And the tab is going to be visible if you're closing it, presumably. Unless the tab calls window.close() on itself I guess. I wonder what happens in that case.
(In reply to :Gijs Kruitbosch from comment #4)
> to avoid having to select tabs that have beforeunload dialogs, because of the large amount of abuse
Imo the only way to notify user of his unfinished work in other tab w/o selecting it is some widget (<panel>) similar to Win7's screen that appears on shutdown and lists all applications which don't let OS shut down. But that looks like a lot of extra work for unimportant feature.
(In reply to Bill McCloskey (:billm) from comment #8)
> Oh, I see, we highlight the tab title in that case. And the tab is going to
> be visible if you're closing it, presumably. Unless the tab calls
> window.close() on itself I guess. I wonder what happens in that case.

IIRC the web can only do that if it's been opened with window.open. Seems like too much of an edge-case (the web page wants to close itself but also has an onbeforeunload handler?) to worry too much about.

(In reply to Bill McCloskey (:billm) from comment #6)
> Comment on attachment 8702655 [details]
> MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an
> onbeforeunload dialog and we're trying to close the window, r?billm
> 
> https://reviewboard.mozilla.org/r/29163/#review25983
> 
> ::: browser/base/content/browser.js:6273
> (Diff revision 1)
> > -    let {permitUnload, timedOut} = browser.permitUnload();
> > +    let {permitUnload, timedOut} = browser.permitUnload(true);
> 
> What's |true| for?

Oops, that's a leftover. I'll revert.


> ::: browser/base/content/browser.js:6282
> (Diff revision 1)
> > -  return true;
> > +  gInCanCloseWindow = false;
> 
> I think it would be a little cleaner to use try...finally here.
> 
> ::: browser/base/content/tabbrowser.xml:4376
> (Diff revision 1)
> > +              !(gInCanCloseWindow && event.detail.inPermitUnload) &&
> 
> As far as I know we only call permitUnload from tab closing or from
> CanCloseWindow. I guess we can close tabs that aren't currently selected. If
> the closing tab has an onbeforeunload handler, do we actually not want to
> select the tab? It seems a little strange not to. (Also, we could get rid of
> this ugly global :-).

As you noted in comment #8, we highlight the title and/or icon of the tab in that case.

I'm biased in that I've fixed a number of bugs where webpages find ways to "trap" users onto a page by a combination of onbeforeunload dialogs and frames/forms/event handlers. One of the things the work in bug 332195 accomplished was give those pages less leverage over the user: now you won't automatically get switched to the right tab, and you can close a tab like that without it getting selected (which sometimes triggered another round of horridness from the page in question).

OTOH, in one way (but not others...) that is a bit of an edge-case. Fixing this bug for all beforeunload dialogs (rather than just the CanCloseWindow case) would also be more upliftable and essentially fix the "new" part of bug 1234937. Though the core issue (multiple dialog-based-event-loop-weirdness) would still need to get fixed, that predates bug 332195 and could be done at a more leisurely pace.

So let's do that for now, even if it makes me sad.
Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29161/diff/1-2/
Attachment #8702654 - Flags: review?(ntim.bugs)
Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

https://reviewboard.mozilla.org/r/29161/#review25997
Attachment #8702654 - Flags: review+
Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29161/diff/1-2/
Attachment #8702654 - Attachment description: MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r?billm → MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim
Comment on attachment 8702655 [details]
MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29163/diff/1-2/
Attachment #8702655 - Attachment description: MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog and we're trying to close the window, r?billm → MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm
Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

https://reviewboard.mozilla.org/r/29161/#review26001

LGTM !
Attachment #8702654 - Flags: review?(ntim.bugs) → review+
See Also: 1234937
https://hg.mozilla.org/mozilla-central/rev/97e179d4873b
https://hg.mozilla.org/mozilla-central/rev/df0d060fbe69
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

(approval request for both changes)

Approval Request Comment
[Feature/regressing bug #]: bug 332195
[User impact if declined]: serious issues with modal dialogs not being noticeable on pinned tabs and/or when used for beforeunload, if the tab is not focused. This is especially surprising when closing a window and when unfocused tabs respond to beforeunload events, thus preventing the window from being closed.
[Describe test coverage new/current, TreeHerder]: there is test coverage for this feature and for dialogs in general, so it's unlikely something got fundamentally broken by this landing
[Risks and why]: the CSS change is trivial and is basically 0-risk. That fixes the pinned tab issue. The other change is slightly riskier, but not dissimilar from a check we already have on release and beta, but that is specific to panorama (tab groups) there. I'll put up a patch for beta that just makes that check slightly more generic.
[String/UUID change made/needed]: no
Attachment #8702654 - Flags: approval-mozilla-beta?
Attachment #8702654 - Flags: approval-mozilla-aurora?
Attached patch Part 2 for betaSplinter Review
Comment on attachment 8703106 [details] [diff] [review]
Part 2 for beta

For beta, we need patch1 + this patch (which is part2)
Attachment #8703106 - Flags: approval-mozilla-beta?
Comment on attachment 8702655 [details]
MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm

Part2 also needs to land on Aurora.
Attachment #8702655 - Flags: approval-mozilla-aurora?
Attachment #8703106 - Attachment description: Patch for beta → Part 2 for beta
Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

This seems related to bug 1233747 that I also uplifted. Taking this one too. Beta44+, Aurora45+
Attachment #8702654 - Flags: approval-mozilla-beta?
Attachment #8702654 - Flags: approval-mozilla-beta+
Attachment #8702654 - Flags: approval-mozilla-aurora?
Attachment #8702654 - Flags: approval-mozilla-aurora+
Comment on attachment 8703106 [details] [diff] [review]
Part 2 for beta

Beta44+
Attachment #8703106 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8702655 [details]
MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm

Aurora45+
Attachment #8702655 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This is fixed on Nightly 46 (ID 20151231030214)
(tab with onbeforeunload dialog becomes active in both cases described in comment 0)
Flags: needinfo?(arni2033)
Has STR: --- → yes
This bug was verified on Nightly 46 in Comment 26, so chaning the status now.
Status: RESOLVED → VERIFIED
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
Test Successful

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Tabs behavior is acceptable

Actual Results: 
As expected
You need to log in before you can comment on or make changes to this bug.