Bug 30678 – Database thread initialization not properly ifdef'ed for JSC vs V8

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

WebCore/storage/Database.cpp and WebCore/loader/icon/IconDatabase.cpp both use calls to initializeThreading(). These calls must be properly ifdef'ed to use the appropriate JS engine - JSC or V8. These ifdefs are incomplete.

[Steve Block]

Created attachment 41665 [details]
Patch 1 for Bug 30678

[Eric Seidel (no email)]

Why can't we share this code?  Can't we make this go through ScriptController or otherwise make it generic?  This seems like the wrong approach.

[Eric Seidel (no email)]

Comment on attachment 41665 [details]
Patch 1 for Bug 30678

r- for above reasons.  At least the headers could easily use the same paths using things like ForwardingHeaders.

[Dimitri Glazkov (Google)]

AFAIK, there's no V8::initializeThreading defined anywhere?

[Steve Block]

Created attachment 41730 [details]
Patch 2 for Bug 30678

> Can't we make this go through ScriptController or otherwise make it generic?
Done

> AFAIK, there's no V8::initializeThreading defined anywhere?
Sorry, yes, my patches got out of order. This code is now included in this patch.

[Eric Seidel (no email)]

Comment on attachment 41730 [details]
Patch 2 for Bug 30678

+    // Darwin is an exception to this rule: it is OK to call this function from any thread, even reentrantly.

The code does not back that up.  The JSC version has no reentrancy guard, but the V8 one does.  That seems to have little to do with darwin vs. non-darwin.

[Steve Block]

Created attachment 41878 [details]
Patch 3 for Bug 30678

> (From update of attachment 41730 [details])
> +    // Darwin is an exception to this rule: it is OK to call this function
> from any thread, even reentrantly.
>
> The code does not back that up.  The JSC version has no reentrancy guard, but
> the V8 one does.  That seems to have little to do with darwin vs. non-darwin.
You're right that the comment about Darwin on V8 is wrong - that was a copy-paste error - now fixed.

The comment for the JSC version is taken from JSC::initializeThreading() in JavaScriptCore/runtime/InitializeThreading.h, so should be correct. The implementation of ScriptController::initializeThreading() for V8 is the same as the logic in JSC::initializeThreading() (less the Darwin and JSC specifics). Both guard against repeated calls to the function.

[Eric Seidel (no email)]

Comment on attachment 41878 [details]
Patch 3 for Bug 30678

Looks sane.  You might consider adding a comment in ScriptControolerV8::initializeThreading that any early-init calls which v8 needs for multi-threaded operation should be made there.

[Steve Block]

Comment on attachment 41878 [details]
Patch 3 for Bug 30678

Commit queue is offline. Landing manually.

[Steve Block]

Committed r50215: <http://trac.webkit.org/changeset/50215>

[Eric Seidel (no email)]

Sorry.  The commit-queue recently got the hiccups.  :(  bug 30869