1641345 - Relationship among <browser>, tabmail tab, and web-ext tabId is broken for shared messagepane tabs.

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[alta88]

Aside from the root problem of the original tabmail implementation, Bug 1455471 did not account at all for a shared #messagepane among tabs and tabIds. Bug 1627556 and Bug 1628245 fixed some things but did not go all the way.

One place to see this is the uBlock extension. Any folder or message type tab opened will break the updating of the toolbar badge, after having switched among these tab types. The _browsers WeakMap keys are wrong for the tabId, being identical and all.

Comment 1

[alta88]

Attachment: tabs2.patch

one way to fix it. not sure why a WeakMap reference to browser elements is used in Fx, but it seems pointless in Tb. the "permanentKey" part in tabmail.js can likely be removed too.

Comment 2

[alta88]

Attachment: tabs3.patch

also clean up on tab close and make sure tab moves (event not emitted) do not break anything by using a session unique tabInfo.tabId rather than tabIndex.

Comment 3

[Geoff Lankow (:darktrojan)]

Comment on attachment 9152555 [details] [diff] [review]
tabs3.patch

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

Okay, this makes sense and is fine with me as long as it doesn't break anything.

Comment 4

[alta88]

Attachment: firstTab.patch

1. remove permanentKey, the only usage was for the WeakMap for we purposes.
2. add a progresslistener to messagepane in firstTab. one is added on all subsequent shared messagepane tab types, so unless one of those is opened, messagepane never gets one. seems wrong.

The second patch has a slight risk; perhaps a try for both would be good, thanks.

Comment 5

[alta88]

For the first patch.

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9153150 [details] [diff] [review]
firstTab.patch

`permanentKey` was added for Marionette. Without it, none of our Marionette or Mochitest tests run.

I think you're right about the second part. I don't *think* it will cause any tests to break, but I will send this all to Try to find out.

Comment 7

[alta88]

I can put it back, but are you sure? It was linkedBrowser that was added for the tests. The permanentKey would only be necessary if browser/linkedBrowser were being used as WeakMap keys I believe..

Comment 8

[Geoff Lankow (:darktrojan)]

Yes I'm sure.

The try run is looking good. There's one failure I've never seen before but I don't think it's related. Re-running that just to be sure.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0a04d2187ccd
Relationship among <browser>, tabmail tab, and web-ext tabId is broken for shared messagepane tabs. r=darktrojan

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7d51695139a7
followup to fix lint of 0a04d2187ccd. rs=eslint DONTBUILD

Comment 11

[alta88]

Attachment: firstTab.patch

add back permanentKey.

Comment 12

[alta88]

Attachment: firstTab.patch

Comment 13

[Geoff Lankow (:darktrojan)]

Comment on attachment 9153401 [details] [diff] [review]
firstTab.patch

I'm pretty sure the failure I saw on Try was a red herring.

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9152555 [details] [diff] [review]
tabs3.patch

[Triage Comment]
Approved for beta (am assuming you consider this to be low risk)

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9153401 [details] [diff] [review]
firstTab.patch

[Triage Comment]
Approved for beta (am assuming you consider this to be low risk)

Comment 16

[alta88]

Thanks, yes both are very low risk.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c79bee7bdfcf
Fix up firstTab browser; it is no longer stored in a WeakMap (for web ext), ensure it gets a listener. r=darktrojan

Comment 18

[Rob Lemley [:rjl]]

Thunderbird 78.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/c84786d3659a
https://hg.mozilla.org/releases/comm-beta/rev/62898a2e9167
https://hg.mozilla.org/releases/comm-beta/rev/f6738b7e63e2