Closed Bug 406216 Opened 17 years ago Closed 15 years ago

"TabClose" event shoud be allow to close related tabs by its listeners

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3.6a1

People

(Reporter: yuki, Assigned: yuki)

References

Details

(Keywords: verified1.9.1, Whiteboard: [fixed by bug 462673])

Attachments

(1 file, 5 obsolete files)

Patch with automated test (v1.4)
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; ja; rv:1.8.1.10) Gecko/20071115 Firefox/2.0.0.10 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; ja; rv:1.8.1.10) Gecko/20071115 Firefox/2.0.0.10 Closing other tabs by event listeners of "TabClose" event possibly cause a null pointer error and breaks Firefox. We extension authors sometimes use "TabClose" event to do something, and I thought to close tabs related to the closing tab. However, I saw the NS_ERROR_NULL_POINTER error when I actually did it. This problem reduces convenience of this API ("TabClose" event). Reproducible: Always Steps to Reproduce: 1. Add event listener which closes following tabs of the closing tab, to "TabClose" event. 2. Close the current tab. Actual Results: Firefox selects no tab and NS_ERROR_NULL_POINTER appears in the error console. Expected Results: Firefox selects the last tab (which was the previous tab of the closed one) and no error appear in the error console. Code to reproduce the description: gBrowser.addEventListener('TabClose', function(aEvent) { var tab = aEvent.originalTarget; while (tab.nextSibling) gBrowser.removeTab(tab.nextSibling); }, false);
I found the cause why this problem appearas. In the "removeTab" method, the number of tabs is put in the variable "l" before the "TabClose" is dispatched, like: ------------------------- <method name="removeTab"> <parameter name="aTab"/> <body> <![CDATA[ this._browsers = null; // invalidate cache if (aTab.localName != "tab") aTab = this.mCurrentTab; var l = this.mTabContainer.childNodes.length; (snip) var evt = document.createEvent("Events"); evt.initEvent("TabClose", true, false); aTab.dispatchEvent(evt); ------------------------- However, the value of "l" isn't updated. If the number of tabs is reduced when event listeners close related tabs, "newIndex" in following section possibly indicates the index of a tab which is not existing. By inserting a line to update "l" after the "TabClose" event is dispatched, this problem will disappear, like: ------------------------- var evt = document.createEvent("Events"); evt.initEvent("TabClose", true, false); aTab.dispatchEvent(evt); // l = this.mTabContainer.childNodes.length; // <= "l" should be updated in this point var index = -1; if (this.mCurrentTab == aTab) index = this.mTabContainer.selectedIndex; (snip) if (newIndex == -1) newIndex = (index == l - 1) ? index - 1 : index; } // Select the new tab this.selectedTab = this.mTabContainer.childNodes[newIndex]; // <- this possibly causes NS_ERROR_NULL_POINTER (snip) ]]> </body> </method> -------------------------
Version: unspecified → Trunk
This has been fixed by bug 113934 (for Firefox 3.1).
Status: UNCONFIRMED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
The problem is back on: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081130 Minefield/3.1b3pre <method name="_beginRemoveTab"> <parameter name="aTab"/> <parameter name="aFireBeforeUnload"/> <parameter name="aCloseWindowWithLastTab"/> <body> <![CDATA[ (snip) var l = this.mTabs.length; (snip) var evt = document.createEvent("Events"); evt.initEvent("TabClose", true, false); aTab.dispatchEvent(evt); (snip) for (let i = 0; i < l; ++i) { let tab = this.mTabs[i]; if ("owner" in tab && tab.owner == aTab) // |tab| is a child of the tab we're removing, make it an orphan tab.owner = null; } return [aTab, closeWindow]; ]]> </body> </method> This code causes just same problem I reported in the last year. When an extension closes related tabs by "TabClose" event, '"owner" in tab' raises an error.
Status: RESOLVED → UNCONFIRMED
Resolution: FIXED → ---
Attached patch Patch to solve the problem (obsolete) (deleted) — — Splinter Review
Attachment #350770 - Flags: review?(mconnor)
Assignee: nobody → piro
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 350770 [details] [diff] [review] Patch to solve the problem Now I'm writing an automated test...
Attachment #350770 - Attachment is obsolete: true
Attachment #350770 - Flags: review?(mconnor)
Blocks: 456002
Attached patch Patch with automated test (obsolete) (deleted) — — Splinter Review
Attachment #351795 - Flags: superreview?(mconnor)
Attachment #351795 - Flags: review?(mconnor)
Attachment #351795 - Attachment is obsolete: true
Attachment #351795 - Flags: superreview?(mconnor)
Attachment #351795 - Flags: review?(mconnor)
Attached patch Patch with automated test (v1.1) (obsolete) (deleted) — — Splinter Review
Updated test.
Attachment #351797 - Flags: superreview?(mconnor)
Attachment #351797 - Flags: review?(mconnor)
Attachment #351797 - Attachment is obsolete: true
Attachment #351797 - Flags: superreview?(mconnor)
Attachment #351797 - Flags: review?(mconnor)
Attached patch Patch with automated test (v1.2) (obsolete) (deleted) — — Splinter Review
Updated test.
Attachment #351998 - Flags: superreview?(mconnor)
Attachment #351998 - Flags: review?(mconnor)
Attachment #351998 - Attachment is obsolete: true
Attachment #351998 - Flags: superreview?(mconnor)
Attachment #351998 - Flags: review?(mconnor)
Attached patch Patch with automated test (v1.3) (obsolete) (deleted) — — Splinter Review
Updated test.
Attachment #352291 - Flags: superreview?
Attachment #352291 - Flags: review?
Attachment #352291 - Attachment is obsolete: true
Attachment #352291 - Flags: superreview?
Attachment #352291 - Flags: review?
Test updated.
Attachment #352984 - Flags: superreview?
Attachment #352984 - Flags: review?
Hiroshi-san, you should request review from one of the browser peers specifically if you want to get your patch reviewed: http://www.mozilla.org/projects/firefox/review.html
Flags: blocking-firefox3.1?
Attachment #352984 - Flags: superreview?(mconnor)
Attachment #352984 - Flags: superreview?
Attachment #352984 - Flags: review?(mconnor)
Attachment #352984 - Flags: review?
Comment on attachment 352984 [details] [diff] [review] Patch with automated test (v1.4) Oh, I forgot to input reviewer's address... thanks!
Attachment #352984 - Flags: review?(mconnor) → review?(gavin.sharp)
Attachment #352984 - Flags: superreview?(mconnor)
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Bug 462673 should have fixed this.
Depends on: 462673
Keywords: fixed1.9.1
Whiteboard: [fixed by bug 462673]
Target Milestone: --- → Firefox 3.6a1
Flags: in-testsuite?
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
Shimoda, your latest test has a nice test which never got reviewed and checked-in. Would you have time to update the patch? Following the steps in comment 0 no error is visible and the last open tab is focused. Marking verified on 1.9.2 and 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b3pre) Gecko/20091112 Namoroka/3.6b3pre (.NET CLR 3.5.30729) ID:20091112051557 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091111 Shiretoko/3.5.6pre (.NET CLR 3.5.30729) ID:20091111044604
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Comment on attachment 352984 [details] [diff] [review] Patch with automated test (v1.4) Thanks for the test. Looks good. I've landed this after making only a few minor adjustments. http://hg.mozilla.org/mozilla-central/rev/bc0bb96586de
Attachment #352984 - Flags: review?(gavin.sharp) → review+
Flags: in-testsuite? → in-testsuite+
Depends on: 714175
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: