1238032 - Closing a tab that closes a dependent tab breaks tabbrowser and leaves you with a useless + tabless window

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[prikolist]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160105164030

Steps to reproduce:

1. Open a Google Spreadsheet
2. Open the Script Editor from that tab via the website's menu bar (opens a second tab)
3. Close the Spreadsheet tab


Actual results:

The second tab closes as well


Expected results:

The second tab should have stayed open as the preference (set to false, as it is by default) should prevent it from being closed

Comment 1

[prikolist]

Moreover, if those are the only tabs in the window (with browser.tabs.closeWindowWithLastTab;false), this leaves you with a weird tabless window where you can't add a new tab (clicking the plus or Ctrl+T or File/New Tab does nothing) or reopen a recently closed tab or go to an address

Comment 2

[YF (Yang)]

I think this is normal, the second window (i.e. the "Script Editor") is opened by JavaScript code, it is of course allowed to be the page to close, without respect for the preference.

As for comment 1, it sounds unusual. It is always reproducible for you? Any JS error messages is displayed in browser console?

Comment 3

[prikolist]

Looks like you're right - I just looked at documentation at http://kb.mozillazine.org/About:config_entries#DOM. and I guess this feature isn't what I was looking for at all.

As for comment 1 (which I guess should now be the actual bug) - I see plenty

Here's after closing the tab:

13:27:57.550 TypeError: candidate is undefined
_enterNewTab() tabbrowser.xml:4676
onmouseover() browser.xul:1
1 tabbrowser.xml:4676:15



Here's when trying to open a link from History (or an item from the Bookmarks toolbar):

13:29:04.078 TypeError: this.mCurrentBrowser.loadURIWithFlags is not a function
loadURIWithFlags() tabbrowser.xml:3752
openLinkIn() utilityOverlay.js:334
openUILinkIn() utilityOverlay.js:188
openUILink() utilityOverlay.js:93
HM__onCommand() browser.js:5902
oncommand() browser.xul:1
1 tabbrowser.xml:3752:20



This is for pressing the new tab (plus) button:

13:30:31.905 TypeError: this.selectedItem is null
_setPositionalAttributes() tabbrowser.xml:4616
addTab() tabbrowser.xml:1861
loadOneTab() tabbrowser.xml:1442
openLinkIn() utilityOverlay.js:345
openUILinkIn() utilityOverlay.js:188
BrowserOpenTab() browser.js:12893
BrowserOpenNewTabOrWindow() browser.js:18686
oncommand() browser.xul:1
1 tabbrowser.xml:4616:1

Comment 4

[:Gijs (out for now; he/him)]

I can reproduce the issue in comment 3 on 45, so I'm morphing this to be about addressing that issue.

Comment 7

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1238032 - fix dependent tab close issues by part-reverting bug 305085, r?jaws

Review commit: https://reviewboard.mozilla.org/r/44109/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44109/

Comment 8

[:Gijs (out for now; he/him)]

Comment on attachment 8737782 [details]
MozReview Request: Bug 1238032 - fix dependent tab close issues by part-reverting bug 305085, r?jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44109/diff/1-2/

Comment 9

[:Gijs (out for now; he/him)]

https://reviewboard.mozilla.org/r/44109/#review40673

::: browser/base/content/tabbrowser.xml:2274
(Diff revision 2)
> +              if (closeWindow) {
> +                // We've already called beforeunload on all the relevant tabs if we get here,
> +                // so avoid calling it again:
> +                window.skipNextCanClose = true;
> +              }

So, to be clear, this patch reorders beforeunload and the call to closeWindow again to how they were before 305085. However, as that would reintroduce that bug, it also avoids calling permitUnload again when the window is finally closed. That is now possible because as part of bug 967873 that logic moved to the consumer side instead of being hidden in docshell-land where we only had limited influence on it from the browser code. So rather than "undoing" the permitUnload call by calling a 'reset' type function, as I did in bug 305085, we can avoid calling permitUnload at all, so that's what this patch does.

Comment 10

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

Comment on attachment 8737782 [details]
MozReview Request: Bug 1238032 - fix dependent tab close issues by part-reverting bug 305085, r?jaws

https://reviewboard.mozilla.org/r/44109/#review42035

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ccb2887c57f

Comment 12

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/mozilla-central/rev/5ccb2887c57f - landed yesterday, didn't get marked by bugherder (everything else in the push did, though) - maybe because of the 2 bug numbers? :-\