Change splitviewIds to use a safe integer
Categories
(Firefox :: Tabbed Browser: Split View, task, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox149 | --- | verified |
People
(Reporter: sclements, Assigned: sfoster)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fidefe-splitview])
Attachments
(1 file)
We copied over what tab groups had done for ID generation, but Rob raised this as something we should fix to avoid conflict ids that could impact extensions in particular. Since we have this persisted with session restore we'll need to look into a migration for this.
Some suggestions from a conversation with Rob:
In the public extension API is it a number, so if the internal value is always a number, it may as well be represented internally as a number to avoid string <-> number translations in places
For ID generation, I was thinking of this:
Collect all split view IDs from the current session.Keep generating a new random integer until it is not one from the setreturn it.
Under the assumption that we don't generate many split views in a browser session, the "leak" of integers would be acceptable.
An alternative is to use an incremental counter, but the downside to that is that at some point we'd hit a high number that overflows, or that the number could reveal how intensively the API had been used. These issues don't sound too bad tbh (provided that we have a way to deal with a too high split view id).
If the number value were to not be a requirement, generating a UUID would have been enough, but that would complicate the mapping in the extension API (especially if the value is expected to be persistent)
|
||
Updated•6 months ago
|
|
||
Updated•6 months ago
|
|
||
Comment 1•6 months ago
|
||
(for visibility: we started persisting split view IDs to session store in bug 1995818; while that has not reached release yet, it is on Nightly)
|
Assignee | |
Comment 2•6 months ago
|
||
(In reply to Sarah Clements [:sclements] from comment #0)
We copied over what tab groups had done for ID generation, but Rob raised this as something we should fix to avoid conflict ids that could impact extensions in particular. Since we have this persisted with session restore we'll need to look into a migration for this.
Some suggestions from a conversation with Rob:
In the public extension API is it a number, so if the internal value is always a number, it may as well be represented internally as a number to avoid string <-> number translations in places
Where does this get defined and handled? I don't see anything split-view related in browser/components/extensions/ currently. Making the splitViewId a number sounds fine to me, I just want to understand the impact of the change and what a migration might look like - if its needed at all.
For ID generation, I was thinking of this:
Collect all split view IDs from the current session.Keep generating a new random integer until it is not one from the setreturn it.
The only problem with this is the deferred session restore case. So we could generate an integer that was unique for the current session, but there's a (small) risk that it collides with an id once we restore a saved session.
An alternative is to use an incremental counter, but the downside to that is that at some point we'd hit a high number that overflows, or that the number could reveal how intensively the API had been used.
Number.MAX_SAFE_INTEGER is a very large number. Its many quadrillion before we have to worry about overflow. The only problem is that the running "next id" number needs to be persisted to the profile somewhere for continuity between sessions. I'm not sure if we have existing incrementing integer ids like this whose implementation we can emulate?
|
|
||
Comment 3•6 months ago
|
||
(In reply to Sam Foster [:sfoster] (he/him) from comment #2)
(In reply to Sarah Clements [:sclements] from comment #0)
We copied over what tab groups had done for ID generation, but Rob raised this as something we should fix to avoid conflict ids that could impact extensions in particular. Since we have this persisted with session restore we'll need to look into a migration for this.
Some suggestions from a conversation with Rob:
In the public extension API is it a number, so if the internal value is always a number, it may as well be represented internally as a number to avoid string <-> number translations in places
Where does this get defined and handled? I don't see anything split-view related in browser/components/extensions/ currently. Making the
splitViewIda number sounds fine to me, I just want to understand the impact of the change and what a migration might look like - if its needed at all.
Work in progress locally, but I am reading tab.splitview.splitViewId to get the ID. Currently it is a string, but ideally it should be an integer to avoid any conversions. Without a fix, I would have to use something similar to the groupId logic like https://searchfox.org/firefox-main/rev/133582f487ff8291ec10bd524db52db0b8ed363e/browser/components/extensions/parent/ext-browser.js#138-179 (this looks like a giant hack and is something I'd rather avoid)
For ID generation, I was thinking of this:
Collect all split view IDs from the current session.Keep generating a new random integer until it is not one from the setreturn it.The only problem with this is the deferred session restore case. So we could generate an integer that was unique for the current session, but there's a (small) risk that it collides with an id once we restore a saved session.
That is why the set of in-memory IDs need to include all IDs that are known (active tabs, session restore, and tabs that have already been closed)
An alternative is to use an incremental counter, but the downside to that is that at some point we'd hit a high number that overflows, or that the number could reveal how intensively the API had been used.
Number.MAX_SAFE_INTEGERis a very large number. Its many quadrillion before we have to worry about overflow. The only problem is that the running "next id" number needs to be persisted to the profile somewhere for continuity between sessions. I'm not sure if we have existing incrementing integer ids like this whose implementation we can emulate?
An example of that would be userContextId (for container tabs) is based on a counter, and the counter is stored together with the file that contains the container tab mappings: https://searchfox.org/firefox-main/rev/133582f487ff8291ec10bd524db52db0b8ed363e/toolkit/components/contextualidentity/ContextualIdentityService.sys.mjs#277
I guess that you could store it together with the session store data (assuming that it contains all data relevant to split views). Other storage mechanisms such as prefs are also possible but less reliable (can go out of sync if people fiddle with files in the profile).
|
|
Assignee | |
Comment 4•6 months ago
|
||
- Keep a running count of the next integer value for splitview ids
- Examine previous session state for maxSplitViewId at statup to avoid id conflicts
- Migrate string ids in saved session data before restoring windows and tabs
- Define the new properties on the session restore JSON schema, fix closed tabs subschema and update test data
|
|
||
Comment 5•6 months ago
|
||
For future reference, Stephen asked whether the IDs values need to persist across restarts. They should (but not must) persist across restarts where relevant (e.g. session restore). The relevant WECG discussions and specification is here:
Backed out for causing mochitest failure @browser_switchTab_override.js
|
|
Assignee | |
Comment 9•6 months ago
|
||
(In reply to pstanciu from comment #8)
Backed out for causing mochitest failure @browser_switchTab_override.js
Looks like I crossed the streams with Bug 2013464. I fixed that test to pass an integer id and re-pushed.
|
||
Comment 10•6 months ago
|
||
|
||
Comment 11•6 months ago
|
||
| bugherder | ||
|
||
Comment 12•6 months ago
|
||
Hello! Do you think there is something we can manually verify here? Thank you!
|
|
Assignee | |
Comment 13•6 months ago
|
||
(In reply to Alexandru Trif, Desktop Test Engineering [:atrif] from comment #12)
Hello! Do you think there is something we can manually verify here? Thank you!
This was mostly to support upcoming web extension api work. You can verify that split views continue to be restored when "Open previous windows and tabs" is enabled.
|
|
||
Comment 14•6 months ago
|
||
(In reply to Sam Foster [:sfoster] (he/him) from comment #13)
(In reply to Alexandru Trif, Desktop Test Engineering [:atrif] from comment #12)
Hello! Do you think there is something we can manually verify here? Thank you!
This was mostly to support upcoming web extension api work. You can verify that split views continue to be restored when "Open previous windows and tabs" is enabled.
Thank you! Confirming that Split Views are correctly restored across windows when "Open previous windows and tabs" is enabled with Firefox 149.0a1 (2026-02-16) on Windows 11, macOS 26 and Ubuntu 24.
|
|
||
Comment 15•6 months ago
|
||
Did you test with a browser session created on a build before the patch (but after bug 1995818) and with session restore from a build after the patch to this bug? That would exercise the migration logic.
|
|
||
Comment 16•6 months ago
|
||
(In reply to Rob Wu [:robwu] from comment #15)
Did you test with a browser session created on a build before the patch (but after bug 1995818) and with session restore from a build after the patch to this bug? That would exercise the migration logic.
Hello! I did not initially, but I have now: I opened Firefox 149.0a1 (2026-02-11), which was before this bug and after bug 1995818, enabled session restore, and made some Split Views across windows and within groups. Then I have updated Firefox to the latest build, and all Split Views are restored. Tested on Windows 11, macOS 26 and Ubuntu 24. Please let me know if I should look for anything else. Thank you!
Description
•