1538119 - Duplicate sessionId in browser.sessions.getRecentlyClosed() after "Restore previous session"

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[jingyu9575]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

1. Enable "Restore previous session" in preferences.
2. Open several tabs, and close them.
3. Exit and restart Firefox.
4. Open several tabs, and close them.
5. Debug an extensin with sessions permission and run:

browser.sessions.getRecentlyClosed().then(console.log)

Actual results:

The returned objects have duplicate sessionId, e.g. multiple tabs with sessionId: "0".

This makes browser.sessions.restore unreliable; restoring a tab may bring up a window or another unrelated tab instead.

Expected results:

Each returned object has unique sessionId.

---

Possible reason: when "Restore previous session" is invoked, the closed tabs and windows from the last session are brought back with the sessionId unchanged. However, the counter

Cu.import("resource:///modules/sessionstore/SessionStore.jsm").SessionStoreInternal._nextClosedId

is not adjusted accordingly. Since new closed tabs get auto-incremented sessionId from _nextClosedId, they can be duplicate with the restored ones.

This gets more complicated with the ☰ → "Restore previous session" button. If the user open and close some tabs and then click this button, it is possible for the restored tab to have duplicate sessionId with (and override) the existing ones.

I'm not sure what is the best way to fix it; maybe it is possible to adjust _nextClosedId so it is still larger after "Restore previous session", or prefix the sessionId to differentiate the current session and all the restored sessions.

Comment 1

[Victor Carciu]

I tried to use https://github.com/mdn/webextensions-examples/tree/master/session-state with latest FF66, but I was not able to reproduce the issue. Can you please attach the extension you used when reproducing this?

Comment 2

[jingyu9575]

Attachment: Reproducing the bug with Firefox 66 and session-state

I can see the bug with the session-state extension. Please see the screenshot, starting from a fresh profile on Firefox 66.

Comment 3

[Victor Carciu]

After following the steps from video I can confirm that the issue is reproducible.

Thank you.

Comment 4

[Dook]

I confirm the problem. And I was thinking why, in my extension, sometimes browser.sessions.restore restores a completely wrong tab.

{
"lastModified": 1567290681379,
"tab": {
"sessionId": "176",
"index": 14,
"windowId": 26,
"highlighted": false,
"active": false,
"pinned": false,
"hidden": false,
"incognito": false,
"lastAccessed": 1567290681360,
"url": "https://bugzilla.mozilla.org/show_bug.cgi?id=1407486",
"title": "1407486 - нет способа настроить Firefox для автоматического восстановления сеансов",
"favIconUrl": ""
}
}

{
"lastModified": 1567238855459,
"tab": {
"sessionId": "176",
"index": 98,
"windowId": 14,
"highlighted": false,
"active": false,
"pinned": false,
"hidden": false,
"incognito": false,
"lastAccessed": 1567238855412,
"url": "https://www.aliexpress.com/item/4000127675106.html?spm=a2g0o.productlist.0.0.b5131501NfQKb8&algo_pvid=2fc2b116-7159-42f8-b3a1-f4699cfc1df8&algo_expid=2fc2b116-7159-42f8-b3a1-f4699cfc1df8-29&btsid=d51ce497-7454-4d9f-88e5-7921bd893a2c&ws_ab_test=searchweb0_0,searchweb201602_6,searchweb201603_55",
"title": "50 pcs Pairs Soft Clear Silicone Replacement Eartips Earbuds Cushions Ear pads Covers For Earphone Headphone-in Earphone Accessories from Consumer Electronics on Aliexpress.com | Alibaba Group",
"favIconUrl": ""
}
}

Comment 5

[wolfsschnute]

This is still an issue with FF 72.0.1
It happens several times a day, actually every few times I want to restore a previously closed tab.

In my humble opinion, this should be on your top priority to-do list, right after any security-related issues.
"You" being those who are responsible the most for tinkering with this whole tabs/windows/sessions system.

Assign unique IDs, make it work forward and backward and sideways as flexibly as possible,
Save them in an indexed list to speed up operations,
Test! QA! Comment! Add markers so you'll be notified of any future changes or discrepancies in your code!
check and recheck and then test some more,
then pass down your work to all qualified branches..

Sorry for (maybe?) seeming a bit toxic here, but this kind of issue is purely related to two aspects: consistency and QA

Comment 7

[Manuel Reimer]

The tab data is saved to "closedTabs":
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2740
And the window data is saved to "_closedWindows"
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2167

I wonder where they get restored if the browser restarts. Does this happen inside "SessionStore.jsm"? And if so: Where?

Finding the maximum in this list and presetting _nextClosedId to the maximum value + 1 would be an easy fix if I knew where the stored session data gets placed into these arrays.

Comment 8

[Manuel Reimer]

I completely forgot that it is (was?) possible to restore a previous session into the current one. This is currently just done with "concat":
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3881
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3911

Is there still an UI for restoring a session into the current one?

Would it be possible to renumber all the restored tabs and windows session ID or are there any cross-references that could get lost in this case? I guess I could just try that and replace the "concat" here with a proper loop which creates a new ID for every single restored entry based on incrementing _nextCloseId.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Mike,
would you mind to take a look to this?
based on the comments it looks like there could be an issue on the sessionstore side (and not in the extension API layer).

Comment 10

[Shane Caraveo (:mixedpuppy)]

Marco, does comment 9 ring any bell? This looks like a bug in session store causing a duplicate id.

Comment 11

[Marco Bonardo [:mak]]

Based on the description in comment 0, it seems to be an architectural bug in how sessionId is generated, for which I think it would be a good idea to file a bug in the SR component that blocks this bug. We need a new owner for SR, but I still don't know who may be, I'd suggest to directly ping mdb on Slack to move this on.
I can't easily answer the question, because I'm not even sure why sessionId was defined as a number, and not a uuid or a guid, that would have a much smaller chance of being a dupe in case of code mistakes. Probably the fix here is about changing the sessionId on restore and not trust the old one, or immediately increase _nextClosedId, unless the architecture can be easily changed to use a likely-unique identifier.

PS: while searching the source I found a typo (sesionId) here https://searchfox.org/mozilla-central/rev/2bc4e5cdf669cc9de3ed98e38be3e50a27b25332/browser/components/extensions/test/browser/browser_ext_sessions_incognito.js#94 not sure if/how this has consequences.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit BugBot documentation.

Comment 13

[Manuel Reimer]

Is it possible that the issue here has been resolved "by accident" while bug 1789652 and bug 1845836 were fixed?

I tried with a modified version of my "Undo Close Tab" Add-on with some "console.log" entries patched in.

I did several restarts and compared the session IDs. I was no longer able to make Firefox report duplicated IDs.

Comment 14

[Rob Wu [:robwu]]

According to comment 13 this bug has been fixed, likely by bug 1789652 (Firefox 106).

I am therefore closing this bug.

Please comment with a test case if the bug has not been fixed.