Stale documentOwner pointers are left in the tree

VERIFIED FIXED in mozilla0.9.9

Status

()

Core
DOM: Core & HTML
P2
major
VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: Ray Whitmer, Assigned: jst)

Tracking

Trunk
mozilla0.9.9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX])

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
Created attachment 63540 [details] [diff] [review]
Clear the document pointer in all children on document destruction when needed.
(Assignee)

Comment 2

17 years ago
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.
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P2
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla0.9.8

Comment 3

17 years ago
ray can you please provide the testcase.
(Assignee)

Comment 4

17 years ago
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.
(Reporter)

Comment 5

17 years ago
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.
(Assignee)

Comment 6

17 years ago
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).
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9
(Assignee)

Updated

17 years ago
Keywords: nsbeta1
Keywords: nsbeta1 → nsbeta1+
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
Attachment #63540 - Flags: review+

Comment 8

17 years ago
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.
Attachment #63540 - Flags: superreview+
(Assignee)

Comment 9

17 years ago
Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 10

16 years ago
verified fixed. code checked in
Status: RESOLVED → VERIFIED

Updated

10 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.