Bug 30678

Summary:

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

Product:

WebKit

Reporter:

Steve Block <steveblock>

Component:

WebCore Misc.

Assignee:

Steve Block <steveblock>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benm, dglazkov, eric, fishd, sam, steveblock

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch 1 for Bug 30678	eric: review-
Patch 2 for Bug 30678	none
Patch 3 for Bug 30678	eric: review+, steveblock: commit-queue-

Description Steve Block 2009-10-22 10:07:25 PDT

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.

Comment 1 Steve Block 2009-10-22 10:11:52 PDT

Created attachment 41665 [details]
Patch 1 for Bug 30678

Comment 2 Eric Seidel (no email) 2009-10-22 10:39:10 PDT

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.

Comment 3 Eric Seidel (no email) 2009-10-22 10:39:44 PDT

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.

Comment 4 Dimitri Glazkov (Google) 2009-10-22 12:07:00 PDT

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

Comment 5 Steve Block 2009-10-23 09:58:12 PDT

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.

Comment 6 Eric Seidel (no email) 2009-10-23 11:08:42 PDT

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.

Comment 7 Steve Block 2009-10-26 11:45:40 PDT

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.

Comment 8 Eric Seidel (no email) 2009-10-27 17:06:26 PDT

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.

Comment 9 Steve Block 2009-10-28 06:54:07 PDT

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

Commit queue is offline. Landing manually.

Comment 10 Steve Block 2009-10-28 06:55:38 PDT

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

Comment 11 Eric Seidel (no email) 2009-10-28 09:56:30 PDT

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