Closed Bug 602122 Opened 14 years ago Closed 9 years ago

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

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: jst, Assigned: nika)

References

Details

Attachments

(2 files, 2 obsolete files)

The warnings from a run of the analysis
15.63 KB, text/plain
Details
Add a static analysis to find XPCOM classes with duplicate mRefCnt members
9.18 KB, patch
Details | Diff | Splinter Review 
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.
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.
Attachment #8624935 - Flags: feedback?(ehsan)
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.
You can look at the bugs blocking bug 895502 if you want some examples of what fixes to this might look like.
Assignee: nobody → michael
Comment on attachment 8624942 [details]
The warnings from a run of the analysis

Do you think you can try to fix them?  :-)
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!
Attachment #8624935 - Flags: feedback?(ehsan) → review+
(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 :(
Flags: needinfo?(ehsan)
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.
I did that in bug 1008420.
(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.
Flags: needinfo?(ehsan)
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
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?
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.
Fancier error messages & warning => error
Attachment #8624935 - Attachment is obsolete: true
Depends on: 1181318
Depends on: 1181319
Depends on: 1181321
Depends on: 1181323
Depends on: 1181324
Depends on: 1185527
Is this ready to go?
I'm pretty sure it can't land right now due to bug 1185527.
Heh, yes, hopefully!
(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.
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!
Flags: needinfo?(michael)
This should get rid of the diagnostic which clang was emitting (I think). 

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51331a93b1e4
Attachment #8630692 - Attachment is obsolete: true
Flags: needinfo?(michael)
That didn't work!
(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.
Depends on: 1189090
https://hg.mozilla.org/mozilla-central/rev/ea041b07a51b
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1244825
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: