429709 - nsNodeUtils::LastRelease needs to do less stuff in an XPCOMGC world

Status: RESOLVED WONTFIX

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

Description

[Benjamin Smedberg]

For XPCOMGC, we can't rely on object lifetime to do cross-object cleanup tasks. The big one I know about in content-land is nsGenericElement::Release calling nsNodeUtils::LastRelease, which ends up in UnbindFromTree.

I think we need to perform these teardown activities at the time a subtree is disconnected or when we navigate away from a document.

Comment 1

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

So I actually think that we're pretty close to simply being able to remove the UnbindFromTree call for all children from the nsGenericElement/nsDocument dtors. Here is what I think we need to do:

1. Make the parent-pointer a owning pointer.
   This will happen automatically with XPCOMGC, right?

Currently the main reason we have to call UnbindFromTree on the children is to deal with the case where a child stays alive past the death of an ancestor. For example nodes need to know that they are being removed from the document so they can reset their IsInDoc() flag, or update state wrt being removed from a form etc. However if the parent pointer is an owning pointer then we can never have the situation where an ancestor dies, but the child doesn't, so this becomes a non-issue.

2. Stop depending on UnbindFromTree saving form-state.
   This is more or less done already.

We currently call SaveState in UnbindFromTree. This used to be the main mechanism for saving form state for documents that were being killed. Basically the callchain was something like nsDocumentViewer::Close->nsDocument::Destroy->nsGenericElement::UnbindFromTree->nsGenericHTMLFormElement::SaveState
However with recent fixes to nsDocument::Destroy and nsDocumentViewer::Close this is no longer the main mechanism for saving state, instead we have a separate callchain explicitly to save state.
So I *think* we can simply remove the SaveState call from UnbindFromTree, but we need to test that that is the case.

3. Audit all UnbindFromTree implementations for similar evilness

We need to check that there is no other code in the UnbindFromTree implementations that isn't simply teardown-cleanup. The fact that we no longer call UnbindFromTree from nsDocument::Destroy and things still seems to be working fine seem to indicate that that is the case, but we should make sure.


I'm actually at this point more worried about the rest of the stuff that happens in nsNodeUtils::LastRelease. Basically that function does three things:

A. Call nsIMutationObserver::NodeWillBeDestroyed to let them know the node is
   going away.

Maybe we can simply remove this in a XPCOMGC world? Since this notification is mostly there to deal with nulling out weak pointers? Though there might be other things too there. Can we keep these notifications?

B. Remove all data associated with the node but living in various hashes.

Not sure how to handle this since the data does need to go away, but we're not allowed to access the document (where many of these hashes live) from the finalizer, right? Do we need to move these hashes out of the document and make them global hashes?

C. Delete the node (this is what ends up calling UnbindFromTree)

With the fixes mentioned above I think this is taken care of.

Comment 2

[Benjamin Smedberg]

> 1. Make the parent-pointer a owning pointer.
>    This will happen automatically with XPCOMGC, right?

Yes

> A. Call nsIMutationObserver::NodeWillBeDestroyed to let them know the node is
>    going away.

Hrm, interesting... what do the observers typically do with this notification? See below about how hash state is maintained.

> other things too there. Can we keep these notifications?

No, we certainly cannot.

> B. Remove all data associated with the node but living in various hashes.

Don't worry about this: we've developed a small art of hash maintenance already. Basically the document will register itself as a GC "presweep" callback. When that callback is fired it can ask "is object X which I have in my hashtable marked?" and if it's not marked remove that entry from the hashtable.

Comment 3

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

(morphing title of the bug as more things than UnbindFromTree needs to be fixed here)

> > A. Call nsIMutationObserver::NodeWillBeDestroyed to let them know the node 
> >    in going away.
> 
> Hrm, interesting... what do the observers typically do with this notification?
> See below about how hash state is maintained.

*Typically* objects null out weak pointers. However it's possible that more things are happening too. Did an lxr search and immediately found this which does a lot more it looks like:

http://lxr.mozilla.org/mozilla/source/extensions/metrics/src/nsLoadCollector.cpp#527

> > other things too there. Can we keep these notifications?
> 
> No, we certainly cannot.

There is no pre-sweep step we can perform? I seem to recall you mentioning something like that before.

> > B. Remove all data associated with the node but living in various hashes.
> 
> Don't worry about this: we've developed a small art of hash maintenance
> already. Basically the document will register itself as a GC "presweep"
> callback. When that callback is fired it can ask "is object X which I have in
> my hashtable marked?" and if it's not marked remove that entry from the
> hashtable.

So all documents would have to registered for this for their full lifetime and look up everything they keep in their hashes at every GC? Sounds pretty expensive :(

If we did make these hashes global we could simply remove call the hash and remove the object in the finalizer, right?

Comment 4

[Benjamin Smedberg]

> *Typically* objects null out weak pointers. However it's possible that more

Yeah, assuming that the number of observers is not huge, these callers should be switched to use a presweep hook.

> There is no pre-sweep step we can perform? I seem to recall you mentioning
> something like that before.

There is a presweep hook... the observers have to do it though.

> So all documents would have to registered for this for their full lifetime and
> look up everything they keep in their hashes at every GC? Sounds pretty
> expensive :(

It's not that expensive, really.

> If we did make these hashes global we could simply remove call the hash and
> remove the object in the finalizer, right?

You certainly *could* that that... whether it's worthwhile depends on the hash, though... I tend to think it's not worthwhile unless we discover a performance issue.

Comment 5

[Sergey Yanovich]

I've pasted a frame of an exact incremental GC in bug 419427. The frame stores data in a double-linked digital tree. It is very fast, but has considerable memory overhead.

It really looks like DOM nodes are stored in a similar structure. As a result, if DOM nodes are managed (enumerating children/parents) with the GC internal tree, there will be no additional memory overhead for this class of objects in the proposed GC.

Comment 6

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 7

[Andrew McCreight [:mccr8]]

XPCOMGC is dead. Long live the cycle collector!