This can cause memory leaks when there's a C++-only cycle and an nsXULPrototypeNode is the last thing to be released. I fixed it by converting nsXULPrototypeNode to be an nsISupports. I'm not sure if the nsRefPtr -> nsCOMPtr conversion is necessary or good. I just noticed that we have macros for easily traversing nsCOMPtrs but not nsRefPtrs.

Did you mean to include flipping the incremental pref? IIRC, you should only use nsCOMPtrs for interfaces. I'm not sure why exactly...

> I'm not sure why exactly... nsCOMPtr<T> asserts certain things about QI to NS_GET_IID(T). So if you use it on an interface, those asserts are somewhere between "misleading" and "wrong".

I mean "a non-interface".

It looks like you should be able to use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR for a ref ptr. It just does a get() on the field. NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, #_field); \ cb.NoteXPCOMChild(tmp->_field.get()); \

OK, fixed all that stuff.

Comment on attachment 619660 [details] [diff] [review] fix v2 > for (i = 0; i < elem->mChildren.Length(); ++i) { >- NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(elem->mChildren[i].get(), >- nsXULPrototypeNode, >- "mChildren[i]") >+ NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mChildren[i]"); >+ cb.NoteXPCOMChild(elem->mChildren[i].get()); Do you really need .get() here?

This was the only user of the NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS macro, so it should probably be removed at some point.

Can we add assertions or analyses to catch bugs like this before they show up as leaks?

This is a problem for all native cycle collected classes: http://mxr.mozilla.org/mozilla-central/ident?i=NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS&filter= I'm not sure why we don't see this more often. Anyways, I filed bug 750570 to about fixing it.

Is this worth porting to older branches? Are nsXULPrototypeNodes ever freed except at shutdown? If they are only freed at shutdown, it doesn't really seem worth doing.

Bug 723832 is tracking-firefox13+ and is duped here.

Comment on attachment 619660 [details] [diff] [review] fix v2 I think we should take this on the branches. [Approval Request Comment] Regression caused by (bug #): The leaks only showed up with the new tab page, but the problem has been there for a while. User impact if declined: Leaks Testing completed (on m-c, etc.): Been on m-c for several days. Risk to taking this patch (and alternatives if risky): Low risk String changes made by this patch: N/A

Comment on attachment 619660 [details] [diff] [review] fix v2 Low risk, stops some leakage - go ahead.

Can we land the fix today in preparation for tomorrow's beta go to build?

https://hg.mozilla.org/releases/mozilla-aurora/rev/90ff5aa6dfe5 https://hg.mozilla.org/releases/mozilla-beta/rev/37b8006c8a3d

Is there something QA can do to verify this fix?

nope

Okay, thanks Andrew.