52726 - XUL roots script objects for elements not in documents

Status: VERIFIED INVALID

(Core :: XUL, defect, P2)

Description

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

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.

Comment 1

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

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.

Comment 2

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

*** Bug 52723 has been marked as a duplicate of this bug. ***

Comment 3

[Chris Waterson]

Yes, that sounds right. Do you want me to fix it? Or can you do the honors...

Comment 4

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

Could you?  I don't have a computer right now, and sshing to home doesn't really
allow much testing...

Comment 5

[Chris Waterson]

no problem.

Comment 6

[Chris Waterson]

Attachment: proposed fix

Comment 7

[Chris Waterson]

hyatt, jst, dbaron: love me up with some r=?

Comment 8

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

Hmmm.  I tried this patch without the fix to bug 51177, and I still see the mail
compose leak with a treechildren root.

Comment 9

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

Attachment: simple XUL testcase where above patch does fix a leak

Comment 10

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

There are two separate problems.  I filed bug 53901 on the other one.

r=dbaron on the patch above.

Comment 11

[Chris Waterson]

Huge potential for leakage. Marking rtm+.

Comment 12

[Michael La Guardia]

Marking rtm++.  Please check this in as soon as possible.

Comment 13

[Chris Waterson]

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.

Comment 14

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

This looks like something we want for mozilla 1.0, even if we can't have it in
Netscape's RTM.

Comment 15

[Chris Waterson]

*** Bug 40480 has been marked as a duplicate of this bug. ***

Comment 16

[Chris Waterson]

I suspect that fixing bug 71483 will make this bug irrelevant; once that bug is 
fixed, I'll re-evaluate.

Comment 17

[laurel]

Uh, the bug you mentioned is assignee == [e-mail].
That translates to me as we won't get to it this release, eh?

Comment 18

[John Morrison]

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 [e-mail]).

Comment 19

[laurel]

Sorry, I misread the fields.  Just trying to make sure it gets in for 6.5. 

Comment 20

[Koike Kazuhiko]

*** Bug 78927 has been marked as a duplicate of this bug. ***

Comment 21

[laurel]

*** Bug 82267 has been marked as a duplicate of this bug. ***

Comment 22

[Chris Waterson]

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

[scottputterman]

Because a couple of months ago you marked 40480 as a dup of this bug.  Is this
no longer true?

Comment 24

[Chris Waterson]

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.

Comment 25

[Chris Waterson]

Attachment: set mDocument in nsXULDocument::CreateElement()

Comment 26

[scottputterman]

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

[Fabian Guisset]

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.

Comment 28

[Chris Waterson]

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?

Comment 29

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

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 30

[Jacek Piskozub]

10 dups (including dups^2). Marking mostfreq.

Comment 31

[Johnny Stenback (:jst)]

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?

Comment 32

[Chris Waterson]

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.

Comment 33

[Chad Austin]

I'll verify this one...  Trusting Waterson...  ;)