Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix issues with v8 contexts in vm module #3039

Closed
wants to merge 2 commits into from

4 participants

@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.

laverdet added some commits
@laverdet laverdet Revert "Revert "Re-add top-level v8::Locker""
Locker does *NOT* cause a crash. The error is in Node, Locker simply
makes this existing errors louder, and only in debug mode. Fix for the
specific issue coming in after this.

This reverts commit 4071815.
aac31bc
@laverdet laverdet Fix issue with unexited contexts in vm module
This EvalMachine function is an RIAA nightmare and doesn't enter and
exit contexts correctly. I used an auto_ptr with a Context::Scope to
handle the dynamic nature of EvalMachine's behavior and created a class
which will dispose a persitent handle at the end of scope. This ensures
100% that the context is exited if needed, and disposed if needed.
648f536
@isaacs
Owner

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
Owner

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.

@isaacs isaacs referenced this pull request
Closed

vm module correction #3042

@TooTallNate
Owner

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 bnoordhuis commented on the diff
src/node_script.cc
((6 lines not shown))
}
+ // context_scope will enter and exit the Context automatically
+ std::auto_ptr<Context::Scope> context_scope;
@bnoordhuis Owner

Use of the STL is proscribed in Node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@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
Owner

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
Owner

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

@laverdet laverdet closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 30, 2012
  1. @laverdet

    Revert "Revert "Re-add top-level v8::Locker""

    laverdet authored
    Locker does *NOT* cause a crash. The error is in Node, Locker simply
    makes this existing errors louder, and only in debug mode. Fix for the
    specific issue coming in after this.
    
    This reverts commit 4071815.
  2. @laverdet

    Fix issue with unexited contexts in vm module

    laverdet authored
    This EvalMachine function is an RIAA nightmare and doesn't enter and
    exit contexts correctly. I used an auto_ptr with a Context::Scope to
    handle the dynamic nature of EvalMachine's behavior and created a class
    which will dispose a persitent handle at the end of scope. This ensures
    100% that the context is exited if needed, and disposed if needed.
This page is out of date. Refresh to see the latest.
Showing with 51 additions and 42 deletions.
  1. +30 −24 src/node.cc
  2. +21 −18 src/node_script.cc
View
54 src/node.cc
@@ -2759,33 +2759,39 @@ int Start(int argc, char *argv[]) {
// Use copy here as to not modify the original argv:
Init(argc, argv_copy);
- v8::V8::Initialize();
- v8::HandleScope handle_scope;
-
- // Create the one and only Context.
- Persistent<v8::Context> context = v8::Context::New();
- v8::Context::Scope context_scope(context);
-
- // Use original argv, as we're just copying values out of it.
- Handle<Object> process_l = SetupProcessObject(argc, argv);
- v8_typed_array::AttachBindings(context->Global());
-
- // Create all the objects, load modules, do everything.
- // so your next reading stop should be node::Load()!
- Load(process_l);
-
- // All our arguments are loaded. We've evaluated all of the scripts. We
- // might even have created TCP servers. Now we enter the main eventloop. If
- // there are no watchers on the loop (except for the ones that were
- // uv_unref'd) then this function exits. As long as there are active
- // watchers, it blocks.
- uv_run(uv_default_loop());
-
- EmitExit(process_l);
+ V8::Initialize();
+ Persistent<Context> context;
+ {
+ Locker locker;
+ HandleScope handle_scope;
+
+ // Create the one and only Context.
+ Persistent<Context> context = Context::New();
+ Context::Scope context_scope(context);
+
+ // Use original argv, as we're just copying values out of it.
+ Handle<Object> process_l = SetupProcessObject(argc, argv);
+ v8_typed_array::AttachBindings(context->Global());
+
+ // Create all the objects, load modules, do everything.
+ // so your next reading stop should be node::Load()!
+ Load(process_l);
+
+ // All our arguments are loaded. We've evaluated all of the scripts. We
+ // might even have created TCP servers. Now we enter the main eventloop. If
+ // there are no watchers on the loop (except for the ones that were
+ // uv_unref'd) then this function exits. As long as there are active
+ // watchers, it blocks.
+ uv_run(uv_default_loop());
+
+ EmitExit(process_l);
+#ifndef NDEBUG
+ context.Dispose();
+#endif
+ }
#ifndef NDEBUG
// Clean up.
- context.Dispose();
V8::Dispose();
#endif // NDEBUG
View
39 src/node_script.cc
@@ -22,6 +22,7 @@
#include "node.h"
#include "node_script.h"
#include <assert.h>
+#include <memory>
namespace node {
@@ -98,6 +99,18 @@ class WrappedScript : ObjectWrap {
Persistent<Script> script_;
};
+template <class T> class PersistentScope {
+ public:
+ explicit inline PersistentScope(Persistent<T> handle) : handle_(handle) {}
+ inline ~PersistentScope() {
+ if (!handle_.IsEmpty()) {
+ handle_.Dispose();
+ }
+ }
+ private:
+ Persistent<T> handle_;
+};
+
Persistent<Function> cloneObjectMethod;
@@ -358,13 +371,18 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
} else if (context_flag == userContext) {
// Use the passed in context
WrappedContext *nContext = ObjectWrap::Unwrap<WrappedContext>(sandbox);
- context = nContext->GetV8Context();
+ context = Persistent<Context>::New(nContext->GetV8Context());
}
+ // context_scope will enter and exit the Context automatically
+ std::auto_ptr<Context::Scope> context_scope;
@bnoordhuis Owner

Use of the STL is proscribed in Node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // context_dtor will dispose the persistent handle automatically
+ PersistentScope<Context> context_dtor(context);
+
// New and user context share code. DRY it up.
if (context_flag == userContext || context_flag == newContext) {
- // Enter the context
- context->Enter();
+ // Enter the context. Disposes it at the end of this function. Magic.
+ context_scope.reset(new Context::Scope(context));
// Copy everything from the passed in sandbox (either the persistent
// context for runInContext(), or the sandbox arg to runInNewContext()).
@@ -407,11 +425,6 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
if (output_flag == returnResult) {
result = script->Run();
if (result.IsEmpty()) {
- if (context_flag == newContext) {
- context->DetachGlobal();
- context->Exit();
- context.Dispose();
- }
return try_catch.ReThrow();
}
} else {
@@ -429,16 +442,6 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
CloneObject(args.This(), context->Global()->GetPrototype(), sandbox);
}
- if (context_flag == newContext) {
- // Clean up, clean up, everybody everywhere!
- context->DetachGlobal();
- context->Exit();
- context.Dispose();
- } else if (context_flag == userContext) {
- // Exit the passed in context.
- context->Exit();
- }
-
return result == args.This() ? result : scope.Close(result);
}
Something went wrong with that request. Please try again.