Closed Bug 491977 Opened 11 years ago Closed 11 years ago

sDeadlockDetector leaks it's hash table (and then some nsTArray_bases)

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sdwilsh, Assigned: cjones)

References

()

Details

Attachments

(1 file, 3 obsolete files)

It would seem that the deadlock detector code leaks it's hashtable, which shows up as nsTArray_bases that leak in our leak statistics.  This is because PL_HashTableDestory is never called on DeadlockDetector<T>::mOrdering because sDeadlockDetector is a static member.
Attached patch Fix (obsolete) — Splinter Review
Attachment #377034 - Flags: review?(benjamin)
Assignee: nobody → jones.chris.g
Attachment #377034 - Attachment is obsolete: true
Attachment #377034 - Flags: review?(benjamin)
Ran into a nasty C++ compiler issue.  I want to |friend| |NS_ShutdownXPCOM()| from within BlockingResourceBase, so that it can free the deadlock detector on exit.  Normally I would want to write:

[1]  friend nsresult NS_ShutdownXPCOM(...);

but g++ won't compile this, because it's treated as a forward declaration of a function in the |mozilla| namespace.  The solution is:

[2]  friend nsresult ::NS_ShutdownXPCOM(...);

But of course, Visual C++ won't compile this.  It treats the global namespace qualification as a syntax error.  So I tried [1] again, but it get the same error as in g++ (NS_ShutdownXPCOM() is treated a forward decl in |mozilla|).  I also thought maybe the visibility was affecting the decl, and also tried

[3]  friend NS_COM nsresult NS_ShutdownXPCOM(...);

but with no luck; I get the same error as [1].

So I think the "right" way to fix this is to put |ShutdownXPCOM()| in the |mozilla| namespace, drop the "NS_" stuff, and friend |ShutdownXPCOM()|.  We can keep around an |NS_ShutdownXPCOM()| that simply calls |mozilla::ShutdownXPCOM()|.

Any better ideas?
Attached patch Fix, v2 (obsolete) — Splinter Review
Builds on Windows and on linux w/ and w/o --enable-libxul
Attachment #377338 - Flags: review?(benjamin)
Comment on attachment 377338 [details] [diff] [review]
Fix, v2

>diff --git a/xpcom/build/nsXPCOM.h b/xpcom/build/nsXPCOM.h
>--- a/xpcom/build/nsXPCOM.h
>+++ b/xpcom/build/nsXPCOM.h

>+#ifdef __cplusplus
>+namespace mozilla {
>+XPCOM_API(nsresult)
>+ShutdownXPCOM(nsIServiceManager* servMgr);
>+}

This is not a frozen function and therefore doesn't belong in this file. Put it in nsXPCOMPrivate.h instead. (It shouldn't be XPCOM_API either, just a plain-old internal function)...

>diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp

>+#ifdef DEBUG
>+    BlockingResourceBase::Shutdown();
>+#endif

This is only going to operate on the BlockingResourceBase which is compiled into libxul/libxpcom_core.so. Anyone using external linkage will have their own copy of BlockingResourceBase statics which will not be freed by this method.

It sounds like what you really want is to be able to register a callback function to call at XPCOM shutdown.

>diff --git a/xpcom/glue/BlockingResourceBase.h b/xpcom/glue/BlockingResourceBase.h

>+    // so it can call Shutdown()
>+    friend NS_COM nsresult ShutdownXPCOM(nsIServiceManager*);

Because ShutdownXPCOM is going to be an internal function, this should be #ifdef MOZILLA_INTERNAL_API.
Attachment #377338 - Flags: review?(benjamin) → review-
(In reply to comment #4)
> (From update of attachment 377338 [details] [diff] [review])
> >diff --git a/xpcom/build/nsXPCOM.h b/xpcom/build/nsXPCOM.h
> >--- a/xpcom/build/nsXPCOM.h
> >+++ b/xpcom/build/nsXPCOM.h
> 
> >+#ifdef __cplusplus
> >+namespace mozilla {
> >+XPCOM_API(nsresult)
> >+ShutdownXPCOM(nsIServiceManager* servMgr);
> >+}
> 
> This is not a frozen function and therefore doesn't belong in this file. Put it
> in nsXPCOMPrivate.h instead. (It shouldn't be XPCOM_API either, just a
> plain-old internal function)...
> 

nsXPCOMPrivate.h scares the hell out of me.  Can I just add this declaration to it verbatim?  That's what my next patch will do, but I need more guidance if that's not the right thing.
Attached patch v3 (obsolete) — Splinter Review
Attachment #377338 - Attachment is obsolete: true
Attachment #378188 - Flags: review?(benjamin)
Attached patch v3, junk removedSplinter Review
Whoops, some unnecessary crap made it into the last patch.
Attachment #378188 - Attachment is obsolete: true
Attachment #378223 - Flags: review?(benjamin)
Attachment #378188 - Flags: review?(benjamin)
Attachment #378223 - Flags: review?(benjamin) → review+
Comment on attachment 378223 [details] [diff] [review]
v3, junk removed

>diff --git a/xpcom/build/nsXPCOMPrivate.h b/xpcom/build/nsXPCOMPrivate.h

>+#ifdef __cplusplus
>+namespace mozilla {

No need for cplusplus ifdefs, this header is a C++ header.
pushed
e3fd01315b9f
Chris Jones - bug 491977: fix "leak" of deadlock detector statics. r=bsmedberg
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.