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

RESOLVED FIXED in mozilla19

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: bzbarsky)

Tracking

Trunk
mozilla19
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 777385
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... :(
(Reporter)

Comment 2

6 years ago
Yeah, I had the same trouble. Maybe throwing it into the constructor wouldn't be so terrible...
Well, I can throw the runtime assert into the binding wrapping code.  That part is easy.
Actually, I have this working.  Somehow SFINAE is ok with the setup, yay.
Created 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

Going to push this to try to make sure
Attachment #676463 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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.
Attachment #676463 - Flags: review?(peterv)
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?
Created attachment 676748 [details] [diff] [review]
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
Attachment #676748 - Flags: review?(peterv)
Summary: Statically assert that nsWrapperCache is the first superclass of non-nsISupport refcounted classes → Assert that nsWrapperCache is the first superclass of non-nsISupport refcounted classes
Attachment #676463 - Attachment is obsolete: true
Created attachment 677252 [details] [diff] [review]
Now with more checking
Attachment #677252 - Flags: review?(peterv)
Attachment #676748 - Attachment is obsolete: true
Attachment #676748 - Flags: review?(peterv)
Attachment #676748 - Attachment is obsolete: false
Attachment #676748 - Flags: review?(peterv)
Attachment #677252 - Attachment is obsolete: true
Attachment #677252 - Flags: review?(peterv)
Attachment #676748 - Flags: review?(peterv) → review+

Comment 11

6 years ago
https://hg.mozilla.org/mozilla-central/rev/ed1c4f642584
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Can't you use offsetof to make a static assertion?
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.
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.  :(
You need to log in before you can comment on or make changes to this bug.