1291830 - chrome.tabs.onRemove does not remove tab before calling the callback function

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[u576804]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160727004019

Steps to reproduce:

subscribe to the tabs.onRemoved event and supplied a callback function that queries the chrome.tabs.query() method to get a list of all tabs.


Actual results:

The tabs included the removed tab.


Expected results:

I expect the tab to not be in the list of tabs at this point in the tab's lifecycle.

Comment 1

[Kris Maglione [:kmag]]

I guess we should probably just filter out tabs where `tab.closing` is true.

Comment 2

[u576804]

Ok, I think I can tackle this one if you want to assign it to me.

Comment 3

[Kris Maglione [:kmag]]

Sure. Thanks!

Comment 5

[fiveNinePlusR]

Attachment: bug1291830_patch_1.diff

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8779574 [details] [diff] [review]
bug1291830_patch_1.diff

Review of attachment 8779574 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the unused code removed.

::: browser/components/extensions/test/browser/browser_ext_tabs_events.js
@@ +263,5 @@
> +    });
> +
> +    let url = "http://example.com/browser/browser/components/extensions/test/browser/context.html";
> +    Promise.all([browser.windows.getCurrent()]
> +    ).then(windows => {

No need for `Promise.all`. In fact, no need for `getCurrent()` either, since you don't actually use the result.

Comment 7

[fiveNinePlusR]

Attachment: bug1291830_patch_1.diff

Fixes the coding style issue raised in review.

Comment 8

[Kris Maglione [:kmag]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81c1de3306b5

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32eb078cd5dd
Do not return tabs that are in the process of closing from a `chrome.tabs.query` call. r=kmag

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32eb078cd5dd

Comment 12

[Amy Tsay [:amyt]]

Thanks! I've added this contribution here: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#September_2016

Comment 13

[fx4waldi]

This bug still occurs. I tested on Firefox 54 and Nightly 56.

Comment 14

[Sinan]

I can confirm this bug is not resolved. There is also another problem which might be a whole new bug, but not sure, I try to describe:

Have some code that subscribes to tabs.onCreated and tabs.onRemoved. Now, close the last tab with index i and very quickly open a new one (Ctrl+W, Ctrl+T). The tabInfo.index in the onCreated callback has the value i+1, whereas it must be i.

I think that's the reason for a bug in the add-on tabcenter-redux: https://github.com/eoger/tabcenter-redux/issues/124

Comment 15

[fiveNinePlusR]

In general, I think the way to report this is in a new bug and to reference this old bug in there for additional info. It helps the developers track their time more effectively and cuts out the prior discussion that is usually not relevant.

Comment 16

[zepeuj]

Hello,

The problem is not fixed at all.

It appears in versions 56, 57 and in the Nightly...

It's really annoying when you need to update the tabs after one has been closed.

Is there someone working on this issue? Can we change the irritating status "RESOLVED FIXED".

Thanks

Comment 17

[zepeuj]

As simple sample if needed:

const onRemovedTab = (removedTabId, removeInfo) => {

	console.log("Tab removed: " + removedTabId);

	const queryOpenedTabs = windowId => {

		chrome.tabs.query({ windowType: "normal", windowId: windowId }, openedTabs => {

			let tabIndex = null;

			for (const openedTab of openedTabs) {
				if (openedTab.id === removedTabId) {
					console.error("removed tab still there!!");
					tabIndex = openedTab.index;
					break;
				}
			}

			//Removed tab is still there, check in session with index
			if (tabIndex) {
				const gettingSessions = browser.sessions.getRecentlyClosed({ maxResults: 1 });
				gettingSessions.then(session => {
					if (session[0].tab) {
						if (session[0].tab.index === tabIndex) {
							console.log("As expected, removed tab is in the object session.");
						}
					}

				});
			}
			else {
				console.log("removed tab is gone.");
			}

		});
	};

	// the setTimeout is used for test to increase the timeout and see how many time to wait...
	setTimeout(() => queryOpenedTabs(removeInfo.windowId), 50);
};

chrome.tabs.onRemoved.addListener(onRemovedTab);

Comment 18

[fiveNinePlusR]

It definitely was fixed a year ago but it seems that the structure of the code got changed and now there is apparently a regression. I think you should file a new bug as that's the common practice.

Comment 19

[zepeuj]

Ok thanks, I've created a new bug https://bugzilla.mozilla.org/show_bug.cgi?id=1409540