1605045 - Document rationale and use cases for OwningNonNull

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Simon Giesecke [:sg] [he/him]]

The purpose and intended use of OwningNonNull is unclear. It has no inline documentation, and only a very brief description at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#OwningNonNull, which doesn't answer these questions.

In particular, I am concerned about the following points:

• When would I use OwningNonNull instead of RefPtr?
• Despite the name OwningNonNull, it can be nullptr even in Debug builds, either when using default-construction, or when calling forget. Please explain under what circumstances this is still beneficial over a plain RefPtr.

Minor points I noticed:

• While it has a forget method, it can't be move-constructed or move-assigned. It also can't be move-constructed or -assigned from a RefPtr<U>&&.

Comment 1

[Nathan Froyd [:froydnj]]

bz added this class in bug 748267, but I don't see any discussion there. bz, do you have context for Simon's questions?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Simon, thank you for bringing this up!

The original intent of OwningNonNull was to implement a class with the same auto-conversion and annotation semantics as NonNull<T> (i.e. never null once you have properly initialized it, auto-converts to T&), but that holds a strong reference to the object involved. This was designed for use in DOM bindings and in particular for storing what WebIDL represents as InterfaceName (as opposed to InterfaceName?) in various containers (dictionaries, sequences). We could have just used RefPtr for this use case, just like we could have used T* instead of NonNull<T>, but it seemed desirable to explicitly annotate the non-null nature of the things involved to eliminate pointless null-checks, which otherwise tend to proliferate.

This wasn't meant to be a generic container type for general use, really, though I see it got moved out of bindings code to XPCOM at some point...

When would I use OwningNonNull instead of RefPtr?

I described the bindings use cases above. Other consumers... it's really up to them. It adds a certain amount of noise to the code, obviously, but does make it clear when things are or are not null, if used properly.

Despite the name OwningNonNull, it can be nullptr even in Debug builds, either when using default-construction, or when calling forget.

Sticking to the original design space here, bindings never allow a default-constructed uninitialized OwningNonNull to escape. So if you are implementing a binding method, the default-constructed case is a non-issue. If you are doing initialization of things manually (i.e. acting as the caller, not implementor, of a binding method), you obviously need to make sure you properly initialize it, just like NonNull and various other things that binding implementors expect to be set up properly.

As for forget, that is a really good catch. the version returning already_AddRefed<T> is meant to be called from CC unlink only. The version with a T** outparam.... should not exist. I have no idea why that was added and why one would use it. I filed bug 1605130 on making this saner.

You're right that in theory one could dereference a null OwningNonNull after it's been unlinked. The same is true of any other container that unlinking puts into an invalid state; generally one should not be doing things after unlink...

Note that unlike a RefPtr, an OwningNonNull that is used in any way when null will assert in debug builds, at least...

Please explain under what circumstances this is still beneficial over a plain RefPtr

Please let me know whether the above covers that. It's only beneficial if you're in a situation where you can 100% guarantee that the OwningNonNull gets set up properly. Bindings can do that, because there's basically one single callsite that does the setup, and we just have to make sure that one callsite is implemented properly...

While it has a forget method, it can't be move-constructed or move-assigned

That's because the forget method should not be used, per above, with the required exception of CC unlink. If C++ had Rust-like move semantics, where a moved-out-of object can't be accessed, we could add move stuff here, but given the actual C++ move semantics, we don't want to create a way to create null OwningNonNull.

It also can't be move-constructed or -assigned from a RefPtr<U>&&.

We hadn't needed a way to do that in bindings code, and since this was not meant as a generic container class but rather to address specific binding-related bits, we didn't add functionality that we didn't need. There's nothing wrong in principle with adding this.

I hope that helps, and I'm happy to write some documentation for this class, or review proposed documentation. Please just let me know.

Comment 3

[Simon Giesecke [:sg] [he/him]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

Simon, thank you for bringing this up!

Thanks for the instant and detailed explanation!

The original intent of OwningNonNull was to implement a class with the same auto-conversion and annotation semantics as NonNull<T> (i.e. never null once you have properly initialized it, auto-converts to T&), but that holds a strong reference to the object involved. This was designed for use in DOM bindings and in particular for storing what WebIDL represents as InterfaceName (as opposed to InterfaceName?) in various containers (dictionaries, sequences). We could have just used RefPtr for this use case, just like we could have used T* instead of NonNull<T>, but it seemed desirable to explicitly annotate the non-null nature of the things involved to eliminate pointless null-checks, which otherwise tend to proliferate.

This wasn't meant to be a generic container type for general use, really, though I see it got moved out of bindings code to XPCOM at some point...

The explanation on the origin makes much sense to me, and it also explains the name. I am not so sure if moving to XPCOM was so good, at least without adding some disclaimer explaining the above. I was somehow mistaken on its semantics.

When would I use OwningNonNull instead of RefPtr?

I described the bindings use cases above. Other consumers... it's really up to them. It adds a certain amount of noise to the code, obviously, but does make it clear when things are or are not null, if used properly.

Despite the name OwningNonNull, it can be nullptr even in Debug builds, either when using default-construction, or when calling forget.

Sticking to the original design space here, bindings never allow a default-constructed uninitialized OwningNonNull to escape. So if you are implementing a binding method, the default-constructed case is a non-issue. If you are doing initialization of things manually (i.e. acting as the caller, not implementor, of a binding method), you obviously need to make sure you properly initialize it, just like NonNull and various other things that binding implementors expect to be set up properly.

As for forget, that is a really good catch. the version returning already_AddRefed<T> is meant to be called from CC unlink only. The version with a T** outparam.... should not exist. I have no idea why that was added and why one would use it. I filed bug 1605130 on making this saner.

Probably it used somewhere, and maybe in a case where a usage evolved beyond the original purpose, and maybe using a regular RefPtr there now makes more sense.

You're right that in theory one could dereference a null OwningNonNull after it's been unlinked. The same is true of any other container that unlinking puts into an invalid state; generally one should not be doing things after unlink...

Note that unlike a RefPtr, an OwningNonNull that is used in any way when null will assert in debug builds, at least...

Please explain under what circumstances this is still beneficial over a plain RefPtr

Please let me know whether the above covers that. It's only beneficial if you're in a situation where you can 100% guarantee that the OwningNonNull gets set up properly. Bindings can do that, because there's basically one single callsite that does the setup, and we just have to make sure that one callsite is implemented properly...

I think that at the places where I thought at some point it might make sense, it probably shouldn't be used, exactly because there is no guarantee of initialization. I added a class template InitializedOnce (see bug 1597954, at the moment residing within dom/indexedDB) which is in some way more generic (it allows not only RefPtr as the contained type, which also means it cannot be just as a drop-in for RefPtr as it doesn't expose its methods directly), and in some other way more constrained, as it doesn't allow to be re-assigned (hence the name) and at least with the default policy requires initialization on construction. But it can also be released (reset) again, and then further accesses assert in debug builds as well.

While it has a forget method, it can't be move-constructed or move-assigned

That's because the forget method should not be used, per above, with the required exception of CC unlink. If C++ had Rust-like move semantics, where a moved-out-of object can't be accessed, we could add move stuff here, but given the actual C++ move semantics, we don't want to create a way to create null OwningNonNull.

Having the forget method has the disadvantage that such uses are not caught by the bugprone-use-after-move clang-tidy rule. I assume the forget methods on smart pointer classes go back to pre-C++11 times where no move semantics were available. However, does unlinking require a forget that returns the pointer value, or does it just require setting it to nullptr?

But it probably makes sense to not add move-constructability or move-assignability as this opens up to further misuses.

It also can't be move-constructed or -assigned from a RefPtr<U>&&.

We hadn't needed a way to do that in bindings code, and since this was not meant as a generic container class but rather to address specific binding-related bits, we didn't add functionality that we didn't need. There's nothing wrong in principle with adding this.

Ok, I will keep that in mind when thinking about using OwningNonNull somewhere.

I hope that helps, and I'm happy to write some documentation for this class, or review proposed documentation. Please just let me know.

I might transform the information from your comment into inline documentation, and let you review it.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #3)

Having the forget method has the disadvantage that such uses are not caught by the bugprone-use-after-move clang-tidy rule. I assume the forget methods on smart pointer classes go back to pre-C++11 times where no move semantics were available.

That's correct, though note due to https://searchfox.org/mozilla-central/source/mfbt/AlreadyAddRefed.h#54-89, already_AddRefed<T> is more efficient to return from a function than RefPtr<T> or similar.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Probably it used somewhere

It was actually unused, per bug 1605130. Just added "just in case"...

I think that at the places where I thought at some point it might make sense, it probably shouldn't be used, exactly because there is no guarantee of initialization.

Yeah, if that's not guaranteed, OwningNonNull is not a good idea at all.

However, does unlinking require a forget that returns the pointer value, or does it just require setting it to nullptr?

Just the latter. I guess we can in fact simplify to that, now that the unlink function is a friend post-bug 1605130...

I might transform the information from your comment into inline documentation, and let you review it.

That would be very much appreciated!

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1605045 - Document rationale and use cases for OwningNonNull. r=bzbarsky

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9664ea28364d
Document rationale and use cases for OwningNonNull. r=bzbarsky

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/9664ea28364d