750570 - Suspect native cycle collected objects

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Native (ie non-nsISupports) cycle collected classes aren't added to the purple buffer.  If they happen to be the last reference to a garbage cycle, we get leaks.  This is not merely a theoretical problem.  In bug 750424, billm figured out that he was getting shutdown leaks with incremental GC because nsXULPrototypeNode wasn't getting added to the purple buffer.  In bug 737976, khuey's deCOM of nsINodeInfo was causing random leaks, perhaps because nsNodeInfo was no longer added to the purple buffer.  It is possible there are other lurking random leaks due to this.

The solution is to either add a new purple buffer for native CCed objects, or to add a new field to the existing purple buffer for the cycle collector participants.  The latter solution might be easier, but would require more stores to happen on a pretty hot path, so I don't know how bad that would be.

I think the first thing to do is to look at what native classes are out there.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Another option is to make non-ISupports classes inherit from something that has a virtual GetParticipant method.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Are there any advantages to a class not being nsISupports? Kyle, in bug 737976, why couldn't you just make nsNodeInfo inherit directly from nsISupports? Then it could be a normal cycle-collected class. I realize this is similar to having a GetParticipant method, but it just seems like it requires less extra machinery.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If we remove nsINodeInfo then nsNodeInfo doesn't need a vtable and everything would be a word smaller.  (Doing comment 1 would negate that advantage, of course).

Comment 4

[Peter Van der Beken [:peterv]]

(In reply to Bill McCloskey (:billm) from comment #2)
> Are there any advantages to a class not being nsISupports?

Non-virtual addref/release methods.

Comment 5

[Andrew McCreight [:mccr8]]

Bug 766717 is another instance of this problem.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: remove unused stabilizeForDelection() arg

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: leaky WIP

This is pretty ugly.

I get a lot of these:
###!!! ASSERTION: nsTimeout not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/amccreight/mz/cent4/dom/base/nsGlobalWindow.cpp, line 9858

I added those checks so maybe it isn't too bad, but it seems a little skeezy that we're failing a thread safety check in a class we want to interact with the purple buffer.  Maybe I'll just leave nsTimeout unchanged.

This patch also seems to leak a lot.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: now with less leaking

This doesn't leak, at least with simple browsing.  I fixed the leaks by reverting my changes to nsTimeout, so they aren't being added to the purple buffer.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 1: lift UnmarkIfPurple to the CC participant

My attempt to split this into a few smaller patches.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 2: add purple buffer support for non-nsISupports classes, WIP

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 3: remove unused stabilizeForDeletion arg

The type of this argument has to be changed anyways, so we might as well just delete it.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 4: switch most native classes over to using the purple buffer

nsTimeout remains un-purplebuffered for the moment.

Comment 13

[Andrew McCreight [:mccr8]]

parts 1-4: https://tbpl.mozilla.org/?tree=Try&rev=c0c62395206c
just bug 750424 backed out: https://tbpl.mozilla.org/?tree=Try&rev=c2978b55a008
parts 1-4, 750424 backed out, then converted to a purple buffered class: https://tbpl.mozilla.org/?tree=Try&rev=c2978b55a008

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I seem to remember that the leaks were more common on Windows.

Comment 15

[Andrew McCreight [:mccr8]]

The base run was okay, but the other ones were broken due to my bad attempt at a backout.  We need to figure out some way to statically assert when you declare something is a script holder, but then you don't put a Trace function into the vtable. At least I think that's what was going on.

Anywho, this time I actually ran the browser with the backouts before I pushed to try. I also did Windows this time, per Bill's suggestion.

without purple buffer power: https://tbpl.mozilla.org/?tree=Try&rev=6b66ab8be6e6
with purple buffer power: https://tbpl.mozilla.org/?tree=Try&rev=0ff00b0e380f

If this works, next I'm going to do some blind taste testing with people walking down Castro St., to see if they can tell the difference.

Comment 16

[Andrew McCreight [:mccr8]]

Well, Moth doesn't seem to leak on Windows any more with that patch backed out.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: part 1: lift UnmarkIfPurple to the CC participant

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: part 2: add purple buffer support for non-nsISupports classes

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: part 4: switch most native CC classes over to using the purple buffer

Comment 20

[Andrew McCreight [:mccr8]]

Pushed to try, along with the patches for 782485 and 782735:
https://tbpl.mozilla.org/?tree=Try&rev=73aa90bca6bf

Comment 21

[Andrew McCreight [:mccr8]]

Comment on attachment 651491 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant

This patch just moved UnmarkIfPurple up to the top level participant class, because we are making it so that all CC classes can be purple. It also removes some unused functions.

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 651491 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant

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

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +669,2 @@
>    {                                                                            \
> +    Downcast(static_cast<nsISupports *>(s))->UnmarkIfPurple();                 \

Side note: at some point, we should be able to use _class * instead of void *, here.

@@ -887,5 @@
>    const CCParticipantVTable<NS_CYCLE_COLLECTION_CLASSNAME(_class)>             \
>      ::Type _class::NS_CYCLE_COLLECTION_INNERNAME =                             \
>    { NS_IMPL_CYCLE_COLLECTION_NATIVE_VTABLE(NS_CYCLE_COLLECTION_CLASSNAME(_class)) };
>  
> -#define NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(_class)            \

Why remove this one? Because it would only be used in nsXPConnect, and isn't?

Comment 23

[Andrew McCreight [:mccr8]]

(In reply to Mike Hommey [:glandium] from comment #22)
> Why remove this one? Because it would only be used in nsXPConnect, and isn't?

I didn't actually remove it, I just moved it down a line. It is now the same as NS_IMPL_CYCLE_COLLECTION_CLASS. NS_IMPL_CYCLE_COLLECTION_CLASS is much more common, so I wanted to reduce the amount of indirection in the common case, so I defined NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS in terms of NS_IMPL_CYCLE_COLLECTION_CLASS rather than vice versa.

Comment 24

[Mike Hommey [:glandium]]

(In reply to Andrew McCreight [:mccr8] from comment #23)
> (In reply to Mike Hommey [:glandium] from comment #22)
> > Why remove this one? Because it would only be used in nsXPConnect, and isn't?
> 
> I didn't actually remove it, I just moved it down a line. It is now the same
> as NS_IMPL_CYCLE_COLLECTION_CLASS. NS_IMPL_CYCLE_COLLECTION_CLASS is much
> more common, so I wanted to reduce the amount of indirection in the common
> case, so I defined NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS in
> terms of NS_IMPL_CYCLE_COLLECTION_CLASS rather than vice versa.

Ah, I got confused by the simultaneous renaming, which actually bothers me. The reason why these macros are named this way is that they match the macros for NS_DECL_*. If you still want to rename, though, you should also change the VTABLE macro names in comments above the nsCycleCollectionParticipantType enum.

Comment 25

[Andrew McCreight [:mccr8]]

(In reply to Mike Hommey [:glandium] from comment #24)
> Ah, I got confused by the simultaneous renaming, which actually bothers me.
> The reason why these macros are named this way is that they match the macros
> for NS_DECL_*. If you still want to rename, though, you should also change
> the VTABLE macro names in comments above the
> nsCycleCollectionParticipantType enum.

The main IMPL macros should still match the main DECL macros, the change I made only made the VTABLE ones be out of sync, as I was viewing them as an internal detail. But you are right, it would be better to just do this:

#define NS_IMPL_CYCLE_COLLECTION_VTABLE(_class) \
NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_VTABLE(_class)

and maintain the parallel structure everywhere.  At least, I think this is what you mean?

Comment 26

[Mike Hommey [:glandium]]

If you mean the following, then yes

diff --git a/xpcom/glue/nsCycleCollectionParticipant.h b/xpcom/glue/nsCycleCollectionParticipant.h
--- a/xpcom/glue/nsCycleCollectionParticipant.h
+++ b/xpcom/glue/nsCycleCollectionParticipant.h
@@ -868,22 +868,22 @@ struct Skippable
   {                                                                            \
     &_class::TraverseImpl,                                                     \
     &_class::RootImpl,                                                         \
     &_class::UnlinkImpl,                                                       \
     &_class::UnrootImpl,                                                       \
+    &_class::UnmarkIfPurpleImpl,                                               \
     _class::isSkippable ? &Skippable<_class>::CanSkipImpl : NULL,              \
     _class::isSkippable ? &Skippable<_class>::CanSkipInCCImpl : NULL,          \
     _class::isSkippable ? &Skippable<_class>::CanSkipThisImpl : NULL           \
   }
  
 #define NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_VTABLE(_class)           \
   NS_IMPL_CYCLE_COLLECTION_NATIVE_VTABLE(_class),                              \
   { &_class::TraceImpl }
 
 #define NS_IMPL_CYCLE_COLLECTION_VTABLE(_class)                                \
-  NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_VTABLE(_class),                \
-  { &_class::UnmarkIfPurpleImpl }
+  NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_VTABLE(_class)
 
 #define NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS(_class)                          \
   const CCParticipantVTable<NS_CYCLE_COLLECTION_CLASSNAME(_class)>             \
     ::Type _class::NS_CYCLE_COLLECTION_INNERNAME =                             \
   { NS_IMPL_CYCLE_COLLECTION_NATIVE_VTABLE(NS_CYCLE_COLLECTION_CLASSNAME(_class)) };


Although NS_IMPL_CYCLE_COLLECTION_CLASS could be changed, too:
#define NS_IMPL_CYCLE_COLLECTION_CLASS(_class) \
  NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(_class)

Comment 27

[Andrew McCreight [:mccr8]]

Attachment: part 1: lift UnmarkIfPurple to the CC participant

Comment 28

[Andrew McCreight [:mccr8]]

Comment on attachment 652496 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant

How's this?  I only changed the stuff starting from:
// VTables for the cycle collector participant implementations.

I like having NS_IMPL_CYCLE_COLLECTION_CLASS be defined in terms of NS_IMPL_CYCLE_COLLECTION_VTABLE, because then any changes to its _VTABLE will be automatically reflected.

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 652496 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant

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

That's what i wrote in comment 26 :)

Comment 30

[Andrew McCreight [:mccr8]]

Yeah, I realized that after I uploaded it... Thanks.

Comment 31

[Andrew McCreight [:mccr8]]

Attachment: part 4: switch most native CC classes over to using the purple buffer

Unbitrotting from changes to part 1.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 651555 [details] [diff] [review]
part 2: add purple buffer support for non-nsISupports classes






> struct nsPurpleBufferEntry {
>   union {
>-    nsISupports *mObject;                 // when low bit unset
>+    void *mObject;                        // when low bit unset
>     nsPurpleBufferEntry *mNextInFreeList; // when low bit set
>   };
>   // When an object is in the purple buffer, it replaces its reference
>   // count with a (tagged) pointer to this entry, so we store the
>   // reference count for it.
>   nsrefcnt mRefCnt;
>+  nsCycleCollectionParticipant *mParticipant; // NULL for nsISupports
> };
So, this increases purple buffer's memory usage. It shouldn't be usually horribly bad though.
But, since we get mParticipant, we should utilize it fully... is there any way we could pass the
participant also in nsISupports case? It would be great if we wouldn't have to QI nsCycleCollectionISupports
when running CC. Maybe not any fast way.

Have you profiled addref/release? Do your patches slow them down? (addref/release are already slower than we'd like them to be.)

code looks good, r=me for that, but please do some profiling, just in case...

Comment 33

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #32)
> So, this increases purple buffer's memory usage. It shouldn't be usually
> horribly bad though.

Yeah, we fire off a CC after 5 seconds when there are 100 or so things in the purple buffer, so it never gets that big.

> But, since we get mParticipant, we should utilize it fully... is there any
> way we could pass the
> participant also in nsISupports case? It would be great if we wouldn't have
> to QI nsCycleCollectionISupports
> when running CC. Maybe not any fast way.

I'm not sure.  It would require redefining AddRef/Release for every class that has its own participant. I'm not sure if that is done currently. I don't think QI takes much time in the CC.

> Have you profiled addref/release? Do your patches slow them down?
> (addref/release are already slower than we'd like them to be.)
> 
> code looks good, r=me for that, but please do some profiling, just in case...

How should I do that?  at a micro level with something like Shark, or look at the broader setting with Talos?  Both?

For nsISupports classes, this is only going to add an additional NULL store (with good locality: it is right next to another store we're already doing) in the slow case when we're putting things in the purple buffer, so I think it shouldn't be too bad, but you are right, I should try to measure it.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Addref/release show up in the micro-benchmarks.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 652500 [details] [diff] [review]
part 4: switch most native CC classes over to using the purple buffer

> 
>-NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS(nsTimeout)
>+NS_IMPL_CYCLE_COLLECTION_NON_PURPLE_NATIVE_CLASS(nsTimeout)
> NS_IMPL_CYCLE_COLLECTION_UNLINK_NATIVE_0(nsTimeout)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_BEGIN(nsTimeout)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mWindow,
>                                                        nsIScriptGlobalObject)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mPrincipal)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mScriptHandler)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(nsTimeout, AddRef)
>diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h
>--- a/dom/base/nsGlobalWindow.h
>+++ b/dom/base/nsGlobalWindow.h
>@@ -133,17 +133,17 @@ NS_CreateJSTimeoutHandler(nsGlobalWindow
>  * timeout.  Holds a strong reference to an nsIScriptTimeoutHandler, which
>  * abstracts the language specific cruft.
>  */
> struct nsTimeout : PRCList
> {
>   nsTimeout();
>   ~nsTimeout();
> 
>-  NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(nsTimeout)
>+  NS_DECL_CYCLE_COLLECTION_NON_PURPLE_NATIVE_CLASS(nsTimeout)
> 
>   nsrefcnt Release();
>   nsrefcnt AddRef();
> 
>   nsTimeout* Next() {
>     // Note: might not actually return an nsTimeout.  Use IsTimeout to check.
>     return static_cast<nsTimeout*>(PR_NEXT_LINK(this));
>   }



> #endif // nsTransactionItem_h__
>diff --git a/xpcom/glue/nsCycleCollectionParticipant.h b/xpcom/glue/nsCycleCollectionParticipant.h
>--- a/xpcom/glue/nsCycleCollectionParticipant.h
>+++ b/xpcom/glue/nsCycleCollectionParticipant.h
>@@ -854,16 +854,19 @@ struct Skippable
> 
> // Cycle collector participant implementations.
> 
> #define NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS(_class)                          \
>   const CCParticipantVTable<NS_CYCLE_COLLECTION_CLASSNAME(_class)>             \
>     ::Type _class::NS_CYCLE_COLLECTION_INNERNAME =                             \
>   { NS_IMPL_CYCLE_COLLECTION_NATIVE_VTABLE(NS_CYCLE_COLLECTION_CLASSNAME(_class)) };
> 
>+#define NS_IMPL_CYCLE_COLLECTION_NON_PURPLE_NATIVE_CLASS(_class)               \
>+  NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS(_class)
>+


>+#define NS_DECL_CYCLE_COLLECTION_NON_PURPLE_NATIVE_CLASS(_class)               \
>+  class NS_CYCLE_COLLECTION_INNERCLASS                                         \
>+   : public nsCycleCollectionParticipant                                       \
>+  {                                                                            \
>+     NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS_BODY(_class)                        \
>+     NS_DECL_CYCLE_COLLECTION_STUB_UNMARK_IF_PURPLE(_class)                    \
>+  };                                                                           \
>+  NS_CYCLE_COLLECTION_PARTICIPANT_INSTANCE
>+

_NON_PURPLE_ is really odd. It assume it won't be clear to API users. needs some documentation.

Comment 36

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #35)
> _NON_PURPLE_ is really odd. It assume it won't be clear to API users. needs
> some documentation.

Good point. Hopefully it is just a weird legacy thing that only nsTimeout will need, but I should still describe it.

Comment 37

[Andrew McCreight [:mccr8]]

Attachment: part 4b: LEGACY instead of NON_PURPLE

I'll land this folded into part 4, but I'm leaving it separate here to make it easier to review.

I renamed the NON_PURPLE macros to LEGACY, and added some description (perhaps in the wrong place in the file) what these various kinds of cycle collected classes are for.

Comment 38

[Andrew McCreight [:mccr8]]

Comment on attachment 652500 [details] [diff] [review]
part 4: switch most native CC classes over to using the purple buffer

patch 4 is unchanged from before, but will be modified with the stuff in 4b when I land it.

Comment 39

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/786f414ea726
https://hg.mozilla.org/integration/mozilla-inbound/rev/9077df78db40
https://hg.mozilla.org/integration/mozilla-inbound/rev/24368b894189
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecbb10bf79b3

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/786f414ea726
https://hg.mozilla.org/mozilla-central/rev/9077df78db40
https://hg.mozilla.org/mozilla-central/rev/24368b894189
https://hg.mozilla.org/mozilla-central/rev/ecbb10bf79b3