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

VERIFIED FIXED in Firefox 3.6a1

Status

()

defect
P1
normal
VERIFIED FIXED
11 years ago
5 years ago

People

(Reporter: duncan.loveday, Assigned: dao)

Tracking

({testcase, verified1.9.1})

Trunk
Firefox 3.6a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

(Reporter)

Description

11 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.
(Reporter)

Comment 1

11 years ago
Posted file Test case
(Reporter)

Comment 3

11 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 ?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
Version: unspecified → Trunk
(Assignee)

Updated

11 years ago
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Updated

11 years ago
Blocks: 456002
(Assignee)

Comment 4

11 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;
>            }
(Assignee)

Comment 5

11 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);
(Assignee)

Comment 6

11 years ago
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
(Assignee)

Comment 7

11 years ago
Posted 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...
(Assignee)

Comment 9

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

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

Comment 10

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

Comment 11

11 years ago
Posted 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)
(Assignee)

Comment 12

11 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.
(Assignee)

Updated

11 years ago
Flags: blocking-firefox3.1?
(Reporter)

Comment 13

11 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.
(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?
(Assignee)

Comment 16

10 years ago
I can probably reverse the dependency.
Whiteboard: [has patch][needs review gavin] → [needs new patch]
Attachment #346028 - Flags: review?(gavin.sharp)
(Assignee)

Comment 17

10 years ago
Posted patch standalone patch (obsolete) — Splinter Review
Attachment #346028 - Attachment is obsolete: true
Attachment #358566 - Flags: review?(gavin.sharp)
(Assignee)

Updated

10 years ago
Blocks: 380960
No longer depends on: 380960
Whiteboard: [needs new patch]
(Assignee)

Updated

10 years ago
Blocks: 457096
(Assignee)

Updated

10 years ago
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.
(Assignee)

Comment 19

10 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.
Blocks: 392870
No longer blocks: 456002
(Assignee)

Comment 20

10 years ago
Posted 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)
(Assignee)

Comment 21

10 years ago
this was broken even before bug 392870...
No longer blocks: 392870
(Assignee)

Updated

10 years ago
Blocks: 113934
(Assignee)

Comment 22

10 years ago
Posted 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)
(Assignee)

Updated

10 years ago
Attachment #362403 - Flags: review?(gavin.sharp)
(Assignee)

Comment 23

10 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.
(Assignee)

Updated

10 years ago
Blocks: 481560
(Assignee)

Comment 24

10 years ago
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.
(Assignee)

Comment 26

10 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.
> The particular problem here is that tab X can be removed in the middle of the
> removal of tab Y

How does this happen?
(Assignee)

Comment 28

10 years ago
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?
(Assignee)

Comment 30

10 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.
(Assignee)

Comment 31

10 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.
(Assignee)

Comment 32

10 years ago
Posted patch patch v3 (obsolete) — Splinter Review
Attachment #362403 - Attachment is obsolete: true
Attachment #366377 - Flags: review?(gavin.sharp)
Attachment #362403 - Flags: review?(gavin.sharp)
(Assignee)

Comment 33

10 years ago
Posted 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)
(Assignee)

Comment 34

10 years ago
Posted 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)
(Assignee)

Comment 35

10 years ago
Posted 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)
(Assignee)

Comment 36

10 years ago
Posted 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]
(Assignee)

Comment 38

10 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.
Whiteboard: [needs landing]
Priority: P2 → P1
(Assignee)

Comment 39

10 years ago
Posted patch for check-inSplinter Review
Attachment #368870 - Attachment is obsolete: true
(Assignee)

Comment 40

10 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
(Assignee)

Comment 41

10 years ago
http://hg.mozilla.org/mozilla-central/rev/a07d2c4218f9
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [baking]
Target Milestone: --- → Firefox 3.6a1
(Assignee)

Updated

10 years ago
Flags: in-testsuite+
(Assignee)

Updated

10 years ago
Blocks: 406216
(Assignee)

Updated

10 years ago
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

Updated

9 years ago
Depends on: 555875

Updated

9 years ago
Depends on: 562336

Updated

9 years ago
No longer depends on: 562336

Updated

9 years ago
Depends on: 555904
Depends on: 782858
(Assignee)

Updated

5 years ago
Blocks: 992526

Updated

5 years ago
Depends on: 1093404
You need to log in before you can comment on or make changes to this bug.