tabs.onUpdated listener triggers broken consistency of tabs.Tab.id, by a tab moved across multiple windows

VERIFIED FIXED in Firefox -esr60

Status

defect
P2
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: yuki, Assigned: zombie)

Tracking

Trunk
mozilla61
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr6061+ fixed, firefox57 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 verified)

Details

Attachments

(6 attachments)

Reporter

Description

2 years ago
When an addon registers any listener for browser.tabs.onUpdated, a tab moved across windows gets new id unexpectedly.

Steps to reproduce:
1. Install any addon which registers browser.tabs.onUpdated listener.
2. Open debug console for the addon and select its "Console" pane.
3. Open two browser windows, A and B.
4. Open a new tab in the window A, and click it to activate.
5. Get the id of the tab. For example:
   ------------------------------------------
   browser.tabs.query({
     active: true,
     currentWindow: true
   }).then(t=>console.log(t[0].id))
   ------------------------------------------
6. Drag the tab and drop into the tab bar of the window B.
7. Click the dropped tab to activate.
8. Get the id of the tab again, by something way like the step 5.

Actual result:
It reports a new id different from the one before the tab is moved.

Expected result:
It reports a consistent id same to the one before the tab is moved.

Additional information:
 * This problem doesn't happen if the addon doesn't register any listener
   for tabs.onUpdated.
 * This problem affects only in the namespace of the addon which registers
   any listener for tabs.onUpdated. Other addons still get consistent id
   for the moved tab.
 * tabs.onActivated for the tab gets correct consistent id as its first
   argument.
 * If you see this problem, the result of browser.tabs.get(consistent id)
   returns a tab object with wrong (incremented) id.
 * I've confirmed this on: Nightly 57.0a1 20170908100218 (e10s-enabled)
 * I couldn't reproduce this problem on Firefox ESR52 (e10s-disabled).
Reporter

Comment 1

2 years ago
These testcase have simple background.js:
-------------------------------------------------
// only "testcase-wrong.xpi" has this listener.
browser.tabs.onUpdated.addListener((aTabId, aChangeInfo, aTab) => {
  console.log(`wrong/tabs.onUpdated window${aTab.windowId}-tab${aTabId}`);
});

// following listeners are common.
browser.tabs.onAttached.addListener((aTabId, aAttachInfo) => {
  console.log(`wrong/tabs.onAttached window${aAttachInfo.newWindowId}-tab${aTabId}`);
});

browser.tabs.onActivated.addListener(aActiveInfo => {
  console.log(`wrong/tabs.onActivated window${aActiveInfo.windowId}-tab${aActiveInfo.tabId}`);
});
-------------------------------------------------

If you install both testcases at same time, you'll see a result that only "wrong" case reports incremented id for the moved tab when you repeatedly move a tab from one to another.
Reporter

Comment 2

2 years ago
Summarized logs on my environment with these two testcases:

When I click a tab on the window A:
> correct/tabs.onActivated window3-tab2
> wrong/tabs.onActivated window3-tab2

When I move the tab to the window B:
> wrong/tabs.onUpdated window12-tab5 <= wrong id
> correct/tabs.onAttached window12-tab2
> wrong/tabs.onAttached window12-tab2
> correct/tabs.onActivated window12-tab2
> wrong/tabs.onActivated window12-tab2
> correct/tabs.onActivated window12-tab2
> wrong/tabs.onActivated window12-tab2
> correct/tabs.onActivated window12-tab2
> wrong/tabs.onActivated window12-tab2

When I move the tab to the window A again:
> wrong/tabs.onUpdated window3-tab7 <= wrong id
> correct/tabs.onAttached window3-tab2
> wrong/tabs.onAttached window3-tab2
> correct/tabs.onActivated window3-tab2
> wrong/tabs.onActivated window3-tab2
> correct/tabs.onActivated window3-tab2
> wrong/tabs.onActivated window3-tab2
> correct/tabs.onActivated window3-tab2
> wrong/tabs.onActivated window3-tab2
Reporter

Comment 3

2 years ago
Related issue on actual addon: https://github.com/piroor/treestyletab/issues/1355
Assignee: nobody → kmaglione+bmo

Updated

2 years ago
Priority: -- → P2
Duplicate of this bug: 1402742
Reporter

Comment 5

a year ago
FYI: I've published a library for the workaround.
https://github.com/piroor/webextensions-lib-tab-id-fixer
Duplicate of this bug: 1427094
Duplicate of this bug: 1426872

Comment 8

a year ago
I think that Bug 1440015, which I have recently reported, may be a duplicate of this bug.

I can confirm that Bug 1440015 only happens if there is a tabs.onUpdated() listener.

For my Tile Tabs WE add-on, this bug is a big deal, breaking key functionality.

The 'tabId' changing, when a tab is moved between windows, is a serious error - and should be a high priority to fix.
Assignee: kmaglione+bmo → tomica
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Duplicate of this bug: 1440015
Assignee

Updated

a year ago
Attachment #8962198 - Flags: review?(kmaglione+bmo)

Comment 13

a year ago
mozreview-review
Comment on attachment 8962198 [details]
Bug 1398272 - Prevent onUpdated from breaking tab IDs for adopted tabs

https://reviewboard.mozilla.org/r/231034/#review236490

Thanks!

::: browser/components/extensions/test/browser/browser_ext_tabs_move_window.js:20
(Diff revision 3)
>      async background() {
>        let tabs = await browser.tabs.query({url: "<all_urls>"});
>        let destination = tabs[0];
>        let source = tabs[1]; // skip over about:blank in window1
>  
> +      browser.tabs.onUpdated.addListener(dummy => {

Nit: s/dummy/()/ please.
Attachment #8962198 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)

Comment 15

a year ago
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0da93aebf9bc
Prevent onUpdated from breaking tab IDs for adopted tabs r=kmag

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0da93aebf9bc
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is it possible to fix this issue for the release of Firefox 60 ESR next month ?

Updated

a year ago
Duplicate of this bug: 1423705

Comment 19

a year ago
Verified as fixed in FF 61. Also, I can confirm that it was reproducible in FF59. I will attach before and after fix screenshots.
Status: RESOLVED → VERIFIED

Comment 20

a year ago
Posted image Before fix

Comment 21

a year ago
Posted image After fix
Assignee

Comment 22

a year ago
(In reply to Baptiste Thémine from comment #17)
> Is it possible to fix this issue for the release of Firefox 60 ESR next
> month ?

Hi Baptiste, this is possible to uplift to 60, but it needs a clear reason to do so.  If you have one, feel free to nominate for uplift (click on attachment details, flag approval-mozilla-beta? and fill in the form), and release drivers can decide as appropriate.

Comment 23

a year ago
> Hi Baptiste, this is possible to uplift to 60, but it needs a clear reason to do so.  If you have one, feel free to nominate > for uplift (click on attachment details, flag approval-mozilla-beta? and fill in the form), and release drivers can decide as > appropriate.

It would be good to have it in 60 ESR, as Baptiste asked.
It's quite critical for web extensions to keep tabId consistency. And keeping this bug for the next year would be bad.

I don't see any "flag approval-mozilla-beta", when I click on details. Where is it? I see only form and submit button. Is it something that comes after submit?
It's too late to uplift this to 60.

Comment 25

a year ago
Is it too big of a change for 2018-06-26 ESR 60.1?
It is a simple fix, had I noticed i would have requested it last week.

TBH I'm uncertain how to uplift into any ESR point release, but that is a good question.  Lets see if we can find an answer...
Flags: needinfo?(sescalante)
Assignee

Comment 27

a year ago
Comment on attachment 8962198 [details]
Bug 1398272 - Prevent onUpdated from breaking tab IDs for adopted tabs

Since it's late in the beta cycle, i'm nominating for esr60 directly:

> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is a simple one-line fix that would be good to fix for ESR.

> User impact if declined: 
Any tab-managing extension is partially broken when tabs are moved between windows.

> Fix Landed on Version:
Nightly while it was at version 61

> Risk to taking this patch (and alternatives if risky): 
Minimal risk, it's a simple patch that is fully understood and well tested.

> String or UUID changes made by this patch: 
none
Attachment #8962198 - Flags: approval-mozilla-esr60?
Attachment #8962198 - Flags: approval-mozilla-beta?
Not a new issue so probably not something I want to take for 60 RC2.
Attachment #8962198 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8962198 [details]
Bug 1398272 - Prevent onUpdated from breaking tab IDs for adopted tabs

FYI, this patch doesn't graft cleanly to ESR60. Please attach a rebased patch and re-request approval.
Flags: needinfo?(sescalante) → needinfo?(tomica)
Attachment #8962198 - Flags: approval-mozilla-esr60?
Assignee

Comment 30

a year ago
> [Approval Request Comment]
see comment #27
Flags: needinfo?(tomica)
Attachment #8975961 - Flags: approval-mozilla-esr60?
Comment on attachment 8975961 [details] [diff] [review]
big-1398272-uplift.patch

fix for tab management extensions in 60.1esr
Attachment #8975961 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.