1693267 - nsRefPtrHashtable/nsInterfaceHashtable are no genuine subclasses of nsBaseHashtable

Status: RESOLVED WONTFIX

(Core :: XPCOM, task, P2)

Description

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

nsRefPtrHashtable::Get(KeyType, UserDataType*) and nsInterfaceHashtable::Get(KeyType, UserDataType*) hide the nsBaseHashtable member function with the same signature, but have a different behaviour: The base class function gets a weak reference, while the sub-class functions addref.

The opposite is true for nsRefPtrHashtable::GetWeak and nsInterfaceHashtable::GetWeak, while is, apart from the additional optional aFound argument, equivalent to nsBaseHashtable::Get(KeyType).

However, nsRefPtrHashtable::Get(KeyType) and nsInterfaceHashtable::Get(KeyType) return already_AddRefed<PtrType> rather than UserDataType.

While the second and third point are "only" confusing (and might indirectly lead to errors), the first is really error-prone.

Unfortunately, it's not clear how to resolve this properly. The reasons behind these inconsistencies are different usage patterns, which are certainly present. One option would be not to inherit publicly from nsBaseHashtable, but rather selectively expose the base class' functions which make sense. This would increase the complexity of the header file, but would improve its usability.

Comment 1

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

A similar issue also affects other methods such as LookupOrInsert. I think that the inheritance should really be made protected, and only specific member functions should be exposed.

nsRefPtrHashtable and nsInterfaceHashtable only differ in the DataType, so the two templates could probably be merged into a nsRefcountedBaseHashtable and the existing types be replaced by type aliases, as a follow-up.

Comment 2

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

Attachment: Bug 1693267 - Stop publicly deriving from nsBaseHashtable in nsRefPtrHashtable/nsInterfaceHashtable. r=#xpcom-reviewers

The derivation is changed to protected, and only specific member functions of
nsBaseHashtable are exposed. This makes it clear that this is not a genuine
specialization and reduces the expectation that the behaviour is the same as
in the base class, in particular for the Get member functions. Also, it avoids
that when new member functions are added to the base class, these are implicitly
exposed for the descendants, which recently happened for LookupOrInsert(With).

Adds refcounting-aware variants of LookupOrInsert(With) as GetOrInsert(With)(Weak)

Depends on D105479

Comment 3

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

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

nsRefPtrHashtable and nsInterfaceHashtable only differ in the DataType, so the two templates could probably be merged into a nsRefcountedBaseHashtable and the existing types be replaced by type aliases, as a follow-up.

I' plan to do this as part of Bug 1634281.

Comment 4

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

After discussion with nika, we came to the conclusion not to fix this, but rather unify the interfaces in Bug 1634281, and then migrate existing users of nsRefPtrHashtable/nsInterfaceHashtable over to the new unified type, and eventually remove nsRefPtrHashtable/nsInterfaceHashtable.