Note: There are a few cases of duplicates in user autocompletion which are being worked on.

nsXULPrototypeNode::Release should add itself to the purple buffer

RESOLVED FIXED in Firefox 13

Status

()

Core
XUL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13+ fixed, firefox14+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 619655 [details] [diff] [review]
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.
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

Comment 2

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

Updated

5 years ago
Blocks: 735099

Comment 3

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

Comment 5

5 years ago
Created attachment 619660 [details] [diff] [review]
fix v2

OK, fixed all that stuff.
Attachment #619655 - Attachment is obsolete: true
Attachment #619655 - Flags: review?(bugs)
Attachment #619660 - Flags: review?(bugs)

Comment 6

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

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae953b2f65a
Target Milestone: --- → mozilla15
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

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 751800
(Assignee)

Updated

5 years ago
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.
Bug 723832 is tracking-firefox13+ and is duped here.
status-firefox13: --- → affected
status-firefox14: --- → affected
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
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?
tracking-firefox13: ? → +
tracking-firefox14: ? → +
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?
https://hg.mozilla.org/releases/mozilla-aurora/rev/90ff5aa6dfe5
https://hg.mozilla.org/releases/mozilla-beta/rev/37b8006c8a3d
status-firefox13: affected → fixed
status-firefox14: affected → fixed
Is there something QA can do to verify this fix?
nope
Okay, thanks Andrew.
Whiteboard: [qa-]
Blocks: 782485
You need to log in before you can comment on or make changes to this bug.