phpv8 / v8js

V8 Javascript Engine for PHP — This PHP extension embeds the Google V8 Javascript Engine
http://pecl.php.net/package/v8js
MIT License
1.83k stars 200 forks source link

Unable to catch JavaScript errors when using V8Function objects outside of V8 context #127

Closed christiaan closed 9 years ago

christiaan commented 9 years ago

I don't see this behaviour described anywhere but it would be really nice to do the following.

<?php
$sandbox = new V8Js();

$cb = $sandbox->executeString('(function() { return oh; });');

try {
    $cb();
} catch( \Exception $e) {
    var_dump($e);
}

This more complex example even gives me a segmentation fault.

<?php
$hook = array(
    'code' => 'this.memory.startTime = 54321; return require("oets")();',
    'memory' => '{"startTime" : "12345"}'
);
$name = 'preDetermineRecipientsHook';
$properties = array();

$sandbox = new V8Js();

$sandbox->setModuleLoader(function($module) use($sandbox) {
        $sandbox->test = 'hoi';
       return 'module.exports = function() { return oh; };';
    });

/** @var callable $invokableHook */
$invokableHook = $sandbox->executeString(
    sprintf(
        '(function(self) {
    return (function() {
    %s
    }).call(self);
});',
        $hook['code']
    ),
    $name,
    V8Js::FLAG_NONE,
    500
);

$memory = $hook['memory'] ? json_decode($hook['memory']) : new \stdClass();

$result = $invokableHook(array('memory' => $memory, 'properties' => $properties));
// If the above line is changed to the following everything works as expected.
//$sandbox->invokableHook = $invokableHook;
//$sandbox->invokableHookArgs = array('memory' => $memory, 'properties' => $properties);
//$result = $sandbox->executeString('PHP.invokableHook(PHP.invokableHookArgs);');

var_dump($result);
var_dump($memory);
stesie commented 9 years ago

Nice catches :-)

I agree, that the error from the first script should be catchable. Besides it's especially strange that it seems to be written to stdout.

Wrt. your second script, I can reproduce that:

(gdb) bt
#0  v8js_to_zval (jsValue=jsValue@entry=..., return_value=return_value@entry=0x7ffff7fc4bc0, flags=1, 
    isolate=isolate@entry=0x109f020) at /home/sts/Projekte/v8js/v8js_convert.cc:171
#1  0x00007ffff4f20a79 in php_v8js_v8_call_method (method=<optimized out>, ht=1, return_value=0x7ffff7fc4bc0, 
    return_value_ptr=<optimized out>, this_ptr=<optimized out>, return_value_used=1) at /home/sts/Projekte/v8js/v8js.cc:516
#2  0x000000000079d75f in zend_do_fcall_common_helper_SPEC (execute_data=0x7ffff7f8c440)
    at /build/buildd/php5-5.5.9+dfsg/Zend/zend_vm_execute.h:600
#3  0x0000000000717758 in execute_ex (execute_data=0x7ffff7f8c440) at /build/buildd/php5-5.5.9+dfsg/Zend/zend_vm_execute.h:363
#4  0x00000000006dd8c9 in dtrace_execute_ex (execute_data=<optimized out>) at /build/buildd/php5-5.5.9+dfsg/Zend/zend_dtrace.c:73
#5  0x00000000006ef350 in zend_execute_scripts (type=type@entry=8, retval=retval@entry=0x0, file_count=file_count@entry=3)
    at /build/buildd/php5-5.5.9+dfsg/Zend/zend.c:1316
#6  0x000000000068f235 in php_execute_script (primary_file=primary_file@entry=0x7fffffffcb00)
    at /build/buildd/php5-5.5.9+dfsg/main/main.c:2506
#7  0x000000000079f9fe in do_cli (argc=9, argv=0xebf420) at /build/buildd/php5-5.5.9+dfsg/sapi/cli/php_cli.c:994
#8  0x0000000000461de0 in main (argc=9, argv=0xebf420) at /build/buildd/php5-5.5.9+dfsg/sapi/cli/php_cli.c:1378

v8::Function::Call returns a null pointer and V8Js assumes it always is set, if PHP wants to have it.

I'll have a closer look tomorrow or so

cheers ~stesie

christiaan commented 9 years ago

I've been giving it some more tought.. Besides fixing this behaviour would it be possible to have something like this? To make the whole V8Function more usable.

<?php
$sandbox = new V8Js();

$cb = $sandbox->executeString('(function(a, b) { return a + b; });');

$cbArgs = array(2, 3);
$sandbox->executeFunction($cb, $cbArgs, $time_limit, $memory_limit); // returns 5
// Or
$cb->execute($cbArgs, $time_limit, $memory_limit); // returns 5

Another option could also be to make it impossible to invoke the V8Function from outside of a V8 context then I'll just work around it. Which seems to be the supported way of doing it anyway.

stesie commented 9 years ago

ehh, one after another :)

Concerning your initial examples the second one was simply a follow-up bug triggered by the exception not being handled properly. If a exception is thrown, the Call function of v8 doesn't return a value ... but if PHP wants to have a result, V8Js tried mapping anyways. You can make the first example crash also, if you simply assign the return value of $cb() to a variable.

Anyways, that's fixed now, see above. Jenkins just turned blue so I'll push to master shortly.


Concerning your most recent note, I think what you're trying to achieve is limitting time and memory consumption when calling V8Functions, right? I stumbled across the fact that that's not possible while fixing the bug ... never noticed before, but never missed it so far also, but I see & agree it's needed.

So I'm not sure if a executeFunction call is pretty to be honest. I like that it's possible to simply call the functions "the PHP way" (it has that call_user_func smell) ... and your second suggestion either doesn't work with functions having unlimited number of arguments or wouldn't be backwards compatible if you don't make them optional.

I was thinking of either

Regarding the time limit we could also distinct between "single call maximum runtime" and "total maximum runtime". I.e. tell JS to never execute for more than 50ms, even if it's 5000 calls from PHP to JS.

What do you think?

cheers ~stesie

PS: I'll immediately open a follow-up issue for the time/memory limit thing where we can discuss further

PPS: fatal error handling & pending exception stuff is also broken wrt. to V8Function calls