118194 - Stale documentOwner pointers are left in the tree

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Ray Whitmer]

I am taking Johnny's word on this one.  Create an XML document (not the main
document) and obtain a reference to a leaf node.  Now, drop the reference to the
document (ffter the garbage collector kicks in if you are in JS), and the
references to the ownerDocument become stale because the document has been
destroyed whereas individual nodes have not.  Having Javascript have access to
stale pointers makes it not safe or secure, because it is probably possible to
write new contents into objects and even the vtable pointer since they are in
the available memory pool and then use the object.  Whenever a document is
destroyed, it must explicitly zero the ownerDocument of all nodes it has ever
created that are still being referenced (not just those currently in the
hierarchy, because the script could be holding on to an node that was removed
from the hierarchy).  This affects SOAP because the user gets into these
situations with documents that are not attached to a document window, such as
those used by SOAP.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Clear the document pointer in all children on document destruction when needed.

Comment 2

[Johnny Stenback (:jst)]

This is hard to trigger, and we don't have a testcase, but I'm sure it's a crash
waiting to happen. The attached patch should make sure this crash can never happen.

Comment 3

[Sivakiran Tummala]

ray can you please provide the testcase.

Comment 4

[Johnny Stenback (:jst)]

I don't think there is a testcase for this, if we really need a testcase we
could probably create one, but I don't think that's needed for this bug. We can
verify this on the code level.

Comment 5

[Ray Whitmer]

As I said before (unless I missed something) it is not enough to clear the
documentOwner pointer on descendants, because the script may easily still have a
pointer to an element that has been removed from underneath the hierarchy.  All
nodes which are still externally referenced must be cleared whether or not they
are in the hierarchy.

Comment 6

[Johnny Stenback (:jst)]

Nodes that are not part of the document will not have a direct pointer back to
the document. The document will be reachable from *elements* that are not part
of the document as long as the document is alive, but that weak reference
(through nsINodeInfo) is already properly cleared un document destruction, the
last few lines in ~nsDocument() takes care of that:

  if (mNodeInfoManager) {
    mNodeInfoManager->DropDocumentReference();
  }

As for non-element nodes that are not part of a document, they don't have a
reference back to the document any more. That is of course a conformance bug
(which is filed already).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 63540 [details] [diff] [review]
Clear the document pointer in all children on document destruction when needed.

r=bzbarsky if you change the aCompileEventHandlers arg to PR_FALSE

Comment 8

[vidur (gone)]

Comment on attachment 63540 [details] [diff] [review]
Clear the document pointer in all children on document destruction when needed.

sr=vidur with bzbarsky's required change.

Comment 9

[Johnny Stenback (:jst)]

Fixed.

Comment 10

[Sivakiran Tummala]

verified fixed. code checked in