883920 - Make HoldJSObjects and DropJSObjects work with inheritance

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8] (PTO until 12-2)]

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.

Comment 1

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: WIP

Comment 2

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: WIP, DROP and HOLD, doesn't fully build

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

Comment 3

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: WIP, arguably less gross

Comment 4

[Andrew McCreight [:mccr8] (PTO until 12-2)]

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.

Comment 5

[(no longer active)]

(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 6

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: WIP, with CheckForRightISupports for DROP

Comment 7

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: Use templates for HoldJSObjects and DropJSObjects

Comment 8

[Andrew McCreight [:mccr8] (PTO until 12-2)]

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

Comment 9

[Peter Van der Beken [:peterv]]

I'll get to this tomorrow.

Comment 10

[Andrew McCreight [:mccr8] (PTO until 12-2)]

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

Comment 11

[Peter Van der Beken [:peterv]]

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)

Comment 12

[Andrew McCreight [:mccr8] (PTO until 12-2)]

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

Comment 13

[Peter Van der Beken [:peterv]]

Ah, because of the NS_CYCLE_COLLECTION_PARTICIPANT?

Comment 14

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(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...

Comment 15

[Andrew McCreight [:mccr8] (PTO until 12-2)]

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

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/40b6e7719101