Bugzilla

(In reply to Jon Coppeard (:jonco) from comment #2)
> (In reply to Steve Fink [:sfink] [:s:] from comment #1)
> > it looks like the invisible machinery is the JS value stack
> 
> It would be great to confirm whether this is the problem.  But if it is, why are we marking unused stack slots?  Maybe there is a good reason, I just don't know what it is.

That's a fair point, but I think it makes sense what it's doing assuming gc() is the next thing called.

Immediately after clearKeptObjects(), I'd guess this looks exactly like clearKeptObjects() returned the target. If there's a gc() call after that, then it may be that the state is indistinguishable from gc() returning the target object, up until the point where the code sets gc()'s rval. And if you set an rval then GC within a native function before returning, I would certainly expect that rval to get marked. So it kind of makes sense that the GC is considering that part of the stack to be live, and also suggests that we might want to consider clearing out the rval in GC() before doing the collection to prevent the leftover Value in the rval from being marked unnecessarily.

I think I'll simplify my opinion: there are 2 bugs here, and once enhancement for another bug. One is that `new WeakMap(target)` should be adding `target` to `KeptObjects`. The other is that clearKeptObjects() should be doings `args.rval().setUndefined()`. The enhancement is to clear out the rval earlier in GC(): filed bug 1603851.

(In reply to Jon Coppeard (:jonco) from comment #2)
> (In reply to Steve Fink [:sfink] [:s:] from comment #1)
> > it looks like the invisible machinery is the JS value stack
> 
> It would be great to confirm whether this is the problem.  But if it is, why are we marking unused stack slots?  Maybe there is a good reason, I just don't know what it is.

That's a fair point, but I think it makes sense what it's doing assuming gc() is the next thing called.

Immediately after clearKeptObjects(), I'd guess this looks exactly like clearKeptObjects() returned the target. If there's a gc() call after that, then it may be that the state is indistinguishable from gc() returning the target object, up until the point where the code sets gc()'s rval. And if you set an rval then GC within a native function before returning, I would certainly expect that rval to get marked. So it kind of makes sense that the GC is considering that part of the stack to be live, and also suggests that we might want to consider clearing out the rval in GC() before doing the collection to prevent the leftover Value in the rval from being marked unnecessarily.

I think I'll simplify my opinion: there are 2 bugs here, and one enhancement for another bug. One is that `new WeakMap(target)` should be adding `target` to `KeptObjects`. The other is that clearKeptObjects() should be doings `args.rval().setUndefined()`. The enhancement is to clear out the rval earlier in GC(): filed bug 1603851.

(In reply to Jon Coppeard (:jonco) from comment #2)
> (In reply to Steve Fink [:sfink] [:s:] from comment #1)
> > it looks like the invisible machinery is the JS value stack
> 
> It would be great to confirm whether this is the problem.  But if it is, why are we marking unused stack slots?  Maybe there is a good reason, I just don't know what it is.

That's a fair point, but I think it makes sense what it's doing assuming gc() is the next thing called.

Immediately after clearKeptObjects(), I'd guess this looks exactly like clearKeptObjects() returned the target. If there's a gc() call after that, then it may be that the state is indistinguishable from gc() returning the target object, up until the point where the code sets gc()'s rval. And if you set an rval then GC within a native function before returning, I would certainly expect that rval to get marked. So it kind of makes sense that the GC is considering that part of the stack to be live, and also suggests that we might want to consider clearing out the rval in GC() before doing the collection to prevent the leftover Value in the rval from being marked unnecessarily.

I think I'll simplify my opinion: there are 2 bugs here, and one enhancement for another bug.
* Bug one: `new WeakMap(target)` should be adding `target` to `KeptObjects`
* Bug two: clearKeptObjects() should be doing `args.rval().setUndefined()`
* Enhancement: clear out the rval earlier in GC(): filed bug 1603851.