Duncan Loveday Reporter
	Description • 16 years ago

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.

	Ria Klaassen (not reading all bugmail)
	Comment 2 • 16 years ago

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.

	Duncan Loveday Reporter
	Comment 3 • 16 years ago

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 ?

	Dão Gottwald [:dao] Assignee
	Comment 4 • 16 years ago

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;
>            }

	Dão Gottwald [:dao] Assignee
	Comment 5 • 16 years ago

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);

	Dão Gottwald [:dao] Assignee
	Comment 6 • 16 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 7 • 16 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 8 • 16 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 9 • 16 years ago

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

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

	Dão Gottwald [:dao] Assignee
	Comment 10 • 16 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 11 • 16 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 12 • 16 years ago

(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.

	Duncan Loveday Reporter
	Comment 13 • 16 years ago

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.

	Tony Mechelynck [:tonymec]
	Comment 14 • 16 years ago

(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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 15 • 16 years ago

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?

	Dão Gottwald [:dao] Assignee
	Comment 16 • 16 years ago

I can probably reverse the dependency.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 18 • 16 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 19 • 16 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 21 • 16 years ago

this was broken even before bug 392870...

	Dão Gottwald [:dao] Assignee
	Comment 22 • 16 years ago

_tPos needs to be updated before the dependant tab is removed

	Dão Gottwald [:dao] Assignee
	Comment 23 • 16 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 24 • 16 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 25 • 16 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 26 • 16 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 27 • 16 years ago

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

How does this happen?

	Dão Gottwald [:dao] Assignee
	Comment 28 • 16 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 29 • 16 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 30 • 16 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 31 • 16 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 33 • 16 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 34 • 16 years ago

updated to latest trunk

	Dão Gottwald [:dao] Assignee
	Comment 35 • 16 years ago

using win.closed instead of !win.document

	Dão Gottwald [:dao] Assignee
	Comment 36 • 16 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 37 • 16 years ago

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).

	Dão Gottwald [:dao] Assignee
	Comment 38 • 16 years ago

(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.

	Dão Gottwald [:dao] Assignee
	Comment 40 • 16 years ago

(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

	Dão Gottwald [:dao] Assignee
	Comment 41 • 16 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 42 • 16 years ago

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

	Aakash Desai [:aakashd]
	Comment 43 • 15 years ago

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