737611 - Crash on exit following creation of multiple JSRuntime instances from the same process.

Status: RESOLVED INACTIVE

(Core :: JavaScript Engine, defect)

Description

[Steve Williams]

Attachment: Patch for the above as discussed with sfink

As title. When first JSRuntime instance released, the static critical section in prmjtime.cpp was destroyed. This caused the second instance to fail as it accesses the same critical section.

Have patched to counts the number of active runtimes and only destroy the critical section when the last instance is destroyed.

Comment 1

[Steve Williams]

Though the patch I've submitted above does indeed improve stability, it's not a complete solution.

Take the following scenario (using the above patch) :

Create 2 Runtimes, delete them both, the csec is correctly deleted when the last runtime is destroyed.

However, if another runtime is then created, a corresponding csec will not be created because there is a RunOnce mechanism triggering its creation.

The RunOnce static initializer should be reset in the csec destructor so it is activated again if a subsequent runtime is created.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 607684 [details] [diff] [review]
Patch for the above as discussed with sfink

You're not supposed to call JS_ShutDown until you're totally done using SpiderMonkey. Calling it after destroying each JSRuntime is just a mistake.

I don't think it's a bug in SpiderMonkey.

Comment 3

[Steve Williams]

> You're not supposed to call JS_ShutDown until you're totally done using SpiderMonkey.

Consider two modules in the same application that share the same jscript backend (spidermonkey). they should be able to run independantly of each other without having to communicate state. this is an easy fix that improves stability. if you don't want it, I'll maintain seperately.

Comment 4

[David Mandelin [:dmandelin]]

(In reply to Steve Williams from comment #3)
> > You're not supposed to call JS_ShutDown until you're totally done using SpiderMonkey.
> 
> Consider two modules in the same application that share the same jscript
> backend (spidermonkey). they should be able to run independantly of each
> other without having to communicate state. this is an easy fix that improves
> stability. if you don't want it, I'll maintain seperately.

Would it work to just use JS_DestroyRuntime to close each runtime when you are done with that runtime, and call JS_ShutDown only when the application exits?

Comment 5

[Steve Williams]

One of the runtimes is in the firefox tree so I'd have to custom patch that. it's easier and better to fix it properly. that way no-one else runs into the problem. to state you're not supposed to re-enter a module is an unnecessary constraint. we could have fixed it in the time we spend arguing over it :)

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Steve Williams from comment #3)
> Consider two modules in the same application that share the same jscript
> backend (spidermonkey). they should be able to run independantly of each
> other without having to communicate state. this is an easy fix that improves
> stability. if you don't want it, I'll maintain seperately.

I agree it's kind of gross that the SpiderMonkey API is like this.

With this patch there is a race condition: one thread can be doing JS_ShutDown cleanup when another thread calls JS_NewRuntime.

I think if we really want to make this better it's best to get rid of JS_ShutDown entirely, and get rid of any shared state that requires cleanup.

(In reply to Steve Williams from comment #5)
> One of the runtimes is in the firefox tree so I'd have to custom patch that.
> it's easier and better to fix it properly. that way no-one else runs into
> the problem. to state you're not supposed to re-enter a module is an
> unnecessary constraint. we could have fixed it in the time we spend arguing
> over it :)

Firefox never calls JS_ShutDown until it really is shutting down. You have to call nsComponentManagerImpl::Shutdown in order to trigger the call to JS_ShutDown.

Just remove your own JS_ShutDown call and it should be fine.

Comment 7

[Steve Williams]

> With this patch there is a race condition: one thread can be doing JS_ShutDown cleanup when another thread calls JS_NewRuntime.

true but it's better than what's there right now. removing the shared state as you suggest is the best solution.

can't simply remove my own JS_Shutdown as the module that contains it doesn't know if gecko was ever initialized. maybe it was, maybe it wasn't. it doesn't know or care. I'd like to keep it that way. the patch above lets me keep a clean API so I'll use it for now as the non-gecko module needs to clean up properly on exit, regardless of what else might be going on.

Comment 8

[David Mandelin [:dmandelin]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> I think if we really want to make this better it's best to get rid of
> JS_ShutDown entirely, and get rid of any shared state that requires cleanup.

I agree that the best solution is not to need JS_ShutDown. It really is a strange API to have given that runtimes are supposed to be independent.

JS_ShutDown does two things:

- Probes::shutdown(). Some of this looks like it could be on a per-runtime basis, but there is some Windows ETW stuff that may be per-process. If so, we could make Probes require a shutdown call separate from the runtime.

- PRMJ_NowShutDown(). This frees two mutexes used by the implementation of PRMJ_Now. It seems like they could move to the runtime, but I don't know if that makes it harder to have a large number of runtimes.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.