Closed Bug 52726 Opened 24 years ago Closed 23 years ago

XUL roots script objects for elements not in documents

Categories

(Core :: XUL, defect, P2)

defect

Tracking

()

VERIFIED INVALID
mozilla0.9.2

People

(Reporter: dbaron, Assigned: waterson)

References

Details

(Keywords: embed, memory-leak, Whiteboard: [rtm-])

Attachments

(3 files)

This bug is the cause of bug 51177. nsXULElement::GetScriptObject calls AddNamedReference whether or not mDocument is null. If the document is null and the element is never added to a document, this means that the element will stay rooted forever and will cause the entire document to leak. I think the only change needed to fix this bug is to make nsXULElement::GetScriptObject work like nsGenericElement::GetScriptObject and only call AddNamedReference if the document is non-null. I think nsXULElement::SetDocument already works right.
The cause of this bug is simple, and the fix is, I think, to move one line down 2 lines into a block. Nominating for nsbeta3, since this may be the cause of other leaks and is certainly a serious bug in the toolkit.
Keywords: mlk, nsbeta3
*** Bug 52723 has been marked as a duplicate of this bug. ***
Yes, that sounds right. Do you want me to fix it? Or can you do the honors...
Could you? I don't have a computer right now, and sshing to home doesn't really allow much testing...
no problem.
Status: NEW → ASSIGNED
Attached patch proposed fix (deleted) — Splinter Review
hyatt, jst, dbaron: love me up with some r=?
Hmmm. I tried this patch without the fix to bug 51177, and I still see the mail compose leak with a treechildren root.
There are two separate problems. I filed bug 53901 on the other one. r=dbaron on the patch above.
Huge potential for leakage. Marking rtm+.
Keywords: embed, rtm
Whiteboard: [rtm+]
Priority: P3 → P2
Marking rtm++. Please check this in as soon as possible.
Whiteboard: [rtm+] → [rtm++]
I take it back. This fix is way more complicated than I originally thought. I'm gonna de-nominate it. :-( See bug 53901 (its evil twin) for more details.
Whiteboard: [rtm++] → [rtm-]
This looks like something we want for mozilla 1.0, even if we can't have it in Netscape's RTM.
Keywords: mozilla1.0
*** Bug 40480 has been marked as a duplicate of this bug. ***
I suspect that fixing bug 71483 will make this bug irrelevant; once that bug is fixed, I'll re-evaluate.
Depends on: 71483
Target Milestone: --- → mozilla0.9.1
Uh, the bug you mentioned is assignee == nobody@mozilla.org. That translates to me as we won't get to it this release, eh?
I dunno. I never thought of 'jst' == 'nobody'. The DOM/XPConnect stuff will land in the next milestone, AFAIK. (On the other hand, the QA contact is indeed nobody@mozilla.org).
Sorry, I misread the fields. Just trying to make sure it gets in for 6.5.
Depends on: 77232
No longer depends on: 77232
Blocks: 77248
Target Milestone: mozilla0.9.1 → mozilla0.9.2
*** Bug 78927 has been marked as a duplicate of this bug. ***
*** Bug 82267 has been marked as a duplicate of this bug. ***
FWIW, all the root management code in nsXULElement is no longer used (it's under #if 0, pending me or jst getting around to ripping it out). So I'm fairly sure that this bug is going to get marked INVALID. I'm not sure people are marking the mail-filtering bugs as dups of this...
Because a couple of months ago you marked 40480 as a dup of this bug. Is this no longer true?
Clever me! In bug 40480, hyatt says, ``The exact problem here is that elements that are created using createElement do not have their mDocument member pointers set until they are actually inserted into the document. Note that for elements created using cloneNode we do set the mDocument member pointer. We are being inconsistent here./The fix IMO for this bug is to ensure that mDocument is set immediately upon the creation of the element. This will fix the bug.'' I'll attach a patch that does just that.
Chris did you need help testing this out for the filters case? Adding Seth and Navin to see if they can verify this fixes 40480?
Sorry to rush in here, but I *believe* we don't set mDocument neither in HTML nor in XML. Doing this only in XUL would be illogical, imho. jst, you can probably tell more precisely than me the consequences of this patch, but this seems quite a big change to me. Please correct me as usual if my fingers type incorrect things.
jst and I just talked about this, and I'm pretty convinced that we _cannot_ set mDocument on an element that isn't in the document. Since the element holds a weak reference to the document, doing so could leave a dangling reference from the element. For example, var element = document.createElement("foo"); /* Store element somewhere, but never insert it into the document. Later, document is destroyed; e.g., because window is closed. But now we grab our stored element, and... */ var doc = element.document; /* crash! doc points to garbage! */ If this _does_ work work today, it probably only does so because the document is still reachable via the JS object's parent slot. Other language bindings might not work this way. (Certainly, if the element is created from native code, and the above situation occurs, we're screwed...) So, that's a long winded way of saying this is probably a WONTFIX, and nsXULElement::CloneNode() should _not_ be setting mDocument, and that we'll need re-examine XBL's dependencies on these ``bugs''. hyatt: is it time for me you and jst to have a cage match?
Another perspective in the fight is that the DOM Level 1 (and 2) spec says that all nodes have an |ownerDocument| property, whether they're in the content tree or not. (It is null only for a Document or for a DocumentType that isn't yet associated with a document.) Perhaps a solution that would be compliant with the DOM spec would be to have the document keep a list of all of its extra content trees that are not children of the Document node, as I suggested in bug 52732.
10 dups (including dups^2). Marking mostfreq.
Keywords: mostfreq
David B., the .ownerDocument problem is solvable (and solved) for element nodes w/o needing a list of nodes not part of the document in the document, if you look at nsGenericElement::GetOwnerDocument() you'll see that it gets the document from the nsINodeInfo if mDocument is nsnull. The problem still remains for non-element nodes, tho. So what exactly is this bug about nowadays anyway? We really shouldn't be rooting script objects any more, did this bug morph into something other than the subject says, what's the deal?
This bug is really about the stuff that got dup'd to it. In particular, bug 40480. We should probably close this bug and re-open that one. Yeah, why don't I do that.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → INVALID
I'll verify this one... Trusting Waterson... ;)
Status: RESOLVED → VERIFIED
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: