1792920 - nsrootidl.idl and nscore.h don't agree what nsrefcnt is

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mike Hommey [:glandium]]

nsrootidl.idl has:

typedef unsigned long       nsrefcnt ;

nscore.h has:

typedef MozRefCountType nsrefcnt;

MozRefCountType is defined in mfbt/RefCountType.h:

typedef uintptr_t MozRefCountType;

The nsrootidl.idl version translates to pub type nsrefcnt = u32; in dist/xpcrs/rt/nsrootidl.rs, used by the xpcom crate.

This creates a discrepancy in how e.g. localization_addref is viewed by C++ vs. how it's viewed by rust, fortunately, it doesn't cause problems because the result is not used as of now.

Comment 1

[Mike Hommey [:glandium]]

It's interesting to note that nsISupports::AddRef returns MozExternalRefCountType, which is different..

Comment 2

[Andrew McCreight [:mccr8]]

The implementation of nsISupports is actually hard coded in nsISupportsBase.h. We might be able to directly use the code genned version of the interface now, which could help eliminate some of these discrepancies.

Comment 3

[Andrew McCreight [:mccr8]]

I started looking at this. I think the "simplest" approach here is to get rid of nsISupportsBase.h and use the XPIDL code gen directly for nsISupports, adjusting nsISupports.idl so that it is accurate. That will avoid issues like this in the future.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1792920, part 1 - Remove the unused return values for various Rust AddRef and Release methods.

Rust's definition of nsrefcnt is incorrect, so I'm eliminating uses of it
where possible. Nobody actually uses these return values, so remove them.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1792920, part 2 - Fix the type of the refcount argument for NS_LogAddRef and NS_LogRelease in Rust.

The type of the refcount argument to these functions is the C++ type
nsrefcnt, which is actually uintptr_t. This is different from the
definition of nsrefcnt in XPIDL which is currently being used. These
functions are not actually defined in XPIDL, and also uintptr_t
can't be expressed in XPIDL, so let's just change it to the type it
really is in Rust, usize. The goal of the patch is to eliminate
some uses of nsrefcnt from Rust.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1792920, part 3 - Make Rust use a new MozExternalRefCountType instead of XPIDL's nsrefcnt.

The remaining uses of XPIDL's definition of nsrefcnt in Rust are all
now related to the return type of nsISupport's AddRef and Release
methods. The C++ name of the return types is MozExternalRefCountType,
not nsrefcnt (which is something else entirely), so use a more accurate
name.

I can't use the C++ definition, because it is in C++, and I eventually
want to remove the XPIDL definition, because it isn't accurate for C++,
so in this patch I'll make a specific one for Rust, so the code is a
little easier to read.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Bug 1792920, part 4 - Add special handling for MozExternalRefCountType to XPIDL.

In XPIDL, the return type of AddRef and Release are nsrefcnt, which
is defined as unsigned long. The first problem is that nsrefcnt is
defined in XPIDL as unsigned long, but in C++ as uintptr_t. The second
problem is that the actual return type of these methods in C++ is
MozExternalRefCountType.

This patch fixes the return type of these methods in XPIDL. However,
due to MSCOM compatibility requirements this type is not expressible
in XPIDL, so I added it as a new builtin type to XPIDL. In C++, it
defers to the typedef and for Rust it uses u32 directly. I tried
using the new Rust typedef I defined in refptr.rs, but it seemed like
it was being used in places that didn't have refptr.rs available.

I also deleted the incorrect definition of nsrefcnt in XPIDL. It is
not used anywhere else in XPIDL files, and my previous patches removed
all uses of this type definition from Rust.

Note that this patch does not really change nsISupports.h,
because this part of the file is ignored via an #if 0. However, this
patch does clear the way for further work to start using the auto
generated nsISupports.h instead of nsISupportsBase.h.

Comment 8

[Andrew McCreight [:mccr8]]

The summary of the changes I landed here to address the initial issue is that I removed nsrefcnt from XPIDL (which mostly involved removing it from where it is being used in Rust) and added MozExternalRefCountType, which is turned into appropriate types in C++ and Rust (though they are still not exactly the same types...). This enables us to use the generated nsISupports.h, but that isn't necessary to fix this specific issue, so I split it off into a separate bug, bug 1794811. Although both of these change nsISupports.idl so I should probably land them at the same time, after the soft freeze is over.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26c5ff54183e
part 1 - Remove the unused return values for various Rust AddRef and Release methods. r=necko-reviewers,platform-i18n-reviewers,gregtatum,valentin
https://hg.mozilla.org/integration/autoland/rev/3f31e79b7c6f
part 2 - Fix the type of the refcount argument for NS_LogAddRef and NS_LogRelease in Rust. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/1c0f3a886f3e
part 3 - Make Rust use a new MozExternalRefCountType instead of XPIDL's nsrefcnt. r=nika
https://hg.mozilla.org/integration/autoland/rev/20a436f635b4
part 4 - Add special handling for MozExternalRefCountType to XPIDL. r=nika

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/26c5ff54183e
https://hg.mozilla.org/mozilla-central/rev/3f31e79b7c6f
https://hg.mozilla.org/mozilla-central/rev/1c0f3a886f3e
https://hg.mozilla.org/mozilla-central/rev/20a436f635b4