This patch takes over the RESERVED flag in nsIXPCScriptable. I'm still not sure whether this is safe or not, but as far as we know, there aren't any users in the tree and jst and brendan both say to go for it, so here it is. I also included a typo from my previous patch when I added the innerObject JS/XPConnect hook, xpc_map_end wasn't undefining it.

jband may remember why RESERVED was that way. We are claiming it, it seems safe. /be

Yes, that bit was reserved for a good reason (as the comment tried to indicate). I believe that would be: #define XPC_WN_SJSFLAGS_MARK_FLAG JS_BIT(31) // only high bit of 32 is set http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/xpcprivate.h&line_nums=on&root=/cvsroot&rev=HEAD&#1434 I'm thinking that you would be better off coming up with some other mechanism for doing what you want to do rather than trying to make xpconnect do its marking on some other bit.

FWIW, It seems to me that all the other flags are there to tell xpconnect how to act. Your flag is an attempt to convey a flag to the JSClass. I'm guessing that you could get away with hacking the flag in the JSClass during the PostCreate callback. Just a thought :)

(In reply to comment #4) > FWIW, It seems to me that all the other flags are there to tell xpconnect how > to act. Your flag is an attempt to convey a flag to the JSClass. I'm guessing > that you could get away with hacking the flag in the JSClass during the > PostCreate callback. Just a thought :) Sadly, PostCreate is too late. JSCLASS_GLOBAL_FLAGS includes JSCLASS_HAS_RESERVED_SLOTS which is used during JS_NewObject. One thought that I had was to create an nsIXPCScriptableGlobal which would do the right thing, but I'm sure there's purity objections there as well. My other alternaive is to create a second flags member (extraFlags?) and pass this information through there.

Well, it's really up to you guys. I think it is a question of how much ABI you are willing to break and how much you might expect to extend this in the future. Options I can think of... 1) Make (or steal) some new space for the marking bit and go ahead and steal the reserved flag. This is the most like what your patch does. It is not disruptive ABI-wise. It might mean using 32 more bits in XPCNativeScriptableShared (something that seems to have been growing with the replacement of JSClass. This seems easy and cheap, but perhaps just forestalls the problem of running out of flag bits. 2) Make a whole new set of flags (as you suggest). This would also mean adding another attribute in nsIXPCScriptable and that would be an ABI change. But, this makes sense if you know you are going to need more flags. You guys are 'already' adding a fourth flag beyond what I knew about when we did XPCDOM. 3) Extend preCreate to take an 'out' pointer where extra JSClass flags could be communicated back to xpconnect whenever a wrapper is about to be created. This breaks ABI a *little* and entails a little more xpconnect work. But is a pretty preCreate-ish thing to do. 4) Combine a couple of the recently added flags; i.e. WANT_OUTER_OBJECT + WANT_INNER_OBJECT ==> WANT_INNER_AND_OUTER_OBJECT so that one flag would control both of those callbacks being called. Then, steal the flag that would no longer be needed. This is cheap and easy and mostly doesn't break ABI - but would suck if anyone outside the tree is actually using the flag that would be getting repurposed. If it was me, I guess I'd choose option 1. Unless I knew that I was going to need more flags for sure, I'd put off messing with the ABI in any way. I'd take the marking stuff out of XPCNativeScriptableFlags and make XPCNativeScriptableShared twiddle bits of its own.

This fix combines the inner and outer hook flags into one (since it doesn't make sense for anybody to WANT_INNER without WANT_OUTER). I might even argue that this does the right thing wrt outside consumers since the inner/outer stuff only really makes sense for global objects.

Comment on attachment 223865 [details] [diff] [review] Better fix >-#ifdef XPC_MAP_WANT_OUTER_OBJECT >+#ifdef XPC_MAP_WANT_INNER_OUTER_OBJECT > #undef XPC_MAP_WANT_OUTER_OBJECT > #endif Shouldn't the #undef change to match? sr=me with that addressed and pending jst's r+. /be

This appears to work, and I think it's even guaranteed to do the right thing all the time, but I still think that it's extremely ugly. It also needs heavier testing that I have time to give it.

Comment on attachment 224936 [details] [diff] [review] Ugly, brutal, invasive fix r=shaver if you normalize to JSBool as XPConnect prefers, and remove the "42" debugging code. Bonus points for using a #define OBJ_IS_GLOBAL JS_TRUE or an enum instead, for improved clarity.

The interdiff should probably do well here. There's a lot of mechanical PRBool -> JSBool changing. The 'interesting' changes are the new XXX comment and the fact that I don't null oldProto's GetScriptableInfo() (since we dereference it on the next line) nor the JSClass, since that return is the address of an object.

Comment on attachment 225039 [details] [diff] [review] Updated r=shaver, but FIXME: bug #### is better than XXX!

Fix checked into trunk.

I filed bug 341834 on the brutal hackiness of this patch.