Closed Bug 1238685 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

(Keywords: addon-compat)

Attachments

(1 file, 1 obsolete file)

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.
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?
(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.
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
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
https://hg.mozilla.org/integration/fx-team/rev/b6ef37a7673e9829de96d0439244343948545955
Bug 1238685: Use Maps instead of arrays to store tab listeners and filters. r=dao
Blocks: 508738
https://hg.mozilla.org/mozilla-central/rev/b6ef37a7673e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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.