Closed Bug 716981 Opened 13 years ago Closed 11 years ago

Finalize everything during JSRuntime shutdown

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: sfink, Unassigned)

Details

Currently during JSRuntime shutdown, we don't finalize all of the finalizable GC-things. This leaks memory, since eg JSScripts don't get a chance to free various bits of data. I think if you manually unroot everything and destroy all your contexts first, you're probably ok, but for both embedders and web workers it sounds like automatic cleanup would be nicer.

This could potentially slow down process shutdown, so we might want to have a flag saying to skip the finalization, but I don't know whether it's actually needed yet so it'd probably be better for that to be done as a separate bug if someone wants it. (It also has a complexity cost, in that anything externally watching would need a separate path to be notified that an entire runtime is going down.)
So you mean just ignore the mark phase and go directly do finalizing?
(In reply to Gregor Wagner [:gwagner] from comment #1)
> So you mean just ignore the mark phase and go directly do finalizing?

I don't really know the control flow well enough to know if there are other wrinkles, but that makes sense to me.

Note that this came up because there was a bug 717104 in Debugger that could hold things alive, so even explicitly destroying the contexts was not enough to avoid the problem. But in further conversation, it still seems like people would prefer full finalization.

An example of where embedders care is when they're maintaining external state associated with eg JSScripts, and manage the lifetime of that state via the script creation and destruction hooks. But if something is rooted (explicitly, or perhaps because you didn't destroy all contexts), it won't get the destruction hook because its finalizer does not get run. In fact, it's fairly easy to get corruption when a subsequently-created runtime creates a new script at the same memory location as a previous runtime's script. (Not to mention that the scripts leak a bunch of memory.) Apparently it's tricky to ensure that everything is properly unrooted, especially in conjunction with the conservative stack scanner.

This is a separate bug, but I wonder if JS_DestroyRuntime should either destroy all remaining contexts or assert that there aren't any.
From the implementation point this should be a small patch. 
We already ignore all stack values during marking if we perform the last GC. 
We can extend this to the whole runtime and just don't call MarkRuntime when rt->state == JSRTS_LANDING.
Any update on this?
I think the direction we've been moving in is to just forgo GCs at shutdown as much as possible. We already don't do CCs, which triggered the bulk of shutdown GCs.  See bug 662444.
It seems like having an option to finalize or not would be good, especially for non-gecko spidermonkey.
The problem I'm running to (in an embedded JSAPI) is that the global object is not finalized if there are remaining rooted object attached to its hierarchy.
It's something like a "chicken or the egg" problem :

1. a non rooted object live in JS land
2. I have a finalizer callback on that object that destroy some of my C++ objects
3. I have to unroot JSObject in my C++ destructor
4. 1 is not called because of remaining rooted object in 3.

To fix this I have to keep track on every rooted object and manually unroot them before destroying the context.

This kind of break the "cascasding destroy object" model.

I don't know if this is possible, but a good option would be to call the finalizer callback even on rooted object during a shutdown.
How are we supposed to deal with this :

http://pastebin.mozilla.org/2078974

I see no solution apart from keeping track on every rooted object.
In the exemple above, createPattern finalizer is supposed to unroot "i". BTW it's not called because "i" is rooted. Also, NO finalizer got called if there are remaining rooted object (like the File object in the exemple). However File is finalized if it's create in a function (not on the global).

Looks like a bug to me.
Or am I thinking wrong?
The example from comment 8:

-------
var i = new Image("foo.png");
var pattern = createPattern(i, "repeat"); /* if the native behind createPattern roots "i" object,
                                            its finalizer is not called when destroying the context nor any finalizer */

var f = new File("foo.txt"); /* Finalizer not called if createPattern roots i */

-------

The problem is that there's a cycle from pattern -> i -> global -> pattern, and i is externally rooted (via JS_AddObjectRoot). So nothing can be collected.

The workaround is to null out global[pattern], breaking the cycle. The easiest way to do that is with JS_SetAllNonReservedSlotsToUndefined(global object).

The problem with finalizing everything regardless of roots is that there's no safe finalization ordering.

I suppose we could add APIs for enumerating all of the explicit roots.
You're right about ordering. That makes this bug invalid.

A good solution would be to imply "JS_SetAllNonReservedSlotsToUndefined" on global object during a JS_DestroyRuntime?

I still have a concern :

Take the exemple above :

* createPattern internally create a C++ object and stores it as private. It also roots its first parameter |i|. It does this because it rely on the |i|'s private object.
* When |pattern| is no longer reachable, it unroot |i| using |pattern|'s finalizer => C++ Destructor. Now |i| can be either collected or still in another root or reachable on the JS.

What if this is done during the shutdown? I mean, |i| is no longer reachable nor root'd, but the JS_RemoveObjectRoot was called during the shutdown GC. How can we be sure that the current GC's run will collect and Finalize something that was unrooted during the same run? Do we have to run the GC again and again until everything is collected?

I see no good solution for such a simple case.
The good solution is to use a trace hook (or just store the thing you depend on in a reserved slot) instead of manual rooting, no?
Alright, I didn't know JS_SetReservedSlot is visible to the GC.
A reserved slot is the best. I'd rather people not implement trace hooks unless absolutely necessary since they're pretty error-prone in combination with incremental GC.
Bill, what should we take care of, related to incremental GC?

I have something like this http://pastebin.mozilla.org/2083000 to root a tree related to my internal structure. Is there something "new" that can happen with the Incremental GC?
Yes, incremental GC requires write barriers. Whenever you remove anything from your tree, you need to invoke a write barrier on the thing that was removed (via js::IncrementalReferenceBarrier).

However, I would suggest running with incremental GC disabled. I think it is off by default anyway.
I use incremental GC. Is JSCLASS_IMPLEMENTS_BARRIERS related to this?
That's just a flag you can set to say that you know what you are doing, it doesn't actually indicate that your trace hooks are implemented correctly.
I mean, is this flag required?
The flag is required to use incremental GC. I still would recommend against doing this unless you really, really need incremental GC.
I need it. I'm working on a js-based game engine on top of JSAPI. GC pauses are not an option.

Do you recommend not to use it because it's error-prone? Or because it's still experimental and generational GC is arriving?

To sum up what I've done :

* I use JSTraceOp to trace object from my tree
* Whenever an object is removed form the tree I call something like
>     if (js::IsIncrementalBarrierNeeded(this->jscx)) {
>        js::IncrementalReferenceBarrier(this->jsobj);
>    }
* There is a possibility that this object gets attached to another tree right after. Is that a problem that IncrementalReferenceBarrier was called even if this object become reachable from another place?

I agree -- Using incremental GC seems to be a pain in the ass.

Sorry for having took over this "thread". We should continue this discution on the mailing/irc.
That looks right. You also need to do the write barrier whenever you change something. So, for example, if you did |handler->jsobj = <something different>|, you should run the write barrier on the old value, before the assignment.

I'm going to close this as WONTFIX since it seems like a bad idea to run finalizers in such a weird order.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.