1396758 - tabs.onCreated and onRemoved are evaluated at different times

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Dietrich Ayala (:dietrich)]

57.0a1 (2017-09-04) (64-bit) Mac OS X

Listeners for onCreated event are called after the action occurs.

Listeners for the onRemoved event are called before the action occurs.

For example, given the following code:

function dumpTabCount() {
  browser.tabs.query({}).then(function(tabs) {
    console.log('TABS', tabs.length);
  }); 
}

browser.tabs.onCreated.addListener(dumpTabCount);
browser.tabs.onRemoved.addListener(dumpTabCount);


To illustrate, here's a log of what happens with the code above and a browser with one tab open:

<add a tab (2 tabs)>
2
<add a tab (3 tabs)>
3
<remove a tab (2 tabs)>
3
<remove a tab (1 tab)>
2

Comment 1

[Andy McKay]

Could you quickly try this in Chrome and tell us what it does there?

Comment 2

[Dietrich Ayala (:dietrich)]

Bob is looking at tab event ordering vs Chrome in bug 1366290 so ni?ing him to see if he knows.

Comment 3

[Bob Silverberg [:bsilverberg]]

This is a different issue from bug 1366290, which is just about the order in which tabs.onActivated and tabs.onRemoved are fired. I checked Chrome and it does have a different behaviour from Firefox, and what I believe is the correct behaviour. The above code does not include the removed tab in the output from onRemoved in Chrome.

As this is a different issue it should remain its own bug, anb does seem like something we should fix.

Comment 4

[Morikko]

The bug is really annoying for keeping a record of the current tabs states.
Now, I have no choice to put a hard coded delay for getting the state when the tab is completely closed.

onRemoved should be fired once the tab is completely closed, I find it makes more sense.

firefox: 57.0b11

browser.tabs.onRemoved.addListener(() => {
  browser.tabs.query({
      currentWindow: true
    }).then((tabs) => {
      // tabs STILL contains the close one
    });
});

browser.tabs.onRemoved.addListener(() => {
  setTimeout(()=>{
    browser.tabs.query({
      currentWindow: true
    }).then((tabs) => {
      // tabs DONT contains the close one
    });
  }, 300); // Value chosen arbitrarily 
});

browser.tabs.onCreated.addListener(() => {
  browser.tabs.query({
      currentWindow: true
    }).then((tabs) => {
      // tabs contains the open one
    });
});

Comment 7

[shivanand]

Is this the same bug? https://bugzilla.mozilla.org/show_bug.cgi?id=1396758

Comment 8

[Dietrich Ayala (:dietrich)]

Yes!

Comment 9

[Oriol Brufau [:Oriol]]

Attachment: Bug 1396758 - Don't fire tabs.onRemoved event until the tab is completely removed. r?mixedpuppy

Comment 10

[Shane Caraveo (:mixedpuppy)]

Oriol, What is the status on this?

Comment 11

[Oriol Brufau [:Oriol]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)

Oriol, What is the status on this?

Unassigning myself since I haven't been working on this lately.

This is actually very tricky. The desired behavior is

• onRemoved should fire once the tab is completely removed, so that it doesn't appear in browser.tabs.query
• for compatibility with Chrome, onRemoved is supposed to happen before onActivated when the active tab is removed.

However, internally this is what happens

1. gBrowser.removeTab calls gBrowser._beginRemoveTab synchronously
2. gBrowser._beginRemoveTab blurs the tab synchronously, this will cause onActivated
3. gBrowser.removeTab calls gBrowser._endRemoveTab. This can happen either
   • synchronously when removing without animations
   • asynchronously, at transitionend
   • asynchronously, after 3 seconds, if there was no transitionend event
4. gBrowser._endRemoveTab removes the tab for real

So, should we really delay onActivated until the tab is really removed, even if we have to wait 3 seconds?
What if meanwhile the extension queries the state of the tab, should we make it seem that the old tab is still selected?

Personally I think it may be better to just report what Firefox actually does. If the tab is blurred before being removed, onActivated should fire before onRemoved. And if this order is different than in Chrome, so be it.