Closed Bug 462673 Opened 12 years ago Closed 11 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: duncan.loveday, Assigned: dao)

References

Details

(Keywords: testcase, verified1.9.1)

Attachments

(2 files, 10 obsolete files)

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.
Attached file Test case
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 ?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
Version: unspecified → Trunk
OS: Windows XP → All
Hardware: PC → All
Blocks: 456002
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;
>            }
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);
Not sure why, but the patch in bug 380960 mostly fixes this... moving to Firefox::Tabbed Browser for now.
Component: DOM: Core & HTML → Tabbed Browser
Depends on: 380960
Flags: blocking1.9.1?
Product: Core → Firefox
QA Contact: general → tabbed.browser
Attached patch patch (obsolete) — Splinter Review
this fixes the remaining bit that bug 380960 doesn't fix
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #345933 - Flags: review?(gavin.sharp)
What does "mostly fixes this" mean? We should figure out what the core bug is here...
(In reply to comment #8)
> What does "mostly fixes this" mean?

It doesn't close the other tab(s) anymore.
It probably has something to do with the DOMWindowClose handler not calling event.preventDefault().
Attached patch better patch (obsolete) — Splinter Review
This fixes the "remaining bit" in a more solid way: _blurTab shouldn't select a tab that's being closed.
Attachment #345933 - Attachment is obsolete: true
Attachment #346028 - Flags: review?(gavin.sharp)
Attachment #345933 - Flags: review?(gavin.sharp)
(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.
Flags: blocking-firefox3.1?
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.
(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.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Whiteboard: [has patch][needs review gavin]
Priority: -- → P2
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?
I can probably reverse the dependency.
Whiteboard: [has patch][needs review gavin] → [needs new patch]
Attachment #346028 - Flags: review?(gavin.sharp)
Attached patch standalone patch (obsolete) — Splinter Review
Attachment #346028 - Attachment is obsolete: true
Attachment #358566 - Flags: review?(gavin.sharp)
Blocks: 380960
No longer depends on: 380960
Whiteboard: [needs new patch]
Blocks: 457096
Whiteboard: [needs review gavin]
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.
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.
Blocks: 392870
No longer blocks: 456002
Attached patch with browser chrome test (obsolete) — Splinter Review
Attachment #358566 - Attachment is obsolete: true
Attachment #362128 - Flags: review?(gavin.sharp)
Attachment #358566 - Flags: review?(gavin.sharp)
this was broken even before bug 392870...
No longer blocks: 392870
Blocks: 113934
Attached patch standalone patch v2 (obsolete) — Splinter Review
_tPos needs to be updated before the dependant tab is removed
Attachment #362128 - Attachment is obsolete: true
Attachment #362128 - Flags: review?(gavin.sharp)
Attachment #362403 - Flags: review?(gavin.sharp)
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.
Blocks: 481560
Something's not right with the test, please review without the test for now.
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.
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.
> The particular problem here is that tab X can be removed in the middle of the
> removal of tab Y

How does this happen?
The unload handler that closes the subsequent tab runs as part of this.mPanelContainer.removeChild(browser.parentNode), as far as I remember.
Why did you need to move the TabClose firing and owner-resetting code?
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.
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.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #362403 - Attachment is obsolete: true
Attachment #366377 - Flags: review?(gavin.sharp)
Attachment #362403 - Flags: review?(gavin.sharp)
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #366377 - Attachment is obsolete: true
Attachment #368245 - Flags: review?(gavin.sharp)
Attachment #366377 - Flags: review?(gavin.sharp)
Attached patch patch v4 (obsolete) — Splinter Review
updated to latest trunk
Attachment #368245 - Attachment is obsolete: true
Attachment #368500 - Flags: review?(gavin.sharp)
Attachment #368245 - Flags: review?(gavin.sharp)
Attached patch patch v4.1 (obsolete) — Splinter Review
using win.closed instead of !win.document
Attachment #368500 - Attachment is obsolete: true
Attachment #368770 - Flags: review?(gavin.sharp)
Attachment #368500 - Flags: review?(gavin.sharp)
Attached patch patch v4.2 (obsolete) — Splinter Review
removing the last setTimeout broke the test (made it pass without the patch)
Attachment #368770 - Attachment is obsolete: true
Attachment #368870 - Flags: review?(gavin.sharp)
Attachment #368770 - Flags: review?(gavin.sharp)
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).
Attachment #368870 - Flags: review?(gavin.sharp) → review+
Whiteboard: [needs review gavin]
(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.
Whiteboard: [needs landing]
Priority: P2 → P1
Attached patch for check-inSplinter Review
Attachment #368870 - Attachment is obsolete: true
(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
http://hg.mozilla.org/mozilla-central/rev/a07d2c4218f9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [baking]
Target Milestone: --- → Firefox 3.6a1
Flags: in-testsuite+
Blocks: 406216
Depends on: 490465
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
Status: RESOLVED → VERIFIED
Depends on: 555875
Depends on: 562336
No longer depends on: 562336
Depends on: 555904
Depends on: 782858
Blocks: 992526
Depends on: 1093404
You need to log in before you can comment on or make changes to this bug.