1960104 - Improve tab group ID generation in addTabGroup

Status: NEW

(Firefox :: Tabbed Browser: Tab Groups, task, P1)

Description

[Rob Wu [:robwu]]

Currently, the internal tab group ID is a randomly generated as follows by addTabGroup in browser/components/tabbrowser/content/tabbrowser.js:

        id = `${Date.now()}-${Math.round(Math.random() * 100)}`;

It is very likely sufficiently unique, but in theory there is a chance of it not being unique. When tab groups are automatically created (e.g. via extensions), the odds of generating multiple groups in the same millisecond increases. And if clock skew happens, that makes it even likelier.

This issue could be addressed by an improved ID generation mechanism. For the WebExtensions use case, a safe integer would be ideal, because that is the value exposed in extension APIs (groupId - bug 1959713, also in tabGroups API - bug 1940631). Independently of whether the value is a string or integer, it would be really useful for internal consumers (and the future tabGroups extension API) if there is a central registry that tells whether an ID is valid and/or existing, as such a registry can be used to verify whether a randomly generated ID is truly unique.

One complication with turning it into a number is that the value is persisted to disk via session store files and saved sessions. It is currently also used as a literal DOM ID for <tab-group> elements.

Comment 1

[Rob Wu [:robwu]]

I created a unit test that creates a few new groups next after each other and hit this issue on a run of the test, judging by the fact that two distinct tab groups had the same ID.

If I can hit it in a unit test, then extensions can hit this issue in the wild.

Comment 2

[Mauro V [:cheff]]

I wonder why UUIDv4 hasn't been used for this case? Using Services.uuid.generateUUID()

Comment 3

[Rob Wu [:robwu]]

The splitview feature copied the same flawed logic for ID generation, unfortunately: https://searchfox.org/firefox-main/rev/ab8a90487516e59b3ac3b5cc503d024a2be8ea80/browser/components/tabbrowser/content/tabbrowser.js#3372-3374

Comment 4

[Sarah Clements [:sclements]]

(In reply to Rob Wu [:robwu] from comment #3)

The splitview feature copied the same flawed logic for ID generation, unfortunately: https://searchfox.org/firefox-main/rev/ab8a90487516e59b3ac3b5cc503d024a2be8ea80/browser/components/tabbrowser/content/tabbrowser.js#3372-3374

Filed bug 2014505 to deal with splitview's use of it.