1316126 - `_endRemoveTab` can get stuck in an infinite loop

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

A `TabClose` event listener can cause `_endRemoveTab` to become stuck in a infinite loop.

Looking at the general flow of removing tabs:

1. `_beginRemoveTab` is called
 a. The tab is added to `this._removingTabs` array
 b. `TabClose` event is emitted
 c. `aTab._endRemoveArgs` is set
2. `_endRemoveTab` is called
 a. If `!aTab || !aTab._endRemoveArgs`, abort
 b. Remove the tab from `this._removingTabs` array
 c. If `aCloseWindow`, loop over all tabs in `this._removingTabs` array and call `_endRemoveTab`

The issue I saw is that listeners for `TabClose` can trigger arbitrary behavior, including removing a second tab, leading to follow timeline of actions:

1. `_beginRemoveTab` is called for tab 1
2. Tab 1 is added to `this._removingTabs` array
  * NOTE: `_endRemoveArgs` has _not_ been set for tab 1 yet
3. `TabClose` event emitted for tab 1
4. Some `TabClose` listener calls `removeTab(tab2)`
5. `_beginRemoveTab` is called for tab 2
6. `_endRemoveTab` is called for tab 2, close window is true
7. Loop over all tabs in `this._removingTabs` array and call `_endRemoveTab`
  * Here we loop forever, because the `this._removingTabs` contains a tab without `_endRemoveArgs` so it just returns immediately

I tried to adjust the timing of when `_endRemoveArgs` is set, but I couldn't find the precise point to do so without its own negative effects.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It can be challenging to trigger this by just using the browser since it is timing dependent.  I'll try to prepare a test case that triggers the issue.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1316126 - Example test with _endRemoveTab infinite loop

Review commit: https://reviewboard.mozilla.org/r/91546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91546/

Comment 3

[:Gijs (he/him)]

Could we just default the _endRemoveArgs to something sane ([false, false], I suppose) if we're calling _endRemoveTab and _endRemoveArgs is not set? If this is only called when we're using closeWindow=true, that seems sane?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)
> I tried to adjust the timing of when `_endRemoveArgs` is set, but I couldn't
> find the precise point to do so without its own negative effects.

If we do this right before firing the TabClose event, what negative effects happen?

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This has fallen out of my queue, unfortunately.  Hopefully I can get back to this in the future.

Comment 5

[Dão Gottwald [:dao]]

Is this a regression?

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Dão Gottwald [::dao] from comment #5)
> Is this a regression?

I don't think so...?  I believe I was hitting an edge case by calling `removeTab` from within a `TabClose` event handler, which may not have been anticipated.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Bug 1391704 changed this code to set `_endRemoveArgs` earlier (before emitting TabClose), which should fix this issue as I understand it.

(I don't have an easily accessible test case right now...  I can re-open if I later find this was inaccurate.)