53901 - nsXULElement::CloneNode sets IS_IN_DOCUMENT flag

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack trace showing addition of leaked root for bug 51177

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Above stack trace was *with* the patch to fix bug 52726.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Chris Waterson]

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

[Chris Waterson]

Attachment: nsXULElement::Create(nsXULPrototypeElement*, ...) shouldn't require aDocument != nsnull

Comment 6

[Chris Waterson]

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().

Comment 7

[Chris Waterson]

Hrmph. That patch ain't ready for primetime. We're now dropping JS objects.

Comment 8

[Brendan Eich [:brendan]]

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

[Chris Waterson]

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

[Chris Waterson]

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

[Chris Waterson]

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

[Chris Waterson]

(from mail to [e-mail]...)

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

[Michael La Guardia]

Marking "needinfo." Will reconsider for inclusion once there is a reviewed and
super reviewed patch.

Comment 14

[David Hyatt]

Don't screw XBL!  What did it ever do to you? :)

Comment 15

[Chris Waterson]

Denominating this bug because of the mess it entails. We'll fix this later. In
the meantime, be careful with Clone'd elements!

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 17

[Chris Waterson]

I suspect that bug 71483 will make fixing this bug irrelevant. I'll re-evaluate 
once the DOM is XPConnected.

Comment 18

[David Hyatt]

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

[David Hyatt]

We need to fix it so that nodes never have a null mDocument when created using
createElement or cloneNode.

Comment 20

[Chris Waterson]

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.

Comment 21

[David Hyatt]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See also bug 52732.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

[Peter Van der Beken [:peterv]]

Just a small note: my patch in bug 27382 (attachment 146411 [details] [diff] [review]) adds InDocument()
to nsGenericElement.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Right.  That's what got me thinking about the api nsIContent should expose...
note that GetOwnerDocument (returning nsIDocument) is also already on
nsGenericElement.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> 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

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for once the deps are fixed (may also need to change ::Create a bit)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Once this is fixed, we should back out the code in nsGenericElement::doReplaceOrInsertBefore that this bug necessitates.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Working on this

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

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.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix -w

Same as above, diffed with -w

Comment 32

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

also, do you know the answer to the question in nsGenericElement::UnbindFromTree in the patch?

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Oh, why check the namespace ID in UnbindFromTree instead of using the FromContent() returning nsXULElement or just IsContentOfType directly?

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, and I'll use the FromContent trick too.

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 264558 [details] [diff] [review]
Patch to fix -w

Looks good to me, r+sr=jst

Comment 40

[Asaf Romano (gone)]

(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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

We should write us some righteous regression tests (testing that the cloneNode thing attaches bindings and createElement does not, or something).

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix bzs comments

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265328 [details] [diff] [review]
Fix bzs comments

Looks good.

Comment 45

[[e-mail]]

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

[Oleg Romashin (:romaxa)]

Attachment: MOZ_XUL missed define

Missed XUL ifdefs.

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This was landed a while ago

Comment 48

[Jesse Ruderman]

Yep:

2007-05-15 18:13
2007-05-21 15:22

Comment 49

[Phil Ringnalda (:philor)]

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.