All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

({addon-compat})

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

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8706663 [details] [diff] [review]
Use WeakMaps for mTabListeners and mTabFilters properties in tabbrowser
(Assignee)

Updated

3 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

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

Comment 6

3 years ago
Created attachment 8707195 [details] [diff] [review]
Use Maps instead of arrays to store tab listeners and filters
Attachment #8707195 - Flags: review?(dao)
(Assignee)

Updated

3 years ago
Attachment #8706663 - Attachment is obsolete: true

Updated

3 years ago
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

3 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

3 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

3 years ago
Blocks: 508738

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6ef37a7673e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
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.