add static checking for proper use of _INHERITED cc class macros

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 719812 [details] [diff] [review]
wip

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

Comment 2

5 years ago
Created attachment 719813 [details] [diff] [review]
DOMSVGTranslatePoint should be an inherited cc class

this seems like a legitimate bug nsISVGPoint has a refptr and is ccable
Attachment #719813 - Flags: review?(bugs)
(Reporter)

Comment 3

5 years ago
Created attachment 719814 [details] [diff] [review]
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
Attachment #719814 - Flags: review?(bugs)
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.
Attachment #719813 - Flags: review?(bugs) → review-
Depends on: 846710
Attachment #719814 - Flags: review?(bugs) → review+
I was told ArchiveZipFile is not exposed to web by default.
(Reporter)

Comment 6

5 years ago
(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?
> 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.
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.
(Reporter)

Updated

5 years ago
Attachment #719812 - Flags: feedback?(continuation)
Attachment #719812 - Flags: feedback?(bugs)
Comment on attachment 719812 [details] [diff] [review]
wip

Yeah, something like this would be good.
fix the alignment of \
Attachment #719812 - Flags: feedback?(bugs)
(Reporter)

Comment 10

5 years ago
archive zip file thing
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f2ed6574365
(Reporter)

Updated

5 years ago
Whiteboard: [leave open]
(Reporter)

Comment 11

5 years ago
Created attachment 721005 [details] [diff] [review]
patch
Attachment #719812 - Attachment is obsolete: true
Attachment #719812 - Flags: feedback?(continuation)
Attachment #721005 - Flags: review?(bugs)
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() ?
Attachment #721005 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/5f2ed6574365
(Reporter)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1653161218b9
https://hg.mozilla.org/mozilla-central/rev/1653161218b9
(Reporter)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.