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

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

({addon-compat})

unspecified
Firefox 46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
Attachment #8706663 - Flags: review?(ttaubert)
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.
Attachment #8706663 - Flags: review?(ttaubert) → review?(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.
Attachment #8706663 - Flags: review?(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?
Assignee

Comment 5

4 years ago
(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.
Assignee

Updated

4 years ago
Attachment #8706663 - Attachment is obsolete: true
Attachment #8707195 - Flags: review?(dao) → review+
This will break add-ons using mTabListeners and mTabFilters.
Keywords: addon-compat
Summary: <tabbrowser>'s mTabListeners and mTabFilters properties should be WeakMaps rather than arrays → <tabbrowser>'s mTabListeners and mTabFilters properties should be Maps rather than arrays
Assignee

Comment 8

4 years ago
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
Assignee

Comment 9

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

Updated

4 years ago
Blocks: 508738

Comment 10

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6ef37a7673e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 11

3 years ago
[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
You need to log in before you can comment on or make changes to this bug.