Closed Bug 750424 Opened 8 years ago Closed 8 years ago

nsXULPrototypeNode::Release should add itself to the purple buffer

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox13 + fixed
firefox14 + fixed

People

(Reporter: billm, Assigned: billm)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch fix (obsolete) — Splinter 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.
Attachment #619655 - Flags: review?(bugs)
Did you mean to include flipping the incremental pref?

IIRC, you should only use nsCOMPtrs for interfaces.  I'm not sure why exactly...
Component: XPCOM → XUL
OS: Linux → All
QA Contact: xpcom → xptoolkit.widgets
Hardware: x86_64 → All
Version: unspecified → Trunk
> 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());                                      \
Attached patch fix v2Splinter Review
OK, fixed all that stuff.
Attachment #619655 - Attachment is obsolete: true
Attachment #619655 - Flags: review?(bugs)
Attachment #619660 - Flags: review?(bugs)
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?
Attachment #619660 - Flags: review?(bugs) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/dae953b2f65a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 751800
Duplicate of this bug: 723832
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 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
Attachment #619660 - Flags: approval-mozilla-beta?
Attachment #619660 - Flags: approval-mozilla-aurora?
Comment on attachment 619660 [details] [diff] [review]
fix v2

Low risk, stops some leakage - go ahead.
Attachment #619660 - Flags: approval-mozilla-beta?
Attachment #619660 - Flags: approval-mozilla-beta+
Attachment #619660 - Flags: approval-mozilla-aurora?
Attachment #619660 - Flags: approval-mozilla-aurora+
Can we land the fix today in preparation for tomorrow's beta go to build?
Is there something QA can do to verify this fix?
Okay, thanks Andrew.
Whiteboard: [qa-]
Blocks: 782485
You need to log in before you can comment on or make changes to this bug.