750424 - nsXULPrototypeNode::Release should add itself to the purple buffer

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

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

Attachment: fix

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.

Comment 1

[Andrew McCreight [:mccr8]]

Did you mean to include flipping the incremental pref?

IIRC, you should only use nsCOMPtrs for interfaces.  I'm not sure why exactly...

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 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".

Comment 3

[Boris Zbarsky [:bzbarsky]]

I mean "a non-interface".

Comment 4

[Andrew McCreight [:mccr8]]

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());                                      \

Comment 5

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

Attachment: fix v2

OK, fixed all that stuff.

Comment 6

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

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?

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/dae953b2f65a

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Jesse Ruderman]

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

Comment 10

[Andrew McCreight [:mccr8]]

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.

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/dae953b2f65a

Comment 13

[Andrew McCreight [:mccr8]]

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.

Comment 14

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

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

Comment 15

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

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 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 619660 [details] [diff] [review]
fix v2

Low risk, stops some leakage - go ahead.

Comment 17

[Alex Keybl [:akeybl]]

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

Comment 18

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

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

Comment 19

[u279076]

Is there something QA can do to verify this fix?

Comment 20

[Andrew McCreight [:mccr8]]

nope

Comment 21

[u279076]

Okay, thanks Andrew.