633158 - _numPinnedTabs should exclude _removingTabs

Status: NEW

(Firefox :: Tabbed Browser, defect)

Description

[tabmix.onemen]

_numPinnedTabs return 
the number of pinned tab without excluding _removingTabs

it can lead to unexpected result in some cases for example:
since visibleTabs contain all visible tabs without _removingTabs then
let tab = this.tabbrowser.visibleTabs[this.tabbrowser._numPinnedTabs] can be null if _removingTabs is grater then visibleTabs.length.
also we can get different tab then the one we want.

var tabsToClose = (aAll ? this.tabs.length : this.visibleTabs.length - 1)
                            - gBrowser._numPinnedTabs;
tabsToClose is obviously not the correct number if _numPinnedTabs contain removingTabs

Comment 1

[Adam]

Attachment: WIP: Simple patch, no test.

No test yet; input on building a test would be appreciated, as my understanding is that the problems related to this will only occur if you get _numPinnedTabs while the browser is still clearing some pinned tabs in _removingTabs, which is timing dependent?

I haven't tested the patch, other than running the browser with it, for the same reason.

Comment 2

[Dão Gottwald [:dao]]

Doing this would break tabbrowser.tabs[tabbrowser._numPinnedTabs]...

Note that pinned tabs are removed synchronously, so this affects only _numPinnedTabs users that respond to events that are dispatched when closing a pinned tab (e.g. TabClose, possibly TabOpen / TabSelect).

Comment 3

[Adam]

Attachment: WIP v2: Split to _numVisiblePinnedTabs and modify users of _numPinnedTabs as needed.

Thanks for the feedback.  Based on that, it looks like there is need for a separate property, this patch calls it _numVisiblePinnedTabs.  Might need a better name than _numVisiblePinnedTabs, though the name does somewhat indicate it's the counterpart to visibleTabs.

Passes all tests that are passed without it.

Comment 4

[Dão Gottwald [:dao]]

It's not clear to me what this bug is concerned about, exactly. Why would TabContextMenu.updateContextMenu be called while a pinned tab is being removed? Same question for gBrowser.warnAboutClosingTabs.

Comment 5

[Adam]

Thanks for the feedback and patience.  I was tilting at a windmill here.  Sorry for that.

I was still under the impression that there could be cases where having a separate count of pinned tabs would be useful.  But, as you said in Comment 2, the only cases would be a small number of event consumers.

Having the separate property for just those few cases doesn't gain enough to justify the additional code when it's easy enough for users to handle it themselves.

Some kind of auto-browser extension might call updateContextMenu under a handler, but I doubt it would call warnAboutClosingTabs, and although it might trigger the same, it would probably disable it first.  In either case, it could handle the cleanup itself.