805988 - Assert that nsWrapperCache is the first superclass of non-nsISupport refcounted classes

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

This will allow us to reinterpret_cast them directly to nsWrapperCache.

[09:34am] bz: It would be easy to static_assert in the wrapping code that the nsWrapperCache is first
[09:34am] bz: Just take your type
[09:34am] bz: static_cast to nsWrapperCache
[09:34am] bz: reinterpret_cast both pointers to void* and compare
[09:34am] bz: if they're not equal, you fail compilation

It might also be nice to have some kind of conversions to nsWrapperCache that do the appropriate checks or something, handling the various cases, though for my purposes I want both the nsWrapperCache and the "raw" pointer (which are different in the nsISupports case) which makes it trickier.  I sort of do something like this in bug 777385.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK.  Except static_cast is not OK in constant expressions.

So it would be trivial to make this fail at runtime (MOZ_ASSERT), but I have yet to make it work with static asserts... :(

Comment 2

[Andrew McCreight [:mccr8]]

Yeah, I had the same trouble. Maybe throwing it into the constructor wouldn't be so terrible...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, I can throw the runtime assert into the binding wrapping code.  That part is easy.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Actually, I have this working.  Somehow SFINAE is ok with the setup, yay.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Add a static assert that new-binding objects are either nsISupports or can be safely reinterpret_cast to nsWrapperCache, if they have an nsWrapperCache at all. that everything except IsValidForWrapNewBindingObject just got moved with no changes

Going to push this to try to make sure

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 676463 [details] [diff] [review]
Add a static assert that new-binding objects are either nsISupports or can be safely reinterpret_cast to nsWrapperCache, if they have an nsWrapperCache at all.   that everything except IsValidForWrapNewBindingObject just got moved with no changes

Hmm.  On Windows and Linux this does not seem to compile, complaining about too few template arguments.  I wonder what gives there.  I'll try it on my Linux box, I guess.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, looks like it working with clang is sort of a clang bug.  Or something.  Or maybe gcc and msvc don't implement some spec update...  It doesn't matter that much.

Should we just do a MOZ_ASSERT instead of a MOZ_STATIC_ASSERT here?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Add some asserts that new-binding objects are nsISupports if they claim to be or or can be safely reinterpret_cast to nsWrapperCache, if they have an nsWrapperCache at all, if they don't claim to be nsISupports. that everything except IsValidForWrapNewB

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with more checking

Comment 10

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ed1c4f642584

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ed1c4f642584

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Can't you use offsetof to make a static assertion?

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

I mean, I'm coming from bug 811371 where we discussed whether an assertion could guard against forgetting to put nsWrapperCache first in the list of base classes. Andrew mentioned that a runtime assertion for that was added in the present bug.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I considered using offsetof, but it would have to be done on a member, right?  And it's not clear what offsetof would do, exactly, on objects with vtables.  Per spec, you're not even allowed to use it on those.  :(