The default bug view has changed. See this FAQ.

Suspect native cycle collected objects

RESOLVED FIXED in mozilla17

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(5 attachments, 8 obsolete attachments)

4.21 KB, patch
smaug
: review+
Details | Diff | Splinter Review
20.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
17.03 KB, patch
glandium
: review+
Details | Diff | Splinter Review
21.50 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.03 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
Another option is to make non-ISupports classes inherit from something that has a virtual GetParticipant method.
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.
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).
Whiteboard: [MemShrink] → [MemShrink:P2]
Blocks: 737976
(In reply to Bill McCloskey (:billm) from comment #2)
> Are there any advantages to a class not being nsISupports?

Non-virtual addref/release methods.
(Assignee)

Comment 5

5 years ago
Bug 766717 is another instance of this problem.
(Assignee)

Comment 6

5 years ago
Created attachment 638943 [details] [diff] [review]
remove unused stabilizeForDelection() arg
(Assignee)

Comment 7

5 years ago
Created attachment 638945 [details] [diff] [review]
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.
(Assignee)

Comment 8

5 years ago
Created attachment 639423 [details] [diff] [review]
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.
Attachment #638945 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 774874
(Assignee)

Updated

5 years ago
Attachment #638943 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 650713 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant

My attempt to split this into a few smaller patches.
(Assignee)

Comment 10

5 years ago
Created attachment 650949 [details] [diff] [review]
part 2: add purple buffer support for non-nsISupports classes, WIP
(Assignee)

Comment 11

5 years ago
Created attachment 651021 [details] [diff] [review]
part 3: remove unused stabilizeForDeletion arg

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

Comment 12

5 years ago
Created attachment 651045 [details] [diff] [review]
part 4: switch most native classes over to using the purple buffer

nsTimeout remains un-purplebuffered for the moment.
(Assignee)

Comment 13

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #639423 - Attachment is obsolete: true
I seem to remember that the leaks were more common on Windows.
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
Well, Moth doesn't seem to leak on Windows any more with that patch backed out.
(Assignee)

Comment 17

5 years ago
Created attachment 651491 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant
Attachment #650713 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
Created attachment 651555 [details] [diff] [review]
part 2: add purple buffer support for non-nsISupports classes
Attachment #650949 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 651609 [details] [diff] [review]
part 4: switch most native CC classes over to using the purple buffer
Attachment #651045 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 782485
(Assignee)

Updated

5 years ago
Blocks: 782735
(Assignee)

Comment 20

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

Comment 21

5 years ago
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.
Attachment #651491 - Flags: review?(mh+mozilla)
Attachment #651491 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #651555 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #651021 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #651609 - Flags: review?(bugs)
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?
Attachment #651491 - Flags: review?(mh+mozilla) → review+

Updated

5 years ago
Attachment #651491 - Flags: review?(bugs) → review+
(Assignee)

Comment 23

5 years ago
(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.
(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.
(Assignee)

Comment 25

5 years ago
(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?
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)
(Assignee)

Comment 27

5 years ago
Created attachment 652496 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant
Attachment #651491 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
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.
Attachment #652496 - Flags: review?(mh+mozilla)
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 :)
Attachment #652496 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 30

5 years ago
Yeah, I realized that after I uploaded it... Thanks.
(Assignee)

Comment 31

5 years ago
Created attachment 652500 [details] [diff] [review]
part 4: switch most native CC classes over to using the purple buffer

Unbitrotting from changes to part 1.
Attachment #651609 - Attachment is obsolete: true
Attachment #651609 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #652500 - Flags: review?(bugs)
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...
Attachment #651555 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #651021 - Flags: review?(bugs) → review+
(Assignee)

Comment 33

5 years ago
(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.
Addref/release show up in the micro-benchmarks.
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.
Attachment #652500 - Flags: review?(bugs) → review-
(Assignee)

Comment 36

5 years ago
(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.
(Assignee)

Updated

5 years ago
No longer depends on: 774874
(Assignee)

Comment 37

5 years ago
Created attachment 654758 [details] [diff] [review]
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.
Attachment #654758 - Flags: review?(bugs)
(Assignee)

Comment 38

5 years ago
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.
Attachment #652500 - Flags: review- → review?(bugs)

Updated

5 years ago
Attachment #654758 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #652500 - Flags: review?(bugs) → review+
(Assignee)

Comment 39

5 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.