Closed Bug 406216 Opened 17 years ago Closed 15 years ago

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

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 to solve the problem 16 years ago YUKI "Piro" Hiroshi (deleted), patch		Details \| Diff \| Splinter Review
Patch with automated test 16 years ago YUKI "Piro" Hiroshi (deleted), patch		Details \| Diff \| Splinter Review
Patch with automated test (v1.1) 16 years ago YUKI "Piro" Hiroshi (deleted), patch		Details \| Diff \| Splinter Review
Patch with automated test (v1.2) 16 years ago YUKI "Piro" Hiroshi (deleted), patch		Details \| Diff \| Splinter Review
Patch with automated test (v1.3) 16 years ago YUKI "Piro" Hiroshi (deleted), patch		Details \| Diff \| Splinter Review
Patch with automated test (v1.4) 16 years ago YUKI "Piro" Hiroshi (deleted), patch	dao : review+	Details \| Diff \| Splinter Review

YUKI "Piro" Hiroshi

Assignee

Description

•

17 years ago

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

YUKI "Piro" Hiroshi

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

Simon Bünzli

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

YUKI "Piro" Hiroshi

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 → ---

YUKI "Piro" Hiroshi

Assignee

Comment 4

•

16 years ago

Attached patch Patch to solve the problem (obsolete) (deleted) — Details — Splinter Review

YUKI "Piro" Hiroshi

Assignee

Updated

•

16 years ago

Attachment #350770 - Flags: review?(mconnor)

Hidehiro Kozawa

Updated

•

16 years ago

Assignee: nobody → piro

Status: UNCONFIRMED → ASSIGNED

Ever confirmed: true

YUKI "Piro" Hiroshi

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)

Atsushi Sakai

Updated

•

16 years ago

Blocks: 456002

YUKI "Piro" Hiroshi

Assignee

Comment 6

•

16 years ago

Attached patch Patch with automated test (obsolete) (deleted) — Details — Splinter Review

Attachment #351795 - Flags: superreview?(mconnor)

Attachment #351795 - Flags: review?(mconnor)

YUKI "Piro" Hiroshi

Assignee

Updated

•

16 years ago

Attachment #351795 - Attachment is obsolete: true

Attachment #351795 - Flags: superreview?(mconnor)

Attachment #351795 - Flags: review?(mconnor)

YUKI "Piro" Hiroshi

Assignee

Comment 7

•

16 years ago

Attached patch Patch with automated test (v1.1) (obsolete) (deleted) — Details — Splinter Review

Updated test.

Attachment #351797 - Flags: superreview?(mconnor)

Attachment #351797 - Flags: review?(mconnor)

YUKI "Piro" Hiroshi

Assignee

Updated

•

16 years ago

Attachment #351797 - Attachment is obsolete: true

Attachment #351797 - Flags: superreview?(mconnor)

Attachment #351797 - Flags: review?(mconnor)

YUKI "Piro" Hiroshi

Assignee

Comment 8

•

16 years ago

Attached patch Patch with automated test (v1.2) (obsolete) (deleted) — Details — Splinter Review

Updated test.

Attachment #351998 - Flags: superreview?(mconnor)

Attachment #351998 - Flags: review?(mconnor)

YUKI "Piro" Hiroshi

Assignee

Updated

•

16 years ago

Attachment #351998 - Attachment is obsolete: true

Attachment #351998 - Flags: superreview?(mconnor)

Attachment #351998 - Flags: review?(mconnor)

YUKI "Piro" Hiroshi

Assignee

Comment 9

•

16 years ago

Attached patch Patch with automated test (v1.3) (obsolete) (deleted) — Details — Splinter Review

Updated test.

Attachment #352291 - Flags: superreview?

Attachment #352291 - Flags: review?

YUKI "Piro" Hiroshi

Assignee

Updated

•

16 years ago

Attachment #352291 - Attachment is obsolete: true

Attachment #352291 - Flags: superreview?

Attachment #352291 - Flags: review?

YUKI "Piro" Hiroshi

Assignee

Comment 10

•

16 years ago

Attached patch Patch with automated test (v1.4) (deleted) — Details — Splinter Review

Test updated.

Attachment #352984 - Flags: superreview?

Attachment #352984 - Flags: review?

:Gavin Sharp [email: gavin@gavinsharp.com]

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?

YUKI "Piro" Hiroshi

Assignee

Updated

•

16 years ago

Attachment #352984 - Flags: superreview?(mconnor)

Attachment #352984 - Flags: superreview?

Attachment #352984 - Flags: review?(mconnor)

Attachment #352984 - Flags: review?

YUKI "Piro" Hiroshi

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!

YUKI "Piro" Hiroshi

Assignee

Updated

•

16 years ago

Attachment #352984 - Flags: review?(mconnor) → review?(gavin.sharp)

YUKI "Piro" Hiroshi

Assignee

Updated

•

16 years ago

Attachment #352984 - Flags: superreview?(mconnor)

Mike Beltzner [:beltzner, not reading bugmail]

Updated

•

16 years ago

Flags: wanted-firefox3.1+

Flags: blocking-firefox3.1?

Flags: blocking-firefox3.1-

Dão Gottwald [:dao]

Comment 13

•

16 years ago

Bug 462673 should have fixed this.

Depends on: 462673

Dão Gottwald [:dao]

Updated

•

16 years ago

Keywords: fixed1.9.1

Whiteboard: [fixed by bug 462673]

Target Milestone: --- → Firefox 3.6a1

Dão Gottwald [:dao]

Updated

•

16 years ago

Flags: in-testsuite?

Dão Gottwald [:dao]

Updated

•

15 years ago

Status: ASSIGNED → RESOLVED

Closed: 16 years ago → 15 years ago

Resolution: --- → FIXED

Henrik Skupin [:whimboo][⌚️UTC+2]

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

Dão Gottwald [:dao]

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+

Dão Gottwald [:dao]

Updated

•

15 years ago

Flags: in-testsuite? → in-testsuite+

Dão Gottwald [:dao]

Updated

•

13 years ago

Depends on: 714175

You need to log in before you can comment on or make changes to this bug.