Closed Bug 846619 Opened 7 years ago Closed 7 years ago

add static checking for proper use of _INHERITED cc class macros

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: tbsaunde, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Attached patch wip (obsolete) — Splinter Review
these functions should atleast be in #ifdef DEBUG, but I'm not terribly thrilled with the approach.
this seems like a legitimate bug nsISVGPoint has a refptr and is ccable
Attachment #719813 - Flags: review?(bugs)
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.
(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.
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)
Whiteboard: [leave open]
Attached patch patchSplinter Review
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+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.