Make Runtime::new safe - Githubissues

nox commented 8 years ago

CC @tschneidereit @Ms2ger @jdm

This change is

tschneidereit commented 8 years ago

Looks good with comments addressed. Using a lazy static is a nice solution.

Previously, nox (Anthony Ramine) wrote…

> #### Make Runtime::new safe > > CC @tschneidereit @Ms2ger @jdm > >

Reviewed 9 of 9 files at r1. Review status: all files reviewed at latest revision, 4 unresolved discussions.

[src/rust.rs, line 122 [r1]](https://reviewable.io:443/reviews/servo/rust-mozjs/267#-KJ1cbP0z76ASsUFqiDZ:-KJ1cbP0z76ASsUFqiD:2012083994) (raw file):_

    pub unsafe fn new(parent_rt: *mut JSRuntime) -> Runtime {
        let js_runtime = JS_NewRuntime(default_heapsize, ChunkSize as u32, parent_rt);
        assert!(!js_runtime.is_null());

We still need this assert or some other form of null check. Creating a child runtime is just as fallible as creating the parent runtime, after all.

[src/rust.rs, line 125 [r1]](https://reviewable.io:443/reviews/servo/rust-mozjs/267#-KJ1cj9bFTm7b346r-M:-KJ1cj9bFTm7b346r-Ma:916274856) (raw file):_

                        assert!(JS_Init());
                        let runtime = JS_NewRuntime(
                            default_heapsize, ChunkSize as u32, ptr::null_mut());

This should use as small a heap size as possible. The only JS ever running in here should be the self-hosted code. Not sure how best to figure out a good value, though.

src/rust.rs, line 128 [r1] (raw file):

                        assert!(!runtime.is_null());
                        let context = JS_NewContext(
                            runtime, default_stacksize as size_t);

Much more important than the heap size, and should be much smaller. 1/16th (512kb) might well be enough, or even 1/32th (256kb).

src/rust.rs, line 130 [r1] (raw file):

                            runtime, default_stacksize as size_t);
                        assert!(!context.is_null());
                        TopRuntime(runtime)

It might make sense to trigger a compacting GC at this point, as no new code execution should happen in this runtime afterwards. I also wonder if we can write-protect the runtime's heap afterwards?

Comments from Reviewable

tschneidereit commented 8 years ago

@jandem informs me that both the stack and the heap are allocated lazily, so I guess my comments on their sizes can be safely ignored.

nox commented 8 years ago

Put back the assertion. LGTM?