Prevent initializing and shutting down SpiderMonkey from different threads

[jdm]

Based on this comment, it appears that JS_Init and JS_ShutDown need to be called on the same thread. These changes make JSEngine a non-sendable type, and add JSEngineHandle as a sendable type that allows verifying that all outstanding consumers of the engine are dropped before shutting down.

[jdm]

Note to self: need to update unit tests.

[gterzian]

To elaborate on servo/servo#24845 (comment)

I think you could make outstanding_handles: Arc<AtomicU32> into a outstanding_handles: Arc<(Mutex<EngineHandles>, Condvar)>.

with EngineHandles being:

enum EngineHandles {
    /// Initial state, equivalent to 0 handles, but before any handles have been created.
    Initialized,
    /// Count handles, start at 1, goes to 0 when all handles are dropped.
    Count(u32)
}

with can_shutdown containing the equivalent of while outstanding_handles != EngineHandles::Count(0) {//wait on the condvar}.

So that the thread in script can then block on that call, and then shutdown.

[gterzian]

Could impl !Send for JSEngine{} be used for that purpose?

[jdm]

error[E0658]: negative trait bounds are not yet fully implemented; use marker types for now
   --> src/rust.rs:181:1
    |
181 | impl !Send for JSEngine {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: for more information, see https://github.com/rust-lang/rust/issues/13231
    = help: add `#![feature(optin_builtin_traits)]` to the crate attributes to enable

I'd rather not introduce nightly-only features to this crate.

[jdm]

The idea of making shutdown be a blocking operation is interesting, but I think I'd rather not make that a requirement of using this crate. It's not difficult to imagine a consumer writing code that uses this API that ends up creating a deadlock by not ensuring that all of the engine handles are disposed of before attempting to shutdown. I would rather provide an API that panics by default and allow consumers to build their own blocking shutdown behaviour on top of it, like servo/servo#24862 does for Servo.

[gterzian]

I would rather provide an API that panics by default and allow consumers to build their own blocking shutdown behaviour on top of it

Yes that makes sense, although it's not clear to me how a non-blocking use, or a blocking use that isn't a spinlock waiting for it to return true, would look like.

It would be possible for a consumer to deadlock on a blocking shutdown operation, and such deadlock is likely to be not intermittent and quickly fixed. On the other hand, a non-blocking can_shutdown could lead to intermittent issues.

I guess either way for the API could be a valid choice thought...

[asajeffrey]

Is it worth dedicating a thread to JS engine creation and destruction? That would make the API a bit cleaner, but at the cost of yet another thread. I'll leave that up to you, this implementation LGTM.

[jdm]

@bors-servo r=asajeffrey

[bors-servo]

📌 Commit c8c1453 has been approved by asajeffrey

[bors-servo]

⌛ Testing commit c8c1453 with merge 9b0d063...

[bors-servo]

☀️ Test successful - checks-travis, status-appveyor
Approved by: asajeffrey
Pushing 9b0d063 to master...