842148 - tab.close creates ghost tab

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P3)

Description

[zepeuj]

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

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Hmm I can't reproduce this.. do you have a sample add-on that I can try?

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/796

Pointer to Github pull-request

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

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

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

cherry-picked the tests on to the 1.13.2 tag and they all pass.

Comment 5

[zepeuj]

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'.

Comment 6

[zepeuj]

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.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(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!

Comment 9

[zepeuj]

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

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(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!

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

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

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

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.

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

Dave you should probably take another look at this and see how important you think it is.

Comment 14

[Dave Townsend [:mossop]]

We'll look at this again in the next triage

Comment 15

[Alexandre Poirot [:ochameau]]

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.

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

darn this isn't fixed still..

Comment 17

[zepeuj]

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.

Comment 18

[zepeuj]

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

Thanks

Comment 19

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Irakli, are you working on this? if you could you please unassign yourself.

Comment 21

[Florian Bender]

Since he is not responding, I'm resetting the assignee. If you are indeed still working on this :irakli, please re-assign yourself.

Comment 22

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

It was on my list along with other 100 bugs so probably unassigning was a good move.

Comment 23

[[github robot]]

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

Comment 24

[Erik Vold [:erikvold] (please needinfo? me)]

This was fixed in bug 854982, so I've now landed the test that I wrote for this bug.