Closed
Bug 1316126
Opened 8 years ago
Closed 7 years ago
`_endRemoveTab` can get stuck in an infinite loop
Categories
(Firefox :: Tabbed Browser, defect, P3)
Tracking
()
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jryans, Unassigned)
References
Details
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
Details |
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.
Reporter | ||
Comment 1•8 years ago
|
||
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 hidden (mozreview-request) |
Updated•8 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 3•8 years ago
|
||
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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jryans)
Reporter | ||
Comment 4•8 years ago
|
||
This has fallen out of my queue, unfortunately. Hopefully I can get back to this in the future.
Flags: needinfo?(jryans)
Comment 5•8 years ago
|
||
Is this a regression?
Flags: needinfo?(jryans)
Keywords: regressionwindow-wanted
Reporter | ||
Comment 6•8 years ago
|
||
(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.
Flags: needinfo?(jryans)
Updated•7 years ago
|
Keywords: regressionwindow-wanted
Priority: -- → P3
Reporter | ||
Comment 7•7 years ago
|
||
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.)
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Depends on: 1391704
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in
before you can comment on or make changes to this bug.
Description
•