Make HoldJSObjects and DropJSObjects work with inheritance

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

Trunk
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Posted patch WIP (obsolete) — Splinter Review
One thing I don't like about this is that it requires adding XPCOM glue functions.
Attachment #789738 - Attachment is obsolete: true
Posted patch WIP, arguably less gross (obsolete) — Splinter Review
Attachment #790255 - Attachment is obsolete: true
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.
Blocks: includehell
(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/
Attachment #791045 - Attachment is obsolete: true
Summary: Make NS_HOLD_JS_OBJECTS and NS_DROP_JS_OBJECTS work with inheritance → Make HoldJSObjects and DropJSObjects work with inheritance
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
Attachment #792419 - Flags: review?(peterv)
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)
Attachment #792419 - Flags: review?(peterv) → review+
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
https://hg.mozilla.org/mozilla-central/rev/40b6e7719101
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Blocks: 912747
You need to log in before you can comment on or make changes to this bug.