1603144 - Use crypto.randomUUID() or Services.uuid.generateUUID() instead of nsIUUIDGenerator.generateUUID() in remote protocol

Status: RESOLVED WORKSFORME

(Remote Protocol :: Agent, task, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Various JS modules make use of the uuid feature in the near future. All have to request the nsIUUIDGenerator service and run the following code:

      const id = uuidGen
        .generateUUID()
        .toString()
        .slice(1, -1);

To not have to duplicate the code all over the place it would be great to have a shared method.

Andreas, any idea for a good module name, or where it could be placed?

Comment 1

[Andreas Tolfsen ❲:ato❳]

What is the problem with returning the UUID with the enclosing { and } characters?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

What is the problem with returning the UUID with the enclosing { and } characters?

I don't have a problem with that. The code above is just a copy and paste from our existing code base. Happy to have them included. Then we could do it similar to devtools, which has shared module:

https://searchfox.org/mozilla-central/source/devtools/shared/generate-uuid.js

The question then is do we need .toString() or not.

My proposal for the location would be remote/shared/generate-uuid.jsm.

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

We may as well move all or most of our string IDs to UUIDs as part of this, too.

Comment 4

[Andreas Tolfsen ❲:ato❳]

Good feedback!

A quick look around DevTools and in other places where nsIUUIDGenerator is used from JSMs shows a lot of repetetive service getters and nsIDPtr to JS string coercions. This is presumably so that the UUID can be serialised over JSON because nsIDPtr lacks a toJSON() function in JS.

In many places we also see JS code trimming off the encapsulating { and } characters, and we end up with code looking a lot like this:

function makeUUID() {
  const uuidGen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
  const uuid = uuidGen.generateUUID().toString().slice(1, -1);
}

return JSON.stringify({
  id: makeUUID(),
  …
});

I wonder if we can improve the current situation and offer something a little more idiomatic to JS consumers?

I can think of two approaches:

1. We change the nsIUUIDGenerator API so it returns a type that is serialisable with JSON.stringify in JS

   This may not be workable because of API constraints and the fact that nsIUUIDGenerator is used everywhere: potentially increasing the memory overhead by using a more complex type than nsIDPtr might be bad.

   Alternatively we could introduce a second API like nsIUUIDGenerator.generateSerializableUUID() that returns a known-serialisable type (AString?) with the coercion happening on the C++ side. The downside I can see is that other XPIDL consumers (Rust) might want something other than a string for their serialisation, so there’s probably a point to be made for not making the XPIDL overly language-specific.

2. We introduce a JS frontend API for nsIUUIDGenerator

   There is precedence for wrapping XPIDL services to offer more JS-friendly APIs under toolkit/modules/. This is a more flexible approach because it allows type alterations that are specific to JS, without imposing any of the overhead on other XPIDL consumers (Rust/C++).

   A strawman proposal for toolkit/modules/UUID.jsm:

   XPCOMUtils.defineLazyServiceGetter(
     this,
     "gUUIDGenerator",
     "@mozilla.org/uuid-generator;1",
     "nsIUUIDGenerator"
   );
   
   var EXPORTED_SYMBOLS = ["UUID"];
   
   class UUID {
     constructor() {
       this.inner = gUUIDGenerator.generateUUID();
     }
   
     toString() {
       return this.inner.toString();
     }
   
     toJSON() {
       return this.toString();
     }
   
     static get() {
       return new UUID();
     }
   }
   

   A consumer:

   const { UUID } = ChromeUtils.import("resource://toolkit/modules/UUID.jsm");
   
   const id = UUID.get();
   dump("I am unique: " + id);
   JSON.stringify({ id });
   

   Potentially this could be extended so JS consumers could register their own serialisations:

   class WebDriverElementID extends UUID {
     static ELEMENT_KEY = "element-6066-11e4-a52e-4f735466cecf";
   
     toJSON() {
       const id = super.toString().toUpperCase().slice(1, -1);
       return { WebDriverElementID.ELEMENT_KEY: id };
     }
   }
   

Since there seems to be ample room to improve the situation in other parts of central, maybe Mossop can offer some thoughts on this?

Comment 5

[Dave Townsend [:mossop]]

(In reply to Andreas Tolfsen 「:ato」 from comment #4)

1. We change the nsIUUIDGenerator API so it returns a type that is serialisable with JSON.stringify in JS

   This may not be workable because of API constraints and the fact that nsIUUIDGenerator is used everywhere: potentially increasing the memory overhead by using a more complex type than nsIDPtr might be bad.

   Alternatively we could introduce a second API like nsIUUIDGenerator.generateSerializableUUID() that returns a known-serialisable type (AString?) with the coercion happening on the C++ side. The downside I can see is that other XPIDL consumers (Rust) might want something other than a string for their serialisation, so there’s probably a point to be made for not making the XPIDL overly language-specific.

The number of places that use it in C++ is pretty low. I don't see any usage in the tree from Rust but maybe I'm not searching right. Of the C++ places, while I didn't survey them all more than half that I looked at were converting the generated UUID into a string. I would expect the number of JS callers that use it and need the nsIDPtr rather than a string is approximately 0. I'm not even sure there is any possible use for the nsIDPtr in JS.

So I think we should add that second method to nsIUUIDGenerator, (generateUUIDString?) and have it return an ACString so it can be used from both C++ and JS.

2. We introduce a JS frontend API for nsIUUIDGenerator

   There is precedence for wrapping XPIDL services to offer more JS-friendly APIs under toolkit/modules/. This is a more flexible approach because it allows type alterations that are specific to JS, without imposing any of the overhead on other XPIDL consumers (Rust/C++).

   A strawman proposal for toolkit/modules/UUID.jsm:

   XPCOMUtils.defineLazyServiceGetter(
     this,
     "gUUIDGenerator",
     "@mozilla.org/uuid-generator;1",
     "nsIUUIDGenerator"
   );
   
   var EXPORTED_SYMBOLS = ["UUID"];
   
   class UUID {
     constructor() {
       this.inner = gUUIDGenerator.generateUUID();
     }
   
     toString() {
       return this.inner.toString();
     }
   
     toJSON() {
       return this.toString();
     }
   
     static get() {
       return new UUID();
     }
   }
   

I'm not sure what the benefit of such an object is, however you use it you end up calling toString on the nsIDPtr, might as well do that once rather than needing to do it repeatedly and strings don't require a toJSON.

I agree that a JS front-end makes sense. But I'd suggest just adding a simple Services.uuid function to return a string (possibly also with an argument to strip the {} since that seems to be common in the tree).

Comment 6

[Andreas Tolfsen ❲:ato❳]

Adding ACString nsIUUIDGenerator.generateUUIDString() and a JS front-end through Services.uuid() -> string sounds good to me, but:

The C++ and Rust equivalents of Services are generated from xpcom/build/Services.py. Nika notes in a comment it may make sense to also generate Services.jsm as a build step.

If we add a uuid() function returning a string to Services.jsm this breaks with the service-getter pattern, potentially making it harder to move generation of Services.jsm to Services.py.

Thoughts?

Comment 7

[Dave Townsend [:mossop]]

(In reply to Andreas Tolfsen 「:ato」 from comment #6)

Adding ACString nsIUUIDGenerator.generateUUIDString() and a JS front-end through Services.uuid() -> string sounds good to me, but:

The C++ and Rust equivalents of Services are generated from xpcom/build/Services.py. Nika notes in a comment it may make sense to also generate Services.jsm as a build step.

If we add a uuid() function returning a string to Services.jsm this breaks with the service-getter pattern, potentially making it harder to move generation of Services.jsm to Services.py.

I'm not convinced that using Services.py to generate Services.jsm gains us much. I guess maybe you could do away with the call to XPCOMUtils.defineLazyServiceGetters, not sure how much time that actually takes. But I haven't heard specific plans to do anything and Services.jsm already breaks the service-getter pattern with xulStore so I wouldn't block this on that in itself right now.

That said, breaking the Services.xxx.yyy in general is worth thinking about. My gut tells me that when there is only one usable function on a service it makes no sense to require spelling out both the service and the function, but then we're already doing that for the version comparator case. Gijs, what do you think here?

Comment 8

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] (he/him) from comment #7)

That said, breaking the Services.xxx.yyy in general is worth thinking about. My gut tells me that when there is only one usable function on a service it makes no sense to require spelling out both the service and the function, but then we're already doing that for the version comparator case. Gijs, what do you think here?

Thoughts, in no particular order:

• from a JS perspective, it's weird to have an object whose sole purpose is one of its methods. That's literally the reason the function keyword exists in XPIDL - because it's weird to write {observe() {} } instead of just passing a function directly.
• this is a little less weird in C++ because lambdas and so on are a bit less idiomatic in the language, and passing them around as parameters can get hairy.
• Services exists mostly to provide a shorthand for the requisite Cc[@blahblahblah].getService(Ci.blahblahblah) goop. In C++ it also has some nice guarantees (I think?) that you don't have to check whether the service exists.
• The xulStore case is only an anomaly because of the unshipped rkv backend and the wrapper around that being JS-module implemented, but the original being a "true" XPCOM service. I'd expect it to go away (and migrate JS consumers to just ChromeUtils.import-ing the jsm) once we ship the rkv thing.
• dumping grounds are bad. Services isn't really a dumping ground as much as a practical way of providing syntactic sugar around XPCOM to JS. But adding methods to it directly makes it feel more like a dumping ground, a "Utils"-style class/object. And I'd rather not go in that direction.

Some personal preferences based on the above opinions/thoughts:

• ideally the version control comparison would live somewhere more related to what it does (e.g. on the appinfo service which also provides the current app and XUL version) instead of having its own little service.
• the uuidgen thing would feel less weird if it read as Services.uuid.generate() than if it was Services.uuidGen.generateUUID(). Ideally this too would live somewhere more related to what it does, but I don't know what that'd be. Perhaps directly on Ci or Components.manager, given that xpcom is still a big part of where we use uuids at all. Perhaps there's a better option I've overlooked. I'd suggest ChromeUtils except that is already a bit of a dumping ground...

I think in the short term, "just" adding an entry in the Services.jsm initTable is the simplest path to making things better here. It'd make our mozilla services eslint rule enforce that everyone uses it, and clean up code a little as a result. But I'd put it in as uuid rather than uuidgen. If we add a method we could just call it create() or generate(), and we could rename the existing thing to generateIDPtr() if we felt so inclined.

Unsure if that helps, but those are my thoughts, at least! :-)

Comment 9

[Nathan Froyd [:froydnj]]

I'm inclined to let Gijs and Mossop make the final call on what to do, if anything, about Services.jsm. I get the appeal of letting Services.py autogenerate Services.jsm, but I don't really see it winning anything, so I don't think we have to block anything on the grounds of "it would make autogeneration difficult".

Comment 10

[Dave Townsend [:mossop]]

(In reply to :Gijs (he/him) from comment #8)

• the uuidgen thing would feel less weird if it read as Services.uuid.generate() than if it was Services.uuidGen.generateUUID(). Ideally this too would live somewhere more related to what it does, but I don't know what that'd be. Perhaps directly on Ci or Components.manager, given that xpcom is still a big part of where we use uuids at all. Perhaps there's a better option I've overlooked. I'd suggest ChromeUtils except that is already a bit of a dumping ground...

We use uuids a lot in xpcom but the uses of nsIUUIDGenerator are almost all unrelated to xpcom, it's just a handy unique identifier for things.

I think in the short term, "just" adding an entry in the Services.jsm initTable is the simplest path to making things better here. It'd make our mozilla services eslint rule enforce that everyone uses it, and clean up code a little as a result. But I'd put it in as uuid rather than uuidgen. If we add a method we could just call it create() or generate(), and we could rename the existing thing to generateIDPtr() if we felt so inclined.

I think this is the most straightforward thing to do. Add it to Services.jsm in bug 1602940 and use this bug for adding a method that returns a simple string.

Comment 11

[Andreas Tolfsen ❲:ato❳]

With the patches up on bug 1602940, we can repurpose this bug to make use of the new nsIUUIDGenerator.generate() member in the remote debugging protocol.

Comment 12

[Chris Peterson [:cpeterson]]

Instead of using nsIUUIDGenerator.generate() (bug 1602940), remote protocol can use the crypto.randomUUID() API (bug 1723674):

https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Chris Peterson [:cpeterson] from comment #12)

Instead of using nsIUUIDGenerator.generate() (bug 1602940), remote protocol can use the crypto.randomUUID() API (bug 1723674):

https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID

Thanks Chris! Here is our usage of the old nsIUUIDGenerator API. All of the following entries need to be updated:
https://searchfox.org/mozilla-central/search?q=generateUUID%28%29&path=remote%2F&case=false&regexp=false

Comment 14

[Chris Peterson [:cpeterson]]

I see now that the crypto object (and thus the crypto.randomUUID() API) is not available in some browser chrome contexts that don't have a window object. In those cases, we probably still need to use the Services.uuid.generateUUID() API.

Note that crypto.randomUUID() returns a UUID string without curly braces like aef1b4a8-9c30-459d-8852-22bb4a4f8b79. nsIUUIDGenerator and Services.uuid.generateUUID() return a UUID string with curly braces like {aef1b4a8-9c30-459d-8852-22bb4a4f8b79}.

Comment 15

[Dave Townsend [:mossop]]

(In reply to Chris Peterson [:cpeterson] from comment #14)

I see now that the crypto object (and thus the crypto.randomUUID() API) is not available in some browser chrome contexts that don't have a window object. In those cases, we probably still need to use the Services.uuid.generateUUID() API.

You should be able to get it in those contexts, e.g. https://searchfox.org/mozilla-central/rev/468a65168dd0bc3c7d602211a566c16e66416cce/browser/components/migration/ChromeMacOSLoginCrypto.jsm#14

Comment 16

[michael s]

Hi all, I'm working on this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1787997
and it looks like all references to uuid in remote/ utilized Services.uuid so perhaps this is a stale bug? Thanks!

Comment 17

[Chris Peterson [:cpeterson]]

(In reply to michael s from comment #16)

Hi all, I'm working on this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1787997
and it looks like all references to uuid in remote/ utilized Services.uuid so perhaps this is a stale bug? Thanks!

Thanks for catching this stale bug! In that case, I'll close this bug as "works for me" (since these function calls were apparently fixed in some other bug.)