ScriptInterpreter cache for domobjects (dangerous change?)

[Maciej Stachowiak]

On Thursday, January 9, 2003, at 09:56  AM, David Faure wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> I'm looking at the safari changes in khtml/ecma.
>
>> - Added code to deal with DOM objects to make sure they keep the same
>> wrapper for the lifetime of the document and don't leak when the
>> document goes away.
>
> - -DOMNode::~DOMNode()
> - -{
> - -  ScriptInterpreter::forgetDOMObject(node.handle());
> - -}
>
> I understand that when destructing the whole document it's faster if 
> we can
> also clear the relevant cache all at once instead of node by node ....
> but what happens in the case where a single node (e.g. element) is
> being destroyed? Won't the removal of the above call mean that the 
> cache
> will hold a dangling pointer?

I can explain this change. DOMNode is a subclass of KJS::ObjectImp, 
which means it's a garbage-collected object. The ScriptInterpreter used 
to mark all objects it had cached, which means that the DOMNode could 
never be collected while the ScriptInterpreter had it cached. So 
effectively this code was a no-op, because the destructor could only be 
called once the ScriptInterpreter holding on to the node had already 
forgotten it. The result was that JavaScript DOM objects would be kept 
around until you destroy the ScriptInterpreter. In the case of Safari, 
this means until you close the window. I'm not sure if it has the same 
effect in Konqueror.

In any case, the new code makes sure the object wrappers are kept alive 
by the ScriptInterpreter until the document is destroyed. At that 
point, the nodes are forgotten and can be collected.

So I am confident this change, along with the per-document object 
caching, will not result in dangling pointers, and plugs an important 
sort-of leak. It's not a true leak, since there are still live pointers 
to the JS DOM objects, but they are not going to be used, and 
accumulate until you close the window.

Does that help?

Regards,

Maciej