We leak variables assigned to the V8 global

[cscott]

The following code:

$v8 = new V8Js();
$v8->executeString("var foo='bar';");
$v8->executeString("print(foo)");

...prints bar -- that is, the variable foo is retained between invocations of executeString. Further, foo then leaks if it's a reference to a PHP object, for example:

class Bar { }
$v8 = new V8Js();
$v8->bar = new Bar;
$v8->executeString("var foo=PHP.bar;");

Somehow we're not cleaning up the v8 global object when the V8Js object is destroyed.

We should do that -- perhaps even doing that between executions of executeString, as I'm not sure that behavior in the first code example is not a bug. Maybe this is as easy as wrapping executeString's execution in a new v8 scope so it's not running in global scope? Perhaps we also want to define a globals object like node does which exposes the global scope if you want to share context between executeString calls.

[cscott]

The above commit demonstrates the problem, by way of illustrating what is necessary to prevent leak errors caused by this issue in our test suite.

[cscott]

FWIW, with the hacky workaround above and pull requests #73 and #74, our test suite passes with no memory leaks (yay). So once we fix this issue we should be good on that front (and I should see if I can reconfigure travis to do the more aggressive leak checking).

[satoshi75nakamoto]

@cscott — Yeah it would be nice to reconfigure travis for testing. Also we could use a labs instance to host travis if we wanted as well.

[stesie]

I'm not sure how to nicely fix this either. After all it would be easy if V8 would call the weak-callbacks if the last context or the isolate is destroyed. But unfortunately it doesn't.

Hence I'm not sure if it'll trigger the weak-callbacks if we'd wrap the executeString calls in seperate contexts (but haven't tried). After all we already force a garbage collection from within php_v8js_free_storage after we destroyed our global context and it doesn't call the weak-callbacks which would de-ref the PHP object as needed.

Thus a globals object of some sort wouldn't help much regarding this issue. Also I'm unsure whether we should introduce the globals object apart from that. After all it's backwards incompatible and might break existing users; on the contrary accidental leakage already can be easily avoided by wrapping the code in a self-calling function, which is kind of best practise anyways... This is I would prefer not to change current behaviour.

Therefore we might just want to keep a list of all PHP objects we addref'ed within php_v8js_ctx and simply release those references from php_v8js_free_storage, feels a bit like a work-around so :-)

[stesie]

Revisiting this almost one year later

• I'm heavily relying on the behaviour you pointed out in the first code block above, that global variables are shared between executeString calls.
• the v8::Isolate is disposed correctly meanwhile, and does not cause crashes any longer (which we omitted it)
• therefore no memory leaking left
• ... and ZTS build is green (read "blue" in Jenkins) for quite a while now

... hence I consider this issue resolved and just close it.