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

RESOLVED FIXED in mozilla51

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: u576804, Assigned: fiveNinePlusR, Mentored)

Tracking

49 Branch
mozilla51

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: tabs, triaged)

Attachments

(1 attachment, 1 obsolete attachment)

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.
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
I guess we should probably just filter out tabs where `tab.closing` is true.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ok, I think I can tackle this one if you want to assign it to me.
Sure. Thanks!
Assignee: nobody → fiveNinePlusR
Mentor: kmaglione+bmo
Status: NEW → ASSIGNED
Assignee: u576804 → fiveNinePlusR
Priority: -- → P2
Whiteboard: tabs, triaged
Duplicate of this bug: 1293485
Posted patch bug1291830_patch_1.diff (obsolete) — Splinter Review
Attachment #8779574 - Flags: review?(kmaglione+bmo)
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.
Attachment #8779574 - Flags: review?(kmaglione+bmo) → review+
Fixes the coding style issue raised in review.
Attachment #8779574 - Attachment is obsolete: true
Attachment #8779896 - Flags: review?(kmaglione+bmo)
Attachment #8779896 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/32eb078cd5dd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1301089
This bug still occurs. I tested on Firefox 54 and Nightly 56.
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
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.
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
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);
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.
Ok thanks, I've created a new bug https://bugzilla.mozilla.org/show_bug.cgi?id=1409540
Product: Toolkit → WebExtensions
See Also: → 1513220
You need to log in before you can comment on or make changes to this bug.