846619 - add static checking for proper use of _INHERITED cc class macros

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Trevor Saunders (:tbsaunde)]

We should probably check that
given classes A and B where B inherits from A, A is a ccable class then if B declares itself to be a ccable class to handle more members then B should declare itself to be an inherited cc class.

If B inherits from A, and B claims to be a inherited ccable class then it should infact inherit from a class that is ccable.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: wip

these functions should atleast be in #ifdef DEBUG, but I'm not terribly thrilled with the approach.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: DOMSVGTranslatePoint should be an inherited cc class

this seems like a legitimate bug nsISVGPoint has a refptr and is ccable

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: ArchiveZipFile should use iherited cc nsISupports macros since it inherits from nsDOMFileCC

This one is correct, since nsDOMFile doesn't hold a refs, but it seems like its better to handle the inheritedness, in case it does some day

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 719813 [details] [diff] [review]
DOMSVGTranslatePoint should be an inherited cc class

Hmm, DOMSVGTranslatePoint inherits nsISVGPoint, which has already
implementation for addref/release.
Looks all broken to me.
Regression from Bug 821955.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I was told ArchiveZipFile is not exposed to web by default.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 719813 [details] [diff] [review]
> DOMSVGTranslatePoint should be an inherited cc class
> 
> Hmm, DOMSVGTranslatePoint inherits nsISVGPoint, which has already
> implementation for addref/release.
> Looks all broken to me.

so, its actually broken to have an inherited class that doesn't call the super classes AddRef / Release even if its the same mRefCnt?  it seems like we should be able to prevent that by making NS_DECL_CYCLE_COLLECTING_ISUPPORTS declare Addref / Release as final, though then I guess we need to adjust NS_DECL_ISUPPORTS_INHERITED maybe add cc version or something).  That might break trace refcount too?

> Regression from Bug 821955.

can't access, but I believe you


(In reply to Olli Pettay [:smaug] from comment #5)
> I was told ArchiveZipFile is not exposed to web by default.

ok, presumably we still want it to have good cc just less important right?

So, we do seem to be finding bugs here, but I'm not a huge fan of the wip to catch them do people think its worth it?

Comment 7

[Andrew McCreight [:mccr8]]

> So, its actually broken to have an inherited class that doesn't call the super classes AddRef / Release even if its the same mRefCnt?

DOMSVGTranslatePoint needs its own participant (because it adds a new field), so it needs its own AddRef/Release.  But it needs to call into the Traverse/Unlink of its parent class.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I filed bug 846710 yesterday to fix the SVG case.


> (In reply to Olli Pettay [:smaug] from comment #5)
> > I was told ArchiveZipFile is not exposed to web by default.
> 
> ok, presumably we still want it to have good cc just less important right?
We certainly should fix ArchiveZipFile, but it is probably not needed on branches. Trunk is enough.
The SVG case looks more worrisome.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 719812 [details] [diff] [review]
wip

Yeah, something like this would be good.
fix the alignment of \

Comment 10

[Trevor Saunders (:tbsaunde)]

archive zip file thing
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f2ed6574365

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 721005 [details] [diff] [review]
patch


>+/**
>+ * We use this macro to force that classes that inherit from a ccable class and
>+ * declare there own
their own?


 participant declare themselves as inherited cc classes.
>+ * To avoid possibly unecessary vtables we only do this checking in debug
unnecessary


>+ * builds.
>+ */
>+#ifdef DEBUG
>+#define NOT_INHERITED_CANT_OVERRIDE virtual void TestFunc() MOZ_FINAL {}
Perhaps make the method name less likely to be used for other things.
Maybe BaseCycleCollectable() ?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f2ed6574365

Comment 14

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1653161218b9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1653161218b9