tab.close creates ghost tab

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: zepeuj, Assigned: evold)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
I've found something strange with my add-on:

When I close a tab "not ready", the tab is well closed but after this, when I list the tabs using 'browserWindows.activeWindow.tabs', the tab still exist with title 'undefined' and url 'undefined'.

If I list the tabs using 'var tabs = require('sdk/tabs');' the closed tab doesn't appear.
It only happens using 'var activeWindowTabs = require("sdk/windows").browserWindows.activeWindow.tabs;'

I use the SDK 1.13.2.


Here's a sample code to reproduce the problem:

function addTabListener(){     

    var tabs = require('sdk/tabs');   
   
    tabs.on('open', function(tab) {
            closeTabOpened(tab);
            });

}

function closeTabOpened(newtab) {       
   
    var activeWindowTabs  = require("sdk/windows").browserWindows.activeWindow.tabs;

    for each (var tab in activeWindowTabs) {                 
       
        try{
            if (tab.index == newtab.index)
                continue;
        }
        catch (e){
            //tab not ready
            continue;
        }
       
        if (tab.url == newtab.title){           
            newtab.close();               
            return;
        }           
    }
}

function listAllTabs(kill) {   
    var count = 0;
    var urlMap = {};
    var activeWindowTabs  = require("sdk/windows").browserWindows.activeWindow.tabs;   
    var tabs = require('sdk/tabs');
 
    console.log(activeWindowTabs.length);
    console.log(tabs .length);

    for each (var tab in tabs) {   
       
        console.log('tab found', tab.title, tab.url);       
    } 

    for each (var tab in activeWindowTabs) {   
       
        console.log('tab found', tab.title, tab.url);       
    } 
}

Thanks
Hmm I can't reproduce this.. do you have a sample add-on that I can try?
Created attachment 714946 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/796

Pointer to Github pull-request
Comment on attachment 714946 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/796

I wrote some tests for this bug since I didn't see any, but I can't reproduce it..
Attachment #714946 - Flags: review?(poirot.alex)
cherry-picked the tests on to the 1.13.2 tag and they all pass.
(Reporter)

Comment 5

6 years ago
I'll create an add-on tomorrow for your tests and send you the link.

The more important for the test, it's to close the tab when it is 'opened' before it is 'ready'.
(Reporter)

Comment 6

6 years ago
I've created an add-on 'Test close tabs' accessible at https://builder.addons.mozilla.org/package/174373/latest/

This add-on close the new opened tab and list all existing tabs in console.
Steps to reproduce the problem:
 1 - open a new tab => the tab is closed and no ghost tab is listed in the console.
 2 - again open a new tab => the tab is closed and this time a ghost tab is listed in the console.
Duplicate of this bug: 842598
(In reply to zepeuj from comment #6)
> I've created an add-on 'Test close tabs' accessible at
> https://builder.addons.mozilla.org/package/174373/latest/
> 
> This add-on close the new opened tab and list all existing tabs in console.
> Steps to reproduce the problem:
>  1 - open a new tab => the tab is closed and no ghost tab is listed in the
> console.
>  2 - again open a new tab => the tab is closed and this time a ghost tab is
> listed in the console.

ok I can reproduce with your example add-on now, thanks for this!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → UNCONFIRMED
Ever confirmed: false
OS: Windows 7 → All
Hardware: x86_64 → All
(Reporter)

Comment 9

6 years ago
Why the status has been changed to unconfirmed? Something wrong in the way to do?

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
(In reply to zepeuj from comment #9)
> Why the status has been changed to unconfirmed? Something wrong in the way
> to do?

oops my mistake, I think I was trying to confirm it, noticed it was already and changed the state by mistake, sorry!
Blocks: 821779
So this isn't a regression, but it is an error that is more obvious now because of the patch that landed in bug 833783.

https://github.com/mozilla/addon-sdk/commit/b06eb5ba5d95fa7c2ccfb916ed7a4bc388579a9a

It looks like this example code. as far back as 1.10 at least, would report an ever increasing value for `console.log('nb tabs with activeWindowTabs :', nbActiveWindowTabs);` 

and cause an error if one tried to access an properties on on the tab.

Traceback (most recent call last):
  File "chrome://browser/content/browser.xul", line 1, in oncommand
    <?xml version="1.0"?>
  File "chrome://browser/content/browser.js", line 9161, in BrowserOpenTab
    openUILinkIn(BROWSER_NEW_TAB_URL, "tab");
  File "chrome://browser/content/utilityOverlay.js", line 206, in openUILinkIn
    openLinkIn(url, where, params);
  File "chrome://browser/content/utilityOverlay.js", line 318, in openLinkIn
    isUTF8: aIsUTF8});
  File "chrome://browser/content/tabbrowser.xml", line 1130, in loadOneTab
    isUTF8: aIsUTF8});
  File "chrome://browser/content/tabbrowser.xml", line 1374, in addTab
    t.dispatchEvent(evt);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/api-utils/lib/tabs/observer.js", line 50, in handleEvent
    this._emit(EVENTS[event.type], event.target, event);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/api-utils/lib/events.js", line 123, in _emit
    return this._emitOnObject.apply(this, args);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/api-utils/lib/events.js", line 153, in _emitOnObject
    listener.apply(targetObj, params);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/api-utils/lib/windows/tabs.js", line 101, in _onTabEvent
    this._emitEvent(type, wrappedTab);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/api-utils/lib/windows/tabs.js", line 106, in _emitEvent
    tabs._emit(type, tab);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/api-utils/lib/events.js", line 123, in _emit
    return this._emitOnObject.apply(this, args);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/api-utils/lib/events.js", line 153, in _emitOnObject
    listener.apply(targetObj, params);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/test-addon/lib/main.js", line 18, in 
    closeTabOpened(tab);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/test-addon/lib/main.js", line 26, in closeTabOpened
    listAllTabs();
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/test-addon/lib/main.js", line 50, in listAllTabs
    console.log('activeWindowTabs tab title:', tab.title);
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/api-utils/lib/tabs/tab.js", line 113, in TabTrait<.title
    get title() getTabTitle(this._tab),
  File "resource://jid1-rysibkibbbgqbg-at-jetpack/api-utils/lib/tabs/utils.js", line 75, in getTabTitle
    return getBrowserForTab(tab).contentDocument.title || tab.label;
TypeError: getBrowserForTab(...) is null
https://github.com/mozilla/addon-sdk/pull/796

This pull request has a test that reproduces the problem (ie the test fails), I don't have a fix for this right now and given it has been a long standing issue and that we plan to rewrite the tabs module for Firefox in order to remove Traits, I'm not sure if we should dig in to this other than to land the test with bug 724632.
Depends on: 724632
Dave you should probably take another look at this and see how important you think it is.
Flags: needinfo?(dtownsend+bugmail)
Depends on: 798069
We'll look at this again in the next triage
Flags: needinfo?(dtownsend+bugmail)
Priority: P2 → --

Updated

6 years ago
Priority: -- → P3
Comment on attachment 714946 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/796

Reseting review request, as it doesn't make sense to land broken tests ;)
Otherwise, this bug may be fixed with this:
  https://github.com/mozilla/addon-sdk/pull/838

Bad stuff happened when doing tab related action during "open" tabs event.
Attachment #714946 - Flags: review?(poirot.alex)
Assignee: nobody → evold
darn this isn't fixed still..
(Reporter)

Comment 17

6 years ago
Hi,

Just to explain why I've used the "open" tab event instead of the "ready" tab event:

My goal is to close a "duplicated" tab comparing the URL but I don't want/need to wait for the new tab to be fully loaded as I may want to close it.
So once the tab is opened I check the tab title (which may be set with the URL) or wait the URL is not "about:blank" or "about:newtab" then if the new tab URL (or tab title) matches an existing tab URL, I close the new tab.

For the moment I use the "ready" tab event but it is not efficient as I need to wait for the tab to be fully loaded before to close it.

Thanks.
(Reporter)

Comment 18

6 years ago
To complete my previous comment, if you have a better alternative, I'll be glad to know. :)

Thanks
Assignee: evold → nobody
Attachment #714946 - Flags: review?(rFobic)
Comment on attachment 714946 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/796

Sure we need this tests, but we should not land them without fix. I'm assigning this to myself and will include or do right after:
https://github.com/mozilla/addon-sdk/pull/1207
Attachment #714946 - Flags: review?(rFobic) → review+
Assignee: nobody → rFobic
Hey Irakli, are you working on this? if you could you please unassign yourself.
Flags: needinfo?(rFobic)

Comment 21

4 years ago
Since he is not responding, I'm resetting the assignee. If you are indeed still working on this :irakli, please re-assign yourself.
Assignee: rFobic → nobody
It was on my list along with other 100 bugs so probably unassigning was a good move.
Flags: needinfo?(rFobic)

Comment 23

4 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d87a3e67fecf8fb90453ff54ca7963a931c68e0f
Bug 842148: adding test for reported issue of closed tab existing in tab iterators r=gozala

From https://github.com/mozilla/addon-sdk/pull/796
This was fixed in bug 854982, so I've now landed the test that I wrote for this bug.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Assignee: nobody → evold
You need to log in before you can comment on or make changes to this bug.