Last Comment Bug 750424 - nsXULPrototypeNode::Release should add itself to the purple buffer
: nsXULPrototypeNode::Release should add itself to the purple buffer
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Bill McCloskey (:billm)
: Neil Deakin
: 723832 (view as bug list)
Depends on:
Blocks: 735099 751800 782485
  Show dependency treegraph
Reported: 2012-04-30 12:51 PDT by Bill McCloskey (:billm)
Modified: 2012-08-15 16:53 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (10.98 KB, patch)
2012-04-30 12:51 PDT, Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
fix v2 (8.17 KB, patch)
2012-04-30 13:13 PDT, Bill McCloskey (:billm)
bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2012-04-30 12:51:20 PDT
Created attachment 619655 [details] [diff] [review]

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] 2012-04-30 12:59:54 PDT
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 [:bz] (still a bit busy) 2012-04-30 13:05:54 PDT
> 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 [:bz] (still a bit busy) 2012-04-30 13:06:20 PDT
I mean "a non-interface".
Comment 4 Andrew McCreight [:mccr8] 2012-04-30 13:12:45 PDT
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 (:billm) 2012-04-30 13:13:36 PDT
Created attachment 619660 [details] [diff] [review]
fix v2

OK, fixed all that stuff.
Comment 6 Olli Pettay [:smaug] 2012-04-30 13:26:22 PDT
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 8 Andrew McCreight [:mccr8] 2012-04-30 17:07:57 PDT
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 2012-05-02 20:27:50 PDT
Can we add assertions or analyses to catch bugs like this before they show up as leaks?
Comment 10 Andrew McCreight [:mccr8] 2012-05-02 20:39:15 PDT
This is a problem for all native cycle collected classes:

I'm not sure why we don't see this more often.  Anyways, I filed bug 750570 to about fixing it.
Comment 12 Bill McCloskey (:billm) 2012-05-04 10:36:23 PDT
*** Bug 723832 has been marked as a duplicate of this bug. ***
Comment 13 Andrew McCreight [:mccr8] 2012-05-04 10:46:47 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-09 08:16:50 PDT
Bug 723832 is tracking-firefox13+ and is duped here.
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-09 08:18:19 PDT
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 2012-05-09 12:34:33 PDT
Comment on attachment 619660 [details] [diff] [review]
fix v2

Low risk, stops some leakage - go ahead.
Comment 17 Alex Keybl [:akeybl] 2012-05-14 09:04:21 PDT
Can we land the fix today in preparation for tomorrow's beta go to build?
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-14 11:56:13 PDT
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-29 11:20:15 PDT
Is there something QA can do to verify this fix?
Comment 20 Andrew McCreight [:mccr8] 2012-05-29 11:21:41 PDT
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-29 11:56:24 PDT
Okay, thanks Andrew.

Note You need to log in before you can comment on or make changes to this bug.