72465 - JavaScript: JS_DestroyRuntime() does not call finalizer for rooted objects

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Klaus Ziegler]

As far as I understood, the above function (in Release candidate 1-3) does not 
call the finializer for rooted objects, thus opening a door for leaks if these 
are custom objects with for example allocated private memory, ipc objects, ...

The current coding seems to address this issue already by a printf (:=

Kind Regards

Klaus

Comment 1

[Phil Schwartau]

cc'ing Brendan, Patrick, jband - 

Comment 2

[Brendan Eich [:brendan]]

If you fail to remove a root before destroying all contexts and calling
JS_DestroyRuntime (and possibly then calling JS_ShutDown), your code is the
guilty party, and it needs to be fixed.

The roots hash table and all of its entries are correctly freed by the JS engine
in js_FinishGC.  But by the time you have called JS_DestroyRuntime, there are no
more JSContexts on which to run yet another GC, in order to collect anything
that might have been retained due to your leaked roots.

The GC's heap will also be freed no matter what.  The only leaks therefore are
private data held by your JSObjects.  But again, we can't finalize those once
there are no more JSContexts.  Creating another JSContext just to do GC might
work, but it seemed better to us to flag the leaks in your code, and not try to
paper over those leaks.

I'm marking this bug WONTFIX, but I acknowledge it's controversial.

/be

Comment 3

[John Bandhauer]

I'm with brendan. I'd consider it a bug if the engine *ever* called the 
finalizers for my rooted objects.

Comment 4

[Phil Schwartau]

Marking Verified. Klaus, thank you for this report - 

Comment 5

[Klaus Ziegler]

Hello John, Hello Phil,

I agree that there was some kind of bug within in our coding, but I also think 
the API needs some kind of improovement to better handle issues like ours.

First let me describe our incident. We have a bigger project with js-script 
where several groups are somehow independenly adding stuff to the js-engine, so 
that there is only a weak knowledge, which contexts are used and which objects 
are rooted. One collegue has rooted a script with the intention to protect it 
from GC as long the context exists ( the documentation of JS_AddRoot() is 
missing a note that it ties the object against the runtime ). By sorrow this 
excluded the whole context from the GC cleanup causing massive memory leaks.

So I would like to propose the API shall be extended somehow to keep an eye on 
forgotten or ancient roots preventing a proper cleanup. First of all I think 
DestroyRuntime() shall return an information that there were possible memory 
leaks due rooted objects or even better return an error condition. Additionaly 
an iterator like JS_DumpNamedRoots() for all rooted objects would be fine, so 
one can buildin a cleanup for any existing root before starting GC and 
DestroyRuntime().

Kind Regards
Klaus

Comment 6

[Brendan Eich [:brendan]]

Klaus: I will make you a deal: I won't change JS_DestroyRuntime (API
compatibility); I will add a JS_DumpGCRoots API you can use to deal with any
leftover roots just before you destroy the last context and then its runtime. 
Patch coming up.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed API extension: JS_DumpGCRoots

Comment 8

[Klaus Ziegler]

Hello Brendan, thanks for your efforts, your offer is higly welcomed.
Faithfully Yours
Klaus

Comment 9

[Brendan Eich [:brendan]]

Attachment: better patch: JS_MapGCRoots, which hides jshash.h implementation

Comment 10

[Brendan Eich [:brendan]]

Thinking a little harder, in the light of day, I do not want to couple this API
to its implementation (jshash.h, currently).  So now the function is called
JS_MapGCRoots(rt, map, data), with the map function having this signature:

intN (*)(void *rp, const char *name, void *data)

The return codes and flags are JS_MAP_GCROOT_NEXT, JS_MAP_GCROOT_STOP, and
JS_MAP_GCROOT_REMOVE.  These happen to match the jshash.h HT_ENUMERATE_* flags,
but need not.

jband, shaver, rogerl: please r=/sr= and we'll get this in.

/be

Comment 11

[Mike Shaver (:shaver emeritus)]

r=shaver.  Should we use that to unroot-and-finalize at shutdown in Mozilla, as
well?

Comment 12

[John Bandhauer]

I repeat my assertion that I would consider it a bug if a finalizer gets 
called for an object that my code has rooted. If these folks want to have their 
components stomp on each other in their embedding, that's fine. But my component 
will not be happy.

Comment 13

[Brendan Eich [:brendan]]

shaver: jband's right, we certainly should not do anything at shutdown that
requires a context and some thought on the part of the embedder about what
native data structures leaked, that also may have leaked their rooted members.

My offer stands: just JS_MapGCRoots, no more, no less.  Embedders who need to
save themselves from their own leaks can use it, and a "default context", to do
a final unroot and GC for themselves.  This should not be done by the engine by
default.

/be

Comment 14

[John Bandhauer]

brendan: Shouldn't you typedef the callback sooner rather than later so that 
CRT_CALL can get into the game? Are there additional limitations on callers' 
interaction with the jsapi because this is run in the gc lock?

Comment 15

[Brendan Eich [:brendan]]

Attachment: best patch: comment fixes/improvements only

Comment 16

[Brendan Eich [:brendan]]

Oops, missed jband's comments.  Yes, I keep forgetting about OS2.

There is no alternative, short of atomically copying the roots hash table, to
holding the GC lock across the enumeration.  I can add a comment saying "don't
call JS_GC, JS_BeginRequest, etc." but that seems obvious.  If I have to write
such a comment, I predict whoever might need to read it, won't read it.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: jband's help makes the best patch yet!

Comment 18

[John Bandhauer]

r/sr=jband Looks good to me.

Comment 19

[Brendan Eich [:brendan]]

Fix is in.

/be

Comment 20

[Phil Schwartau]

Marking Verified -