Fix issues with v8 contexts in vm module

[laverdet]

The EvalMachine machine's memory management is pretty much a mess. This cleans that up a little bit. It also unreverts the revert of my top-level locker.

[bnoordhuis]

/cc @piscisaureus

[isaacs]

The vm module IS clearly problematic. However, 648f536 strikes me as overly magical.

If we're going to fix vm, we ought to fix it all the way, and make vm.runIn*Context behave as expected.

[laverdet]

I know it kind of seems that way, but I'm not trying to be magical for magic's sake. There's not really that many clean ways to do this. The issue is that there are just a lot of different possibilities of contexts being allocated, contexts being entered, and it's all compounded by the function returning in several places. It's best to leave the job of exiting and disposing to the compiler. The whole module could probably be rewritten in a way that would make this simpler, but without significant refactoring, a helper class and auto_ptr are pretty simple to pull off.

C++ doesn't have any kind of finally like in JS, so this kind of thing is just accepted practice. The idea is that the class destructors are supposed to handle cleaning up for you, but since v8 doesn't provide any way to scope a persistent handle you're left cleaning up some mess.

You could also copy and paste that "clean up" block 3 or 4 more times if you're more comfortable with that. That would also do the trick.

Anyway it's not really my call. I'm only up in this because my top-level Locker addition was blamed which isn't what's causing the crash. The assertion that fails (only in debug mode, and only on exit) is that the number of entered contexts is not 0 at the time that the locker is destroyed. Removing the top-level Locker is only going to lead to quieter failures which may have material harm to performance.

[isaacs]

Let's unrevert the Locker change. You make a good point. We should just let the error happen, and use it as a blocker from declaring 0.8 stable. However, I'd like to explore a better solution for the vm module.

[TooTallNate]

Why can't we just use MakeWeak() and a callback function on the contexts to know when to clean them up?

[laverdet]

If you make the handle weak, v8 may try to garbage collect the entered context and all hell breaks loose.

[bnoordhuis]

Use of the STL is proscribed in Node.

[laverdet]

Use of the STL is proscribed in Node.

But.. C++, it's part of the standard. Anyway, not my call.

Do you think we could get the Locker change reverted though? Are we in agreement that it is a good call?

[isaacs]

Having given this a bit more thought, it seems like the issues with the vm module are not within reach for solving in v0.8, and the Locker seems to do more good than harm. The auto_ptr approach is not a good idea, though.

We ought to put a XXX comment around it with a link to #3042, for the next person who wonders why the vm tests throw assertion failures in debug mode.

[laverdet]

The feasibility of the safety issues you brought in #3042 is definitely suspect. Secure JS sandboxes are very difficult to implement correctly, I would know (I created the JS sandbox for Facebook Platform). Since Node is only running under v8, and has access to bleeding edge JS features it should be significantly easier than implementing something similar for, say Internet Explorer. I think it's possible but not unless it's given significant priority from Joyent and the community. I'm confident it can be done without separate processes as well.

Regarding vm's memory issues, though I think it's totally reasonable to fix them by 0.8. If auto_ptr is unacceptable (I admit, my use of auto_ptr<Context::Scope> is kind of magic) there's a couple of other solutions we could consider:

1. Copy and paste! This function has lots of copy and paste "clean up" logic. We could fix the issues by making sure the unhanded branches have the cleanup logic. This obviously is not safe in the face of C++ exceptions (though neither is much else of node), and presents long-term maintainability issues. On the other hand, it's no worse than what's already there.

2. Create a version of v8::Context::Scope which can accept scopes dynamically instead of statically. Additionally it would be valuable to create a version of the Persistent<T> class which will automatically call Dispose() at the end of scope. This class could be used in at least a dozen places in node to get rid of the Dispose() calls everywhere and make v8 handle management more robust in the face of changes to core code. Honestly I'm surprised this class doesn't already exist in v8, I will ping v8-users about that.

Anyway I'm available to implement either solution if the node team wants to provide direction.

[bnoordhuis]

Regarding vm's memory issues, though I think it's totally reasonable to fix them by 0.8. If auto_ptr is unacceptable (I admit, my use of auto_ptrContext::Scope is kind of magic)

I can live with the general solution but I don't want to depend on std::auto_ptr. We aim for maximum portability and besides, I don't like debugging issues where people have a broken STL implementation somewhere on the include path. Replace it with a small hand-rolled class and I'll merge the PR.

[laverdet]

Followed up with a better solution in #3090