602122 - Add static analysis to find XPCOM classes with duplicate mRefCnt members.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Johnny Stenback (:jst)]

We've found a few of these already, where we have a class that implements some interface and thus gets a reference count member. Then some other class inherits from that class, and some other interfaces, and forgets to use the _INHERITED macros and ends up with its own reference count member. This should be all safe as long as all accesses to the reference count goes through virtual functions etc, but it's a waste of space and a potential hazard in cases where code uses the reference count for other reasons.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Add a static analysis to find XPCOM classes with duplicate mRefCnt members

I got bored & took a shot at implementing this static analysis. Right now this patch emits warning level messages for any classes which it finds with duplicate mRefCnt members due to inheritance. I'm currently running a build to get a list of classes which this analysis detects. I'll post it in this bug when I find it.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: The warnings from a run of the analysis

These are the duplicate mRefCnt members which the analysis detected, in the form ClassName => SuperClassName:
nsSVGFilterReference     => nsSVGRenderingObserver
MultipartImage           => ImageWrapper
PromiseWorkerProxy       => PromiseNativeHandler
AllResolveHandler        => PromiseNativeHandler
ChildPledge<T>           => Pledge<T>
DataChannelChild         => nsHashPropertyBag
FetchHandler             => PromiseNativeHandler
CompareCache             => PromiseNativeHandler
RegisterDebuggerRunnable => nsRunnable
DelayedResolveOrReject   => nsRunnable

I've also attached the parts of the log which contained the warnings emitted by the analysis.
I haven't confirmed yet whether these warnings are correct, but based on my testing I imagine that they are.

Comment 4

[Andrew McCreight [:mccr8]]

You can look at the bugs blocking bug 895502 if you want some examples of what fixes to this might look like.

Comment 5

[(no longer active)]

Comment on attachment 8624942 [details]
The warnings from a run of the analysis

Do you think you can try to fix them?  :-)

Comment 6

[(no longer active)]

Comment on attachment 8624935 [details] [diff] [review]
Add a static analysis to find XPCOM classes with duplicate mRefCnt members

Review of attachment 8624935 [details] [diff] [review]:
-----------------------------------------------------------------

This is really great!

I think we can land it as it (with the possible build issues fixed, of course!) unless if you know of a reason not to!

::: build/clang-plugin/clang-plugin.cpp
@@ +481,5 @@
>  
> +bool classHasRefCntMember(const CXXRecordDecl *D) {
> +  for (RecordDecl::field_iterator field = D->field_begin(), e = D->field_end();
> +       field != e; ++field) {
> +    if (field->getName() == "mRefCnt") {

It seems like mRefCnt is such a prevalent idiom that we can get away with this!  We can of course extend it in the future.

@@ +1038,5 @@
> +void DiagnosticsMatcher::NoDuplicateRefCntMemberChecker::run(
> +    const MatchFinder::MatchResult &Result) {
> +  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
> +  unsigned warningID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Warning, "Refcounted record %0 has multiple mRefCnt members");

Given that we warn with this, can you please see if we can build with this on builds with --enable-warnings-as-errors?  As in the try server builds for example?

Thanks!

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #6)
> Given that we warn with this, can you please see if we can build with this
> on builds with --enable-warnings-as-errors?  As in the try server builds for
> example?

My plan was to get the warnings fixed locally, and then promote the warning to an error before landing it in central. It was just a warning because that way it's easier to run over the entire mozilla codebase (I just ran the static analysis and grepped for the warning text). Probably should've made it an error before I attached the patch to the bug, but hey, what's done is done!

Fixing the mRefCnt uses may be a bit complicated. The worst example I've seen so far is PromiseNativeHandler. PromiseWorkerProxy overrides PromiseNativeHandler's default ref count mechanism to have threadsafe reference counting, and AllResolveHandler overrides it to have cycle collecting.

This is problematic, as cycle collection and threadsafe reference counting are, as far as I can tell, mutually exclusive, which means that we can't simply choose an implementation and make PromiseNativeHandler implement that one. I'm really not sure what to do with that :(

Comment 8

[Andrew McCreight [:mccr8]]

That sounds like the issue with nsArray. Basically, you need a base class that does not implement refcount (nsArrayBase for nsArray), then you have subclasses that implement each kind of refcounting you want. Then the other subclasses inherit from whichever refcounting subclass.

Comment 9

[Andrew McCreight [:mccr8]]

I did that in bug 1008420.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> That sounds like the issue with nsArray. Basically, you need a base class
> that does not implement refcount (nsArrayBase for nsArray), then you have
> subclasses that implement each kind of refcounting you want. Then the other
> subclasses inherit from whichever refcounting subclass.

Ah, that makes sense :), I'll try to do that for PromiseNativeHandler tomorrow.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

I've gotten the build to run on my local computer - but I probably butchered the reference counting :)

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7968eea9872

Comment 12

[Andrew McCreight [:mccr8]]

You should probably file bugs for each of the issues you are fixing.

Also, the argument that NS_INLINE_DECL_THREADSAFE_REFCOUNTING that seems to be only used for overriding refcounting looks like a bad idea. You should probably remove that... Are you detecting all of these as errors?

Comment 13

[Andrew McCreight [:mccr8]]

Well, the override thing is used in at least one place (NrSocketIpc) just to override an abstract method which I guess is okay, so it isn't all bad.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Add a static analysis to find XPCOM classes with duplicate mRefCnt members

Fancier error messages & warning => error

Comment 15

[Nika Layzell [:nika] (ni? for response)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b033b7d9b2c

Comment 16

[(no longer active)]

Is this ready to go?

Comment 17

[Andrew McCreight [:mccr8]]

I'm pretty sure it can't land right now due to bug 1185527.

Comment 18

[(no longer active)]

Heh, yes, hopefully!

Comment 19

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #17)
> I'm pretty sure it can't land right now due to bug 1185527.

I've confirmed on a local build that this patch does in fact error due to that bug.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d94504b602d

Comment 21

[(no longer active)]

I landed this but it bounced: <https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6d94504b602d>

Michael, do you mind taking a look when you get a chance?  Thanks!

Comment 22

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6934c1805984

Comment 23

[Nika Layzell [:nika] (ni? for response)]

Attachment: Add a static analysis to find XPCOM classes with duplicate mRefCnt members

This should get rid of the diagnostic which clang was emitting (I think). 

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51331a93b1e4

Comment 24

[(no longer active)]

That didn't work!

Comment 25

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #24)
> That didn't work!

That's unfortunate. I'm out of the house right now but I'll try something else tonight.

Comment 26

[(no longer active)]

This, with the fix for bug 1189090 on top of latest inbound: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a1400a7a2ed>

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea041b07a51b

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea041b07a51b