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)
Core
XUL
Tracking
()
VERIFIED
INVALID
mozilla0.9.2
People
(Reporter: dbaron, Assigned: waterson)
References
Details
(Keywords: embed, memory-leak, Whiteboard: [rtm-])
Attachments
(3 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/xul
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•24 years ago
|
||
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.
Assignee | ||
Comment 3•24 years ago
|
||
Yes, that sounds right. Do you want me to fix it? Or can you do the honors...
Reporter | ||
Comment 4•24 years ago
|
||
Could you? I don't have a computer right now, and sshing to home doesn't really
allow much testing...
Assignee | ||
Comment 6•24 years ago
|
||
Assignee | ||
Comment 7•24 years ago
|
||
hyatt, jst, dbaron: love me up with some r=?
Reporter | ||
Comment 8•24 years ago
|
||
Hmmm. I tried this patch without the fix to bug 51177, and I still see the mail
compose leak with a treechildren root.
Reporter | ||
Comment 9•24 years ago
|
||
Reporter | ||
Comment 10•24 years ago
|
||
There are two separate problems. I filed bug 53901 on the other one.
r=dbaron on the patch above.
Assignee | ||
Comment 11•24 years ago
|
||
Huge potential for leakage. Marking rtm+.
Assignee | ||
Updated•24 years ago
|
Priority: P3 → P2
Comment 12•24 years ago
|
||
Marking rtm++. Please check this in as soon as possible.
Whiteboard: [rtm+] → [rtm++]
Assignee | ||
Comment 13•24 years ago
|
||
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
Assignee | ||
Comment 15•24 years ago
|
||
*** Bug 40480 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 16•24 years ago
|
||
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
Comment 17•24 years ago
|
||
Uh, the bug you mentioned is assignee == nobody@mozilla.org.
That translates to me as we won't get to it this release, eh?
Comment 18•24 years ago
|
||
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).
Comment 19•24 years ago
|
||
Sorry, I misread the fields. Just trying to make sure it gets in for 6.5.
Assignee | ||
Updated•24 years ago
|
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Comment 20•23 years ago
|
||
*** Bug 78927 has been marked as a duplicate of this bug. ***
Comment 21•23 years ago
|
||
*** Bug 82267 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 22•23 years ago
|
||
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...
Comment 23•23 years ago
|
||
Because a couple of months ago you marked 40480 as a dup of this bug. Is this
no longer true?
Assignee | ||
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
Comment 26•23 years ago
|
||
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?
Comment 27•23 years ago
|
||
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.
Assignee | ||
Comment 28•23 years ago
|
||
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?
Reporter | ||
Comment 29•23 years ago
|
||
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.
Comment 31•23 years ago
|
||
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?
Assignee | ||
Comment 32•23 years ago
|
||
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
Comment 33•23 years ago
|
||
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.
Description
•