462673 - Browser exits unexpectedly whan calling window.close() from an unload handler

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Duncan Loveday]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081101 Minefield/3.1b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081101 Minefield/3.1b2pre

When the attached test case is loaded, say in tab 1, it uses window.open() from a load even handler on the document body to open a second tab, say tab 2. The test case loaded in tab 1 also has an onunload handler on the document body that invokes "window.close()" on tab 2 when tab 1 is unloaded. The intention is that closing tab 1 will also close tab 2. However, in the current trunk build closing tab 1 causes the entire browser to exit even if other tabs are open.

Reproducible: Always

Steps to Reproduce:
1. Open the attached test case - this will cause an additional tab to open.
2. Open another at least one other tab so you now have at least 3 tabs open.
3. Close the tab with the test case loaded it it.
Actual Results:  
The whole browser exits

Expected Results:  
The tab with the test case in it should close along with the additional tab the test case opened - however any other tabs should remain open.

Regression since 3.0.2.

This is nasty - it's most disconcerting to have the whole browser go away unexpectedly.

Comment 1

[Duncan Loveday]

Attachment: Test case

Comment 2

[Ria Klaassen (not reading all bugmail)]

Seems to have regressed on 21 Okt 2008; http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-10-21+00%3A00%3A00&enddate=2008-10-21+23%3A00%3A00
Bug 456002 is about tabs and also Bug 461013.

Comment 3

[Duncan Loveday]

To see this bug you have to CLOSE the tab containing the document with the onunload handler. If you just RELOAD the document, the browser does not exit. So it's probably not so much that the window.close() is run from an unload event - rather it is the fact that it is run from a tab that is in the process of closing.

Should this be a critical given that there is the potential to cause loss of data, e.g. if a user was mid way through filling out a form when they decide to close another tab and the browser exits, losing their input ?

Comment 4

[Dão Gottwald [:dao]]

Backing out the patch from bug 456002 seems to fix this, but it also spawns this error:

Error: this.mTabContainer.childNodes[i] is undefined
Source File: chrome://browser/content/tabbrowser.xml
Line: 1554

>            for (i = aTab._tPos; i < length; i++) {
>              this.mTabContainer.childNodes[i]._tPos = i;
>            }

Comment 5

[Dão Gottwald [:dao]]

Seems like this is really a Core bug. It's not _endRemoveTab that closes the window. The window is close with this line:

>            this.mPanelContainer.removeChild(browser.parentNode);

Comment 6

[Dão Gottwald [:dao]]

Not sure why, but the patch in bug 380960 mostly fixes this... moving to Firefox::Tabbed Browser for now.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

this fixes the remaining bit that bug 380960 doesn't fix

Comment 8

[:Gavin Sharp [email: [e-mail]]]

What does "mostly fixes this" mean? We should figure out what the core bug is here...

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> What does "mostly fixes this" mean?

It doesn't close the other tab(s) anymore.

Comment 10

[Dão Gottwald [:dao]]

It probably has something to do with the DOMWindowClose handler not calling event.preventDefault().

Comment 11

[Dão Gottwald [:dao]]

Attachment: better patch

This fixes the "remaining bit" in a more solid way: _blurTab shouldn't select a tab that's being closed.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #10)
> It probably has something to do with the DOMWindowClose handler not calling
> event.preventDefault().

The one in tabbrowser.xml, that is. So, no core bug.

Comment 13

[Duncan Loveday]

I'd definitely vote for this to block Fx 3.1...only I can't find the place in bugzilla to put votes in.

If Fx 3.1 ships without this fixed my web app is broken in a big way.

Comment 14

[Tony Mechelynck [:tonymec]]

(In reply to comment #13)
> I'd definitely vote for this to block Fx 3.1...only I can't find the place in
> bugzilla to put votes in.

To vote, click (vote) on the "Importance" line in the bug header. You must first be logged-in, of course.
Blocking-firefox 3.1 is already requested (near top right) but not yet accepted or rejected as of this writing.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Dao, I don't think we're going to be taking bug 380960 for 1.9.1 - would it be possible to get a patch for this bug that doesn't depend on it?

Comment 16

[Dão Gottwald [:dao]]

I can probably reverse the dependency.

Comment 17

[Dão Gottwald [:dao]]

Attachment: standalone patch

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Could you summarize why this bug is occuring and how that patch fixes it? Comment 12 isn't exactly clear, and it would save me some time reviewing.

Comment 19

[Dão Gottwald [:dao]]

The DOMWindowClose handler doesn't find the browser with the right contentWindow. Why, I haven't found out yet. Explicitly setting this._browsers = null; in the handler doesn't help, neither does using browsers[i] instead of this.getBrowserAtIndex(i). That's not a regression from bug 456002; in some cases there was some other breakage that masked this one.

Comment 20

[Dão Gottwald [:dao]]

Attachment: with browser chrome test

Comment 21

[Dão Gottwald [:dao]]

this was broken even before bug 392870...

Comment 22

[Dão Gottwald [:dao]]

Attachment: standalone patch v2

_tPos needs to be updated before the dependant tab is removed

Comment 23

[Dão Gottwald [:dao]]

To sum it up, this bug was caused by the use of _tPos in attachment 332538 [details] [diff] [review]. This could be fixed by restoring the original "Find and locate the tab in our list" algorithm in both _beginRemoveTab and _endRemoveTab, along with another fix to the use of the "length" variable. I think the current patch is better, as it continues to use the faster _tPos, and allows _beginRemoveTab and _endRemoveTab to be called asynchronously by design. It also fixed the case of a wrong panel being visible temporarily, as the old one is removed before the owner tab is selected.

Comment 24

[Dão Gottwald [:dao]]

Something's not right with the test, please review without the test for now.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

This patch is really hard to review, because it makes all kinds of functional changes for which it's not easy to judge the side effects.

Why is making it possible to call beginRemoveTab/endRemoveTab asynchronously valuable? I forget why we even split the two at all. Looks like the intent was to have the "close" event fire before the new tab's "open" (bug 113934 comment 53), but we already create the new tab before calling _beginRemoveTab, so I'm no longer sure what we're gaining here.

Comment 26

[Dão Gottwald [:dao]]

Even if we'd merge them again, that wouldn't immediately fix this bug. The problem isn't the number of methods but what happens in them (although I'll need two decoupled methods for bug 380960 some day). The particular problem here is that tab X can be removed in the middle of the removal of tab Y (but not between beginRemoveTab and endRemoveTab), so it's important not to make the wrong assumptions e.g. about the number of tabs, tab positions etc.. It's safe to say that the patch does more than what would be needed to fix this particular bug, but all that stuff is intended to make the whole process more robust.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

> The particular problem here is that tab X can be removed in the middle of the
> removal of tab Y

How does this happen?

Comment 28

[Dão Gottwald [:dao]]

The unload handler that closes the subsequent tab runs as part of this.mPanelContainer.removeChild(browser.parentNode), as far as I remember.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Why did you need to move the TabClose firing and owner-resetting code?

Comment 30

[Dão Gottwald [:dao]]

The owner thing isn't critical, since _blurTab skips closing tabs. It can be moved back. TabClose can also be moved back; the right point depends on what's considered "teardown", as the comment says, but sooner can't hurt.

Comment 31

[Dão Gottwald [:dao]]

Oh, _blurTab doesn't check if aTab.owner is closing, so it's actually better to remove the tab as the owner of any other tabs as soon as possible.

Comment 32

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 33

[Dão Gottwald [:dao]]

Attachment: patch v4

Fixed the test / the bug because of which it failed. Also collapsing the old tab early in _endRemoveTab so that it's no longer visible.

Comment 34

[Dão Gottwald [:dao]]

Attachment: patch v4

updated to latest trunk

Comment 35

[Dão Gottwald [:dao]]

Attachment: patch v4.1

using win.closed instead of !win.document

Comment 36

[Dão Gottwald [:dao]]

Attachment: patch v4.2

removing the last setTimeout broke the test (made it pass without the patch)

Comment 37

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 368870 [details] [diff] [review]
patch v4.2

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>       <method name="_endRemoveTab">

In general it would help a lot if this method had more comments explaining the ordering requirements to avoid unnecessary flicker and for correctness in general (e.g. in what order we need to do the selecting of a new tab, tab removal, panel removal, etc.).

>+              // see notes in addTab
>+              let _delayedUpdate = function (aTabContainer) {
>+                aTabContainer.adjustTabstrip();
>+                aTabContainer.mTabstrip._updateScrollButtonsDisabledState();
>+              };

Change this to just function _delayedUpdate() while you're at it? I suppose we could also consider only doing this once in the _removingTabs.length > 1 case as a followup?

>+            // Clean up mTabFilters and mTabListeners now rather than in
>+            // _beginRemoveTab so that their size is always in sync with
>+            // the number of tabs and browsers.
>+            this.mTabFilters.splice(aTab._tPos, 1);
>+            this.mTabListeners.splice(aTab._tPos, 1);

Was this necessary (i.e. do we have code that breaks if you do this earlier)? The filter/listener will be detached if someone does try to access them, but I guess that doesn't really cause a problem...

It seems oddly placed between the tab and panel removals, but maybe there's some dependencies there that I'm missing (again, comments would help).

>+            if (this.mTabBox)
>+              this.mTabBox.selectedPanel = this.getBrowserForTab(this.mCurrentTab).parentNode;

Why are you adding a null check?

>+      <method name="_blurTab">

>+            if (aTab.owner &&
>+                this.mPrefs.getBoolPref("browser.tabs.selectOwnerOnClose")) {
>+              this.selectedTab = aTab.owner;
>+              return;
>+            }

Shouldn't this also check that the owner isn't in _removingTabs?

>+            this.selectedTab = tab;

tab can end up null here, if all tabs are in _removingTabs, which I suppose could happen if a tab closes it's opener tab from an unload listener. The selectedTab setter deals with a null value, so change isn't required here, but would be good to get a test that tests that scenario.

r=me with those addresed, but we should file a Core bug about the JS execution stopping without a thrown exception during the call to removeTab from the DOMWindowClose handler (even if the STR involve backing out this patch).

Comment 38

[Dão Gottwald [:dao]]

(In reply to comment #37)
> >+            // Clean up mTabFilters and mTabListeners now rather than in
> >+            // _beginRemoveTab so that their size is always in sync with
> >+            // the number of tabs and browsers.
> >+            this.mTabFilters.splice(aTab._tPos, 1);
> >+            this.mTabListeners.splice(aTab._tPos, 1);
> 
> Was this necessary (i.e. do we have code that breaks if you do this earlier)?
> The filter/listener will be detached if someone does try to access them, but I
> guess that doesn't really cause a problem...

The xbl destructor does this:

> for (var i = 0; i < this.mTabListeners.length; ++i) {
>   this.getBrowserAtIndex(i).webProgress.removeProgressListener(this.mTabFilters[i]);
>   ...

This can associate the wrong browser with the tab filter and will fail if there are more tab listeners than browsers. I've seen this in the error console, but I don't remember how it was triggered.

> >+            if (this.mTabBox)
> >+              this.mTabBox.selectedPanel = this.getBrowserForTab(this.mCurrentTab).parentNode;
> 
> Why are you adding a null check?

It's possible that the tabbrowser is destructed at that point. I think the first test run would trigger this.

> >+      <method name="_blurTab">

> >+            this.selectedTab = tab;
> 
> tab can end up null here, if all tabs are in _removingTabs, which I suppose
> could happen if a tab closes it's opener tab from an unload listener. The
> selectedTab setter deals with a null value, so change isn't required here, but
> would be good to get a test that tests that scenario.

_beginRemoveTab prevents that by adding a blank tab.

Comment 39

[Dão Gottwald [:dao]]

Attachment: for check-in

Comment 40

[Dão Gottwald [:dao]]

(In reply to comment #37)
> In general it would help a lot if this method had more comments explaining the
> ordering requirements to avoid unnecessary flicker and for correctness in
> general (e.g. in what order we need to do the selecting of a new tab, tab
> removal, panel removal, etc.).

done

> >+              // see notes in addTab
> >+              let _delayedUpdate = function (aTabContainer) {
> >+                aTabContainer.adjustTabstrip();
> >+                aTabContainer.mTabstrip._updateScrollButtonsDisabledState();
> >+              };
> 
> Change this to just function _delayedUpdate() while you're at it? I suppose we
> could also consider only doing this once in the _removingTabs.length > 1 case
> as a followup?

done, and will file a follow-up

> >+            // Clean up mTabFilters and mTabListeners now rather than in
> >+            // _beginRemoveTab so that their size is always in sync with
> >+            // the number of tabs and browsers.
> >+            this.mTabFilters.splice(aTab._tPos, 1);
> >+            this.mTabListeners.splice(aTab._tPos, 1);
> 
> Was this necessary (i.e. do we have code that breaks if you do this earlier)?
> The filter/listener will be detached if someone does try to access them, but I
> guess that doesn't really cause a problem...
> 
> It seems oddly placed between the tab and panel removals, but maybe there's
> some dependencies there that I'm missing (again, comments would help).

moved up a bit and added a comment

> >+      <method name="_blurTab">
> 
> >+            if (aTab.owner &&
> >+                this.mPrefs.getBoolPref("browser.tabs.selectOwnerOnClose")) {
> >+              this.selectedTab = aTab.owner;
> >+              return;
> >+            }
> 
> Shouldn't this also check that the owner isn't in _removingTabs?

done

Comment 41

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a07d2c4218f9

Comment 42

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fb5e6afcfaa4

Comment 43

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre ID:20090505030940

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090505 Shiretoko/3.5b5pre ID:20090505030932