802275 - The generated binding code for nativeOwnership:"isupports" classes which do not inherit from nsISupports should fail to compile

Status: REOPENED

(Core :: DOM: Core & HTML, defect, P5)

Description

[(no longer active)]

Recipe for disaster:

1. Make sure your native implementation class does not inherit from nsISupports.
2. Forget to add nativeOwnership:refcounted in the Bindings.conf file.
3. Spend several hours to debug the weird crashes.
4. Look inside the generated bindings code to find stuff like reinterpret_cast<nsISupports*>(foo).
5. Reflect on how much type checking can be useful.  ;-)

The least we should do here is something like:

MOZ_STATIC_ASSERT((mozilla::IsBaseOf<nsISupports, NativeClass>::value), "badness");

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yes, agreed on IsBaseOf static assert!

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

reinterpret_cast<nsISupports*>(foo) is wrong not only when nsISupports is not a base class of NativeClass, but also when nsISupports is not the first base class of NativeClass, right?  That can happen for wrappercached things.

Comment 3

[Boris Zbarsky [:bzbarsky]]

WebIDL objects don't support that.  nsISupports must be the first base class, just like the classes that map to parent protos must be.

Comment 4

[(no longer active)]

(In reply to comment #2)
> reinterpret_cast<nsISupports*>(foo) is wrong not only when nsISupports is not a
> base class of NativeClass, but also when nsISupports is not the first base
> class of NativeClass, right?  That can happen for wrappercached things.

Right.  Also, that means that IsBaseOf won't work if you derive from multiple nsISupports (nsISupports's? nsISupports'?)

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> Right.  Also, that means that IsBaseOf won't work if you derive from
> multiple nsISupports (nsISupports's? nsISupports'?)

Yup, that's why I didn't add compile-time checking for that in the first place. It'd be really nice if IsBaseOf would handle that.

I'm working on replacing nativeOwnership with compile-time detection, but that might take a while.

Comment 6

[(no longer active)]

(In reply to comment #3)
> WebIDL objects don't support that.  nsISupports must be the first base class,
> just like the classes that map to parent protos must be.

Do we enforce that anywhere?

Comment 7

[(no longer active)]

Attachment: WIP patch

I got this to a point that it builds and doesn't crash when running tests, which is good.  Then I faced assertions like this: "ASSERTION: nsCycleCollectionParticipant shouldn't change!".  mccr8 tells me that I need to add a virtual GetParticipant function which returns the right type of participant based on the concrete type of the object, and that is on top of the virtual AddRef and Release methods that I had to add, so this is no longer worth doing in my opinion.  I'm attaching the patch that I have here for future reference and WONTFIXing this.

Comment 8

[(no longer active)]

Shoot, wrong bug!  Ignore comment 7.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Do we enforce that anywhere?

Unfortunately, no.  If you have ideas on how to enforce it, that would be great.

I guess we could try, in a debug build, QIing in Wrap() and asserting if the pointers are different?

Comment 10

[(no longer active)]

(In reply to comment #9)
> > Do we enforce that anywhere?
> 
> Unfortunately, no.  If you have ideas on how to enforce it, that would be
> great.
> 
> I guess we could try, in a debug build, QIing in Wrap() and asserting if the
> pointers are different?

That's exactly what I had in mind (assuming you meant MOZ_ASSERT).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm.  But I thought we had cases where the binding object inherits from a non-canonical isupports...  The QI approach won't work there.

Comment 12

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.