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)
Firefox
Tabbed Browser
Tracking
()
VERIFIED
FIXED
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)
(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);
Assignee | ||
Comment 1•17 years ago
|
||
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
Comment 2•16 years ago
|
||
This has been fixed by bug 113934 (for Firefox 3.1).
Status: UNCONFIRMED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 3•16 years ago
|
||
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 → ---
Assignee | ||
Comment 4•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Attachment #350770 -
Flags: review?(mconnor)
Updated•16 years ago
|
Assignee: nobody → piro
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 5•16 years ago
|
||
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)
Assignee | ||
Comment 6•16 years ago
|
||
Attachment #351795 -
Flags: superreview?(mconnor)
Attachment #351795 -
Flags: review?(mconnor)
Assignee | ||
Updated•16 years ago
|
Attachment #351795 -
Attachment is obsolete: true
Attachment #351795 -
Flags: superreview?(mconnor)
Attachment #351795 -
Flags: review?(mconnor)
Assignee | ||
Comment 7•16 years ago
|
||
Updated test.
Attachment #351797 -
Flags: superreview?(mconnor)
Attachment #351797 -
Flags: review?(mconnor)
Assignee | ||
Updated•16 years ago
|
Attachment #351797 -
Attachment is obsolete: true
Attachment #351797 -
Flags: superreview?(mconnor)
Attachment #351797 -
Flags: review?(mconnor)
Assignee | ||
Comment 8•16 years ago
|
||
Updated test.
Attachment #351998 -
Flags: superreview?(mconnor)
Attachment #351998 -
Flags: review?(mconnor)
Assignee | ||
Updated•16 years ago
|
Attachment #351998 -
Attachment is obsolete: true
Attachment #351998 -
Flags: superreview?(mconnor)
Attachment #351998 -
Flags: review?(mconnor)
Assignee | ||
Comment 9•16 years ago
|
||
Updated test.
Attachment #352291 -
Flags: superreview?
Attachment #352291 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #352291 -
Attachment is obsolete: true
Attachment #352291 -
Flags: superreview?
Attachment #352291 -
Flags: review?
Assignee | ||
Comment 10•16 years ago
|
||
Test updated.
Attachment #352984 -
Flags: superreview?
Attachment #352984 -
Flags: review?
Comment 11•16 years ago
|
||
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?
Assignee | ||
Updated•16 years ago
|
Attachment #352984 -
Flags: superreview?(mconnor)
Attachment #352984 -
Flags: superreview?
Attachment #352984 -
Flags: review?(mconnor)
Attachment #352984 -
Flags: review?
Assignee | ||
Comment 12•16 years ago
|
||
Comment on attachment 352984 [details] [diff] [review] Patch with automated test (v1.4) Oh, I forgot to input reviewer's address... thanks!
Assignee | ||
Updated•16 years ago
|
Attachment #352984 -
Flags: review?(mconnor) → review?(gavin.sharp)
Assignee | ||
Updated•16 years ago
|
Attachment #352984 -
Flags: superreview?(mconnor)
Updated•16 years ago
|
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Updated•16 years ago
|
Updated•16 years ago
|
Flags: in-testsuite?
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
Comment 14•15 years ago
|
||
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 15•15 years ago
|
||
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+
Updated•15 years ago
|
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•