Segmentation fault with safe code

[espindola]

The following code crashes if V8 is built with debug checks:

use rusty_v8 as v8;
fn create() -> v8::OwnedIsolate {
    v8::Isolate::new(Default::default())
}
fn main() {
    let v8_platform = v8::new_default_platform(0, false).make_shared();
    v8::V8::initialize_platform(v8_platform);
    v8::V8::initialize();
    let mut bar = create();
    bar = create();
}

The failed check is:

# Fatal error in ../../../../rusty_v8/v8/src/execution/isolate.cc, line 3897
# Debug check failed: CurrentPerIsolateThreadData()->isolate_ == this.

The problems comes from the fact that rust compiles the last two lines into code that is equivalent to:

let foo = create();
let bar = create();
drop(foo);

This would be a small annoyance, if not for the fact that without a debug v8 we don't get a reliable assert and can instead end up with a segmentation fault in a bigger program.

I can't think of any solution other than keeping our own mini stack of isolates ids so that we can panic! if we try to exit one that is not at the top of the stack.

[srijs]

I've run into this issue as well, and after applying a manual workaround by adding explicit drops, have been thinking about ways to solve it.

It looks to me like it may not be limited to the drop/exit scenario as well? Generally, when multiple OwnedIsolates exist simultaneously in a given thread, it seems that only the most recently created one will be the current isolate, and the others will segfault/crash when used?

Would the deno project be open to a breaking API change to fix this?

I think what could work here is a "scoping" approach, meaning something similar to the following example, which I believe could prevent the use of any non-current isolates in a thread.

let owned_isolate: OwnedIsolate = Isolate::new(); // create, but don't enter

owned_isolate.enter(|isolate: &mut Isolate| {
  // enters the isolate
  let handle = HandleScope::new(isolate);
  // exists the isolate
});

drop(owned_isolate); // destroy, but don't exit

I've been thinking through other approaches as well, but it appears that there aren't many complete solutions to the problem here.

[bartlomieju]

@piscisaureus please take a look

[piscisaureus]

It is a known issue. Entering an Isolate sets the “current isolate” TLS slot. Originally we had designed rusty_v8 to not rely on this TLS slot so you would never have to Enter/Exit an isolate. At some point it that approach hit a dead end and this “enter on create” logic was hacked in and this never got cleaned up.

A PR that cleans up the hack would be greatly appreciated.

[srijs]

A PR that cleans up the hack would be greatly appreciated.

Great, I'm more than happy to take this on!

@piscisaureus would you be open to discuss design constraints here a little bit before I get to coding? Specifically interested in understanding:

• What is the view of the deno project general on API breaking changes? Would an API change need to simultaneously take into account how v8 is used in deno? (I suspect yes but would like to confirm)
• The README says about the philosophy for this crate that "[t]he API should match the original API as closely as possible" as well as that a goal is to "not introduce additional call overhead". I can see multiple solutions in the design space that make a certain trade-off, where on one hand we could aim to be truer to the original v8 APIs and introduce runtime checks to make them safe, or we could deviate from the original APIs in order to make them safe at compile time w/o adding runtime overhead. How do you think about trading these off against each other?
• As I'm not an expert at the original v8 APIs, what are the constraints that we need to enforce? You said that "[a]t some point [not setting current isolate] hit a dead end"; is this documented somewhere, i.e. which APIs rely on current isolate and need additional guarding? Or is it generally problematic to allow use of an Isolate if it's not registered as the current one?