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

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

[YUKI "Piro" Hiroshi]

Attachment: testcase-wrong.xpi: testcase to reproduce this problem

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

Comment 1

[YUKI "Piro" Hiroshi]

Attachment: testcase-correct.xpi: testcase to confirm the expected result

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.

Comment 2

[YUKI "Piro" Hiroshi]

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

Comment 3

[YUKI "Piro" Hiroshi]

Related issue on actual addon: https://github.com/piroor/treestyletab/issues/1355

Comment 5

[YUKI "Piro" Hiroshi]

FYI: I've published a library for the workaround.
https://github.com/piroor/webextensions-lib-tab-id-fixer

Comment 8

[dw-dev]

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.

Comment 9

[Tomislav Jovanovic :zombie]

Attachment: Bug 1398272 - Prevent onUpdated from breaking tab IDs for adopted tabs

Review commit: https://reviewboard.mozilla.org/r/231034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231034/

Comment 10

[Tomislav Jovanovic :zombie]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231034/diff/1-2/

Comment 11

[Tomislav Jovanovic :zombie]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231034/diff/2-3/

Comment 13

[Kris Maglione [:kmag]]

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.

Comment 14

[Tomislav Jovanovic :zombie]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231034/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0da93aebf9bc
Prevent onUpdated from breaking tab IDs for adopted tabs r=kmag

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0da93aebf9bc

Comment 17

[Baptiste Thémine]

Is it possible to fix this issue for the release of Firefox 60 ESR next month ?

Comment 19

[Victor Carciu]

Verified as fixed in FF 61. Also, I can confirm that it was reproducible in FF59. I will attach before and after fix screenshots.

Comment 20

[Victor Carciu]

Attachment: Before fix

Comment 21

[Victor Carciu]

Attachment: After fix

Comment 22

[Tomislav Jovanovic :zombie]

(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

[kroppy]

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

Comment 24

[Shane Caraveo (:mixedpuppy)]

It's too late to uplift this to 60.

Comment 25

[kroppy]

Is it too big of a change for 2018-06-26 ESR 60.1?

Comment 26

[Shane Caraveo (:mixedpuppy)]

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

Comment 27

[Tomislav Jovanovic :zombie]

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

Comment 28

[Julien Cristau [:jcristau]]

Not a new issue so probably not something I want to take for 60 RC2.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 30

[Tomislav Jovanovic :zombie]

Attachment: big-1398272-uplift.patch

> [Approval Request Comment]
see comment #27

Comment 31

[Julien Cristau [:jcristau]]

Comment on attachment 8975961 [details] [diff] [review]
big-1398272-uplift.patch

fix for tab management extensions in 60.1esr

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f2165acbf563