If an nsISupports class nsFoo has a subtype nsBar with its own participant, and |x| is an instance of nsBar, then |NS_HOLD_JS_OBJECTS(x, nsFoo);| will result in |x| being added with nsFoo's participant, not nsBar's. This is especially tricky if nsBar initially didn't have its own participant, but was later changed. There should be some kind of nsISupports HOLD/DROP that does all of the proper canonicalization, and leave the existing form for non-nsISupports cycle collected classes, which don't support inheritance right now anyways.

One thing I don't like about this is that it requires adding XPCOM glue functions.

I'm not writing the patch for this purpose, but it does let us drop a number of dependencies on nsContentUtils, which is a fairly giant header.

(In reply to comment #4) > I'm not writing the patch for this purpose, but it does let us drop a number of > dependencies on nsContentUtils, which is a fairly giant header. \o/

Comment on attachment 792419 [details] [diff] [review] Use templates for HoldJSObjects and DropJSObjects Suggestions welcome for naming and namespace for HoldJSObjects and DropJSObjects or whatever. There's a lot of bootlegging for the new header. I'd be happy to fix that if you think it would be better. The main drawback of this patch is that we're going to QI every time we do hold and maybe drop, but hopefully that's not a big deal. try run: https://tbpl.mozilla.org/?tree=Try&rev=fe03c887b3fc

I'll get to this tomorrow.

In IRC, ms2ger suggests omitting the "ns" from nsCycleCollectionHoldDrop.h, so I can do that if it makes sense.

Comment on attachment 792419 [details] [diff] [review] Use templates for HoldJSObjects and DropJSObjects Review of attachment 792419 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBCursor.cpp @@ +321,5 @@ > IDBDatabase* database = aTransaction->Database(); > cursor->mScriptOwner = database->GetScriptOwner(); > > if (cursor->mScriptOwner) { > + mozilla::HoldJSObjects(cursor.get()); So without a get, would we end up using the non-nsISupports template? If so, that's a little scary and we might want to add a template for SmartPtr? (|template <template <typename> class SmartPtr, typename T>|, ugh)

IIRC, it just didn't compile, but I'll double check that.

Ah, because of the NS_CYCLE_COLLECTION_PARTICIPANT?

(In reply to Peter Van der Beken [:peterv] from comment #13) > Ah, because of the NS_CYCLE_COLLECTION_PARTICIPANT? Yeah, probably. And drop fails presumably because it is under the same template abstraction as hold, which I decided to do fairly arbitrarily, but I guess it works out...

Landed with an additional fix for nsXULPDGlobalObject. I ended up leaving the "ns" in the file name, because most of the things in xpcom/glue were doing that and I wasn't sure how it should go exactly. https://hg.mozilla.org/integration/mozilla-inbound/rev/40b6e7719101