1238685 - <tabbrowser>'s mTabListeners and mTabFilters properties should be Maps rather than arrays

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Kris Maglione [:kmag]]

The logic that tries to keep these in sync with tab indices is fragile. I've had a lot of problems over the years caused by them getting out of sync, especially when add-ons are involved.

I'd like to head off potential problems before we start adding more features to the WebExtension tabs API.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Use WeakMaps for mTabListeners and mTabFilters properties in tabbrowser

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8706663 [details] [diff] [review]
Use WeakMaps for mTabListeners and mTabFilters properties in tabbrowser

Let's ask Dão, he knows this stuff better.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8706663 [details] [diff] [review]
Use WeakMaps for mTabListeners and mTabFilters properties in tabbrowser

Seems like a good idea, but let's use the more modern _fooBar naming convention (i.e. _tabListeners, _tabFilters) as we change the meaning of these properties.

I wonder if we should use Map rather than WeakMap since we need to explicitly remove the progress listeners anyway.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Kris Maglione [:kmag] from comment #0)
> The logic that tries to keep these in sync with tab indices is fragile. I've
> had a lot of problems over the years caused by them getting out of sync,
> especially when add-ons are involved.

I don't think I'm aware of these problems. Were there bugs filed on them?

Comment 5

[Kris Maglione [:kmag]]

(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 8706663 [details] [diff] [review]
> Use WeakMaps for mTabListeners and mTabFilters properties in tabbrowser
> 
> Seems like a good idea, but let's use the more modern _fooBar naming
> convention (i.e. _tabListeners, _tabFilters) as we change the meaning of
> these properties.
> 
> I wonder if we should use Map rather than WeakMap since we need to
> explicitly remove the progress listeners anyway.

I was on the fence about that. I decided on WeakMaps because that's what _tabForBrowser was using, but Maps seem fine to me too.

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to Kris Maglione [:kmag] from comment #0)
> > The logic that tries to keep these in sync with tab indices is fragile. I've
> > had a lot of problems over the years caused by them getting out of sync,
> > especially when add-ons are involved.
> 
> I don't think I'm aware of these problems. Were there bugs filed on them?

I don't know if there are any bugs that point to this as the cause. There are definitely several bugs about the behavior, though. Bug 1060332, for instance.

Once the listeners get out of sync, it becomes impossible to remove some tabs, because the removeTab code throws when it tries to remove the progress listener.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Use Maps instead of arrays to store tab listeners and filters

Comment 7

[Dão Gottwald [:dao]]

This will break add-ons using mTabListeners and mTabFilters.

Comment 8

[Kris Maglione [:kmag]]

I was hoping there wouldn't be very many add-ons touching those properties, but it looks like I was wrong. I may have to send some emails to developers.

Thanks

Comment 9

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/b6ef37a7673e9829de96d0439244343948545955
Bug 1238685: Use Maps instead of arrays to store tab listeners and filters. r=dao

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6ef37a7673e

Comment 11

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected