Bug 114487 – JSLockHolder in JSCMainThreadExecState::evaluate seems redundant

• Component: JavaScriptCore
• Status: UNCONFIRMED

[SangGyu Lee]

Hello,

In Source/WebCore/bindings/js/JSMainThreadExecState.h


static JSC::JSValue evaluate(JSC::ExecState* exec, const JSC::SourceCode& source, JSC::JSValue thisValue, JSC::JSValue* exception)
{
    JSMainThreadExecState currentState(exec);
    JSC::JSLockHolder lock(exec);
    return JSC::evaluate(exec, source, thisValue, exception);
};


The statement "JSC::JSLockHolder lock(exec);" seems redundant since JSC::evaluate always locks same JSGlobalData like followings:

In JSC::evaluate(), 

JSValue evaluate(ExecState* exec, const SourceCode& source, JSValue thisValue, JSValue* returnedException)
{       
    JSLockHolder lock(exec);
    ....
}

Although JSLock uses spinlock and lightweight, therefore the overhead may be insignificant, it seems redundant.

Is it right? Or am I missing something?
Could you tell me the reason of locking twice?

[Mark Hahnenberg]

> Although JSLock uses spinlock and lightweight, therefore the overhead may be insignificant, it seems redundant.
> 
> Is it right? Or am I missing something?
> Could you tell me the reason of locking twice?

I believe you're correct that in this particular case locking twice is redundant. I'm sure you could find a number of places throughout the codebase with this sort of redundancy, but it's not really a problem since JSLock is reentrant. The general rule for calling into JSC is to always at least take the JSLock (sometimes you also want to use an APIEntryShim, which takes the lock for you), then do your JSC work. The reentrancy of JSLock allows you to not worry whether or not the current thread already holds the lock. You also don't have to worry about whether the function you're calling takes the lock or whether you need to take it yourself.

Many people don't like reentrant locks because they think they're sloppy. Is there a reason that this reentrancy concerns you aside from its arguable sloppiness?