Closed
Bug 53901
Opened 24 years ago
Closed 17 years ago
nsXULElement::CloneNode sets IS_IN_DOCUMENT flag
Categories
(Core :: XUL, defect, P2)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha5
People
(Reporter: dbaron, Assigned: sicking)
References
Details
(Keywords: embed, memory-leak, Whiteboard: [rtm-])
Attachments
(4 files, 3 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
nsXULElement::CloneNode (unlike all the other CloneNode methods, I think), sets
the |mDocument| of the cloned XUL element to non-null in some cases. This means
that if a JS object for the cloned node is created and it is never added to the
document tree, it leaks, possibly taking the whole document with it. See bug
52726.
The possible solutions I can think of are:
* not set mDocument to non-null (maybe override later). Will this cause other
problems?
* fix bug 52732 instead
Reporter | ||
Comment 1•24 years ago
|
||
Reporter | ||
Comment 3•24 years ago
|
||
I confirmed that the call to nsXULElement::CloneNode immediately before the call
to JS_AddNamedRoot took the branch where mPrototype is nonzero (the one with the
problem).
Comment 4•24 years ago
|
||
This, along with bug 52726, have a huge potential for leakage. Adding
"embedding" keyword because I'm sure jud'll be interested. We should fix for RTM.
Comment 5•24 years ago
|
||
Comment 6•24 years ago
|
||
I think the above patch is the fix: I'm testing it now.
nsXULElement::Create() should allow aDocument to be null; if it is null, then
the event handlers and popup stuff'll be set up upon addition to the document
(see nsXULDocument::AddSubtreeToDocument). With that done, we can now safely
pass nsnull as the aDocument argument when calling nsXULElement::CreateElement()
from nsXULElement::Clone().
Updated•24 years ago
|
Priority: P3 → P2
Comment 7•24 years ago
|
||
Hrmph. That patch ain't ready for primetime. We're now dropping JS objects.
Comment 8•24 years ago
|
||
I suspected as much. Is it because of GetScriptObject calls and consequent data
dependencies on the one true script object for a content node being made without
a JS reference, and then JS_GC runs? Or are we "dropping" script objects even
with JS references?
/be
Comment 9•24 years ago
|
||
hyatt, I'm pretty sure my fix is right, but I'm having trouble getting XBL to
co-operate. After calling cloneNode(), nsXBLBinding::GenerateAnonymousContent()
calls SetDocument(), which it should. One problem that I've noticed is that
AllowScripts() is always returning false, even in chrome. But even if I
hard-code SetDocument(..., PR_TRUE) to force scripts to be allowed, I'm ending
up with properties being undefined sometimes.
Comment 10•24 years ago
|
||
Ok, tried making nsXBLElement::AllowScripts() return PR_TRUE *all* the time.
With this set, some of the auto-complete stuff works in the URL bar and the
addressing widget, but the binding for the return key is busted in the
addressing widget. (Without AllowScripts() hardwired to true, *none* of the
auto-complete stuff works.)
Comment 11•24 years ago
|
||
brendan: to answer your question, AFAICT, XBL is trying to make anonymous
content "be in the document". So I suspect that this is what is failing.
Comment 12•24 years ago
|
||
(from mail to reviewers@mozilla.org...)
Vidur Apparao wrote:
> I don't believe it's a serious issue for HTML and generic XML elements. Most
> calls to GetScriptObject from non-idlc-generated code are from call points
> that are themselves directly called from idlc-generated glue code
> (nsIJSScriptObject methods, for example).
Ugh, it is an issue with HTML, but I'm not sure how serious. For example, the
HTML below will lose the property set on the "br" element that's supposed to be
hanging off of Node.
XBL is potentially quite screwed. If we fix the leak, then anyone who holds on
to XBL-generated content risks losing properties when an element is removed from
the document (addressing widget breaks mightily, for example). If we leave the
leak in, then we've got a time bomb waiting to leak megabytes of memory.
Thoughts?
chris
<html>
<head>
<script language="JavaScript">
var Node;
function setProperty()
{
// create a <span> element that we'll never put it in
// the document.
Node = document.createElement("span");
// The only JS ref will be from this local var, which'll
// die after the function leaves scope.
var child = document.createElement("br");
child.randomProperty = "random property!";
Node.appendChild(child);
// Kill JS's newborn slot
document.createElement("br");
// Now Node.firstChild's JS object is dangling, and
// will get scooped up at the next GC point!
}
function getProperty()
{
// try running this before, and then after, a GC has run!
document.getElementById("propertyValue").value =
Node.firstChild.randomProperty;
}
</script>
</head>
<body onload="setProperty();">
<form>
<input id="propertyValue" type="text"></input>
</form>
<button onclick="getProperty();">Get the property!</button>
</body>
</html>
Comment 13•24 years ago
|
||
Marking "needinfo." Will reconsider for inclusion once there is a reviewed and
super reviewed patch.
Whiteboard: [rtm+] → [rtm+ needinfo]
Comment 14•24 years ago
|
||
Don't screw XBL! What did it ever do to you? :)
Updated•24 years ago
|
Whiteboard: [rtm+ needinfo] → [rtm-]
Comment 15•24 years ago
|
||
Denominating this bug because of the mess it entails. We'll fix this later. In
the meantime, be careful with Clone'd elements!
I think we want to have this for mozilla1.0, though I'll believe that it's going
to miss the NS6.0 RTM train.
Keywords: mozilla1.0
Comment 17•24 years ago
|
||
I suspect that bug 71483 will make fixing this bug irrelevant. I'll re-evaluate
once the DOM is XPConnected.
Depends on: 71483
Target Milestone: --- → mozilla0.9.1
Comment 18•24 years ago
|
||
mDocument must be non-null here. In fact, I deliberately patched CloneNode so
that it is non-null along both the lightweight and heavyweight paths. You will
bust mailcompose if you don't ensure a non-null mDocument immediately.
Comment 19•24 years ago
|
||
We need to fix it so that nodes never have a null mDocument when created using
createElement or cloneNode.
Comment 20•24 years ago
|
||
Allright, let me re-state that. Fixing bug 71483 will hopefully untangle the
``should this script object be rooted or not'' issue from the mDocument pointer.
I'll re-examine once jst lands.
Updated•24 years ago
|
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Updated•24 years ago
|
Target Milestone: mozilla0.9.2 → mozilla1.0
Comment 21•23 years ago
|
||
Just emphasizing that many things will break if this bug summary is taken
literally (and fixed by making the new cloned subtree have a null mDocument ptr).
IMO mDocument should not be null for both created elements and cloned elements.
Reporter | ||
Comment 22•23 years ago
|
||
See also bug 52732.
Updated•23 years ago
|
Target Milestone: mozilla1.0 → Future
Comment 23•21 years ago
|
||
OK, so my short take on this if we don't plan to change nsIContent is:
1) We should absolutely fix this bug as described in the summary
2) We should fix XBL to stop making some rather incorrect assumptions about how
document pointers work in gecko. That will prevent the mailnews bustage
hyatt speaks of.
That will allow me to sorta work on fixing bug 211128 (creating BindToTree()).
See bug 52732 and its dependencies, however. It may be worth it to consider
changing nsIContent to expose two methods:
1) GetOwnerDocument()
2) IsInDocument()
and convert callers to use these explicitly. It's very rare that someone
actually wants to get the "document this node is attached to, but only if it's
in a document", I would think....
Comment 24•21 years ago
|
||
Just a small note: my patch in bug 27382 (attachment 146411 [details] [diff] [review]) adds InDocument()
to nsGenericElement.
Comment 25•21 years ago
|
||
Right. That's what got me thinking about the api nsIContent should expose...
note that GetOwnerDocument (returning nsIDocument) is also already on
nsGenericElement.
Assignee | ||
Comment 26•21 years ago
|
||
> 1) GetOwnerDocument()
We might want some other name since otherwise we'll get hide-warnings (and
possibly compile errors) from collisions with the nsIDOMNode::GetOnwerDocument
function.
Maybe GetOwnerDoc() like GetAttr vs. GetAttribute
Updated•20 years ago
|
Comment 27•20 years ago
|
||
Comment 28•19 years ago
|
||
Once this is fixed, we should back out the code in nsGenericElement::doReplaceOrInsertBefore that this bug necessitates.
Updated•18 years ago
|
Assignee: waterson → nobody
Status: ASSIGNED → NEW
QA Contact: jrgmorrison → xptoolkit.xul
Assignee | ||
Comment 30•18 years ago
|
||
This should do it. I'm a bit nervous that the change to make XUL elements not call GetChildCount() during BindToTree will cause badness somehow, but I assert without it, and it seems like the right thing to do.
Attachment #15452 -
Attachment is obsolete: true
Attachment #162584 -
Attachment is obsolete: true
Assignee | ||
Comment 31•18 years ago
|
||
Same as above, diffed with -w
Attachment #264558 -
Flags: superreview?(bzbarsky)
Attachment #264558 -
Flags: review?(bzbarsky)
Comment 32•18 years ago
|
||
It'll might take me a week or so to get to this; I'm pretty swamped right now, and have a lot of reviews on my plate. :(
Assignee | ||
Updated•18 years ago
|
Attachment #264558 -
Attachment description: Patch to fix -2 → Patch to fix -w
Assignee | ||
Comment 33•18 years ago
|
||
bz, any input would be great, i.e. things to look out for or things that failed when you last attempted this (i tried tabbed browsing and that worked fine).
In the meantime I can ask someone else to review it.
Assignee | ||
Comment 34•18 years ago
|
||
also, do you know the answer to the question in nsGenericElement::UnbindFromTree in the patch?
Comment 35•18 years ago
|
||
All the problems I ran into when I last touched this code stemmed from applying XBL bindings to nodes created via createElement() before they were inserted into the DOM. Since this patch doesn't do it, we should be ok. ;)
I was sure we had discussion somewhere about the GetChildCount() call in BindToTree, but I guess not... Taking that out does make sense.
As for UnbindFromTree, I think we should try making that assert fire for XUL as well. If we hit it, we should figure out why. I do think it was mostly, if not always, firing because of this bug.
Comment 36•18 years ago
|
||
Oh, why check the namespace ID in UnbindFromTree instead of using the FromContent() returning nsXULElement or just IsContentOfType directly?
Assignee | ||
Comment 37•18 years ago
|
||
Comment on attachment 264558 [details] [diff] [review]
Patch to fix -w
Jst, would be great if I could get this reviewed for alpha5.
The answer to the question at the beginning of nsGenericElement::UnbindFromTree seems to be "no" so I'll change that before checkin
Attachment #264558 -
Flags: superreview?(jst)
Attachment #264558 -
Flags: superreview?(bzbarsky)
Attachment #264558 -
Flags: review?(jst)
Attachment #264558 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 38•18 years ago
|
||
Oh, and I'll use the FromContent trick too.
Assignee | ||
Updated•18 years ago
|
Summary: nsXULElement::CloneNode sets non-null mDocument → nsXULElement::CloneNode sets IS_IN_DOCUMENT flag
Comment 39•18 years ago
|
||
Comment on attachment 264558 [details] [diff] [review]
Patch to fix -w
Looks good to me, r+sr=jst
Attachment #264558 -
Flags: superreview?(jst)
Attachment #264558 -
Flags: superreview+
Attachment #264558 -
Flags: review?(jst)
Attachment #264558 -
Flags: review+
Comment 40•18 years ago
|
||
(In reply to comment #35)
> All the problems I ran into when I last touched this code stemmed from applying
> XBL bindings to nodes created via createElement() before they were inserted
> into the DOM. Since this patch doesn't do it, we should be ok. ;)
See bug 380990.
Comment 41•18 years ago
|
||
Comment on attachment 264558 [details] [diff] [review]
Patch to fix -w
>Index: content/base/public/nsINode.h
>+// Forces the XBL code to treat this node as if it was
s/was/were/
>+// in the document and therefor should get bindings attached.
"therefore"
>Index: content/xul/content/src/nsXULElement.cpp
>- // mControllers can own objects that are implemented
>- // in JavaScript (such as some implementations of
>- // nsIControllers.
I don't see where we're nulling out mControllers now. That would introduce leaks through the controllers, per above. Please restore this code, one way or another (e.g. we could make nsXULElement cycle-collect through mControllers and teach all nsIControllers implementations and everything they hold refs to about the cycle collector, or we could restore this nulling-out code in nsGenericElement::UnbindFromTree.
>Index: content/xul/content/src/nsXULElement.h
>+ // The function exists soly because XUL elements store the binding parent
s/soly/solely/
>+ // differently than nsGenericElement does.
"as a member instead of in the slots, as nsGenericElement does"
Rest looks good, modulo comments coming up in bug 380990.
Comment 42•18 years ago
|
||
We should write us some righteous regression tests (testing that the cloneNode thing attaches bindings and createElement does not, or something).
Flags: in-testsuite?
Assignee | ||
Comment 43•18 years ago
|
||
Attachment #265328 -
Flags: superreview?(bzbarsky)
Attachment #265328 -
Flags: review?(bzbarsky)
Comment 44•18 years ago
|
||
Comment on attachment 265328 [details] [diff] [review]
Fix bzs comments
Looks good.
Attachment #265328 -
Flags: superreview?(bzbarsky)
Attachment #265328 -
Flags: superreview+
Attachment #265328 -
Flags: review?(bzbarsky)
Attachment #265328 -
Flags: review+
Comment 45•18 years ago
|
||
Did this cause bug 380913? XBL anonymous nodes bound to cloned created elements don't seem to be getting rendered. Note that cloning an element that's in the document doesn't help and indeed additionally triggers an assertion.
Comment 46•18 years ago
|
||
Missed XUL ifdefs.
Attachment #265905 -
Flags: superreview?(bzbarsky)
Attachment #265905 -
Flags: review?(bzbarsky)
Updated•18 years ago
|
Attachment #265905 -
Flags: superreview?(bzbarsky)
Attachment #265905 -
Flags: superreview+
Attachment #265905 -
Flags: review?(bzbarsky)
Attachment #265905 -
Flags: review+
Updated•18 years ago
|
Target Milestone: Future → mozilla1.9alpha5
Assignee | ||
Comment 47•17 years ago
|
||
This was landed a while ago
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 48•17 years ago
|
||
Yep:
2007-05-15 18:13
2007-05-21 15:22
Comment 49•17 years ago
|
||
Comment on attachment 265905 [details] [diff] [review]
MOZ_XUL missed define
Near as I can puzzle out from bug 386854, this patch is obsoleted by the one there.
Attachment #265905 -
Attachment is obsolete: true
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in
before you can comment on or make changes to this bug.
Description
•