Permalink
Browse files

vm: fix cyclic reference memory leak

Clean out the context's prototype object after we're done with it. Avoids
cyclic references that the V8 garbage collector is not able to resolve.
  • Loading branch information...
1 parent a25ebb1 commit 551001fc5e473256c0e73e9e0ac8b605b26e514c @bnoordhuis committed Nov 21, 2012
Showing with 54 additions and 1 deletion.
  1. +12 −1 src/node_script.cc
  2. +42 −0 test/pummel/test-vm-memleak-circular.js
View
@@ -432,7 +432,18 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
if (context_flag == userContext || context_flag == newContext) {
// success! copy changes back onto the sandbox object.
- CloneObject(args.This(), context->Global()->GetPrototype(), sandbox);
+ Local<Value> prototype_v = context->Global()->GetPrototype();
+ assert(prototype_v->IsObject());
+ Local<Object> prototype = prototype_v.As<Object>();
+
+ CloneObject(args.This(), prototype, sandbox);
+
+ // Clean out the prototype object again. Avoids memory leaks when there
+ // are cyclic references between the context and the outside world.
+ Local<Array> property_names = prototype->GetPropertyNames();
+ for (unsigned int i = 0, k = property_names->Length(); i < k; i++) {
+ prototype->ForceDelete(property_names->Get(i));
+ }
}
return result == args.This() ? result : scope.Close(result);
@@ -0,0 +1,42 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var vm = require('vm');
+var assert = require('assert');
+var common = require('../common');
+
+var maxMem = 0;
+
+var interval = setInterval(function() {
+ var ctx = vm.createContext({});
+ ctx.obj = { ctx: ctx };
+ vm.runInContext('', ctx);
+ maxMem = Math.max(maxMem, process.memoryUsage().rss);
+}, 1);
+
+setTimeout(function() {
+ clearInterval(interval);
+}, 5000);
+
+process.on('exit', function() {
+ console.error('max mem: %dmb', Math.round(maxMem / (1024 * 1024)));
+ assert.ok(maxMem < 50 * 1024 * 1024);
+});

5 comments on commit 551001f

LGTM

@mraleph, if you have any comments, drop them here :-)

@bnoordhuis: Vyacheslav tells me to file a v8 bug. The honor is yours.

Owner

bnoordhuis replied Nov 21, 2012

I'll see if I can turn it into a standalone test case.

What's the status on this one? Can confirm that the test case provided above still fails on mac running master (v0.13.0-pre) but the fix above definitely looks to be out of date.

Owner

bnoordhuis replied Dec 12, 2014

@jasnell It's been a while but I don't think I managed to put that standalone test case together. For that matter, I suspect it has only got worse with the integration of contextify into core. If it's still an issue for you, can you file an issue at https://github.com/iojs/io.js/issues? Thanks.

Please sign in to comment.