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.84k stars 200 forks source link

Isolate / general protection faults #472

Closed redbullmarky closed 2 years ago

redbullmarky commented 2 years ago

Hey

I'm currently investigating a few different issues that occur somewhat randomly & infrequently, giving these sort of outputs in the logs:

traps: php[5367] general protection fault ip:7fdd4d25479a sp:7ffc221e4680 error:0 in v8js.so[7fdd4d245000+15000]

PHP Warning: Fatal V8 error in v8::Isolate::Dispose(): Disposing the isolate that is entered by a thread.

V8Js 2.1.1 with V8 8.3, PHP 7.4.3

At the moment, i'm just trying to find out what circumstances either of them could be triggered. They occur on otherwise very busy systems with a lot of moving parts, so it's hard to narrow down or add relevant debugging without knowing a bit more about why they might happen. The V8Js instance is used to return an array of functions that are called, therefore the V8 instance is kept alive for as long as its needed (though I do recycle it every 1000 or so iterations replacing it with a new instance.

I am using executeString with the flags V8Js::FLAG_FORCE_ARRAY | V8Js::FLAG_PROPAGATE_PHP_EXCEPTIONS

Whilst there might not be enough info above to identify a specific problem with the library yet, even just being able to trigger them (especially the latter of the two errors) consistently would be a HUGE help in determining whether it's the code, the library or V8 itself at fault.

Cheers!

redbullmarky commented 2 years ago

Unsure yet as to whether this is related, but if you return a callable from your JS and destroy the V8 instance, you get this: Can't access V8Object after V8Js instance is destroyed!

however, if you destroy the instance somehow from within, you get a segfault.

class myjs extends \V8Js
{
   public function bosh()
   {
      $GLOBALS['v8test'] = null;
   }
}

$GLOBALS['v8test'] = new myjs('myjs');
$GLOBALS['v8test']->executeString('myjs.bosh()');
stesie commented 2 years ago

Regarding your second comment ...

The Can't access V8Object after V8Js instance is destroyed! exception after destroying the V8 instance is expected, since the V8Object instances themselves don't hold a reference on V8. Also this should provide a means to the PHP world to definitely being able to stop V8 engine ... so you simply keep the instance around as long as you want it to be running and then destroy it.

As to the segfault report, of course this shouldn't happen. Yet I cannot reproduce it, just tried with PHP 7.4.29. Do you remember which version of PHP you were testing with back in November?

Also setting a breakpoint in v8js_execute_script shows the refcount to be 2, so it should be safe.

Thread 1 "php" hit Breakpoint 1, v8js_execute_script (this_ptr=0x7f4474335900, res=0x603000028990, flags=1, time_limit=0, memory_limit=0, return_value=0x7ffd12aab7d0)
    at /php-7.4.29/ext/v8js/v8js_class.cc:668
668     {
(gdb) print *this_ptr
$4 = {value = {lval = 107202383712112, dval = 5.2965014944445744e-310, counted = 0x618000000f70, str = 0x618000000f70, arr = 0x618000000f70, obj = 0x618000000f70,
    res = 0x618000000f70, ref = 0x618000000f70, ast = 0x618000000f70, zv = 0x618000000f70, ptr = 0x618000000f70, ce = 0x618000000f70, func = 0x618000000f70, ww = {w1 = 3952,
      w2 = 24960}}, u1 = {v = {type = 8 '\b', type_flags = 3 '\003', u = {extra = 32}}, type_info = 2097928}, u2 = {next = 1, cache_slot = 1, opline_num = 1, lineno = 1,
    num_args = 1, fe_pos = 1, fe_iter_idx = 1, access_flags = 1, property_guard = 1, constant_flags = 1, extra = 1}}
(gdb) print *this_ptr->value.obj
$5 = {gc = {refcount = 2, u = {type_info = 3221226520}}, handle = 1, ce = 0x631000014818, handlers = 0x557b02583b00 <v8js_object_handlers>, properties = 0x6060000059c0,
  properties_table = {{value = {lval = 0, dval = 0, counted = 0x0, str = 0x0, arr = 0x0, obj = 0x0, res = 0x0, ref = 0x0, ast = 0x0, zv = 0x0, ptr = 0x0, ce = 0x0,
        func = 0x0, ww = {w1 = 0, w2 = 0}}, u1 = {v = {type = 0 '\000', type_flags = 0 '\000', u = {extra = 0}}, type_info = 0}, u2 = {next = 0, cache_slot = 0,
        opline_num = 0, lineno = 0, num_args = 0, fe_pos = 0, fe_iter_idx = 0, access_flags = 0, property_guard = 0, constant_flags = 0, extra = 0}}}}

But might very well be that ref-counting behaved differently in older PHP versions...

Anyways, I like the idea of trying to destroy V8Js instance via $GLOBALS ... kudos for that :-)

redbullmarky commented 2 years ago

@stesie i believe it would have been 7.4.3. I’ll give it a checkover tomorrow with this version as well as the latest 7.4.x

redbullmarky commented 2 years ago

@stesie tested this one; i can confirm that it no longer seems to be an issue.

redbullmarky commented 2 years ago

Managed to recreate it again now, perhaps the original example wasn't right...this time, i just wrapped the JS inside of a function that gets returned.

class myjs extends \V8Js
{
   public function bosh()
   {
      $GLOBALS['v8test'] = null;
      unset($GLOBALS['v8test']);
    }
}

$GLOBALS['v8test'] = new myjs('myjs');
$ret = $GLOBALS['v8test']->executeString('
    (() => {
        myjs.bosh()
    })
');

$ret();
print_r($ret);

if i change the JS code to be a self-executing function and removed $ret(), then it'll not segfault - examining $ret will produce NULL which seems sensible given that i destroyed it from inside.

this is very much an edge case that, in our situation, is probably impossible to produce. At one point a few versions ago, we'd replace the main V8Js instance from time to time with a fresh instance (to fully clean up memory, etc) and this segfault would occasionally occur.

I guess it's one of those "if we can fix it, great!" ones...the issue with segfaults is how brutal they are.

EDIT: p.s. this is completely unrelated to the most recent commit (and probably unrelated to the same problems the fix was for), the same outcome happens before and after applying it.

stesie commented 2 years ago

@redbullmarky just created a PR which should fix the issue. Let me know if it works for you.

redbullmarky commented 2 years ago

@stesie it certainly does! no issues here and all tests pass.