Bug 30678 - Database thread initialization not properly ifdef'ed for JSC vs V8
: Database thread initialization not properly ifdef'ed for JSC vs V8
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Steve Block
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-22 10:07 PDT by Steve Block
Modified: 2009-10-28 09:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch 1 for Bug 30678 (2.57 KB, patch)
2009-10-22 10:11 PDT, Steve Block
eric: review-
Details | Formatted Diff | Diff
Patch 2 for Bug 30678 (6.06 KB, patch)
2009-10-23 09:58 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch 3 for Bug 30678 (5.97 KB, patch)
2009-10-26 11:45 PDT, Steve Block
eric: review+
steveblock: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 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 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 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 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 2009-10-28 09:56:30 PDT
Sorry.  The commit-queue recently got the hiccups.  :(  bug 30869