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
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-10-22 10:07 PST by
Modified: 2009-10-28 09:56 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-22 10:07:25 PST
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 From 2009-10-22 10:11:52 PST -------
Created an attachment (id=41665) [details]
Patch 1 for Bug 30678
------- Comment #2 From 2009-10-22 10:39:10 PST -------
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 From 2009-10-22 10:39:44 PST -------
(From update of attachment 41665 [details])
r- for above reasons.  At least the headers could easily use the same paths using things like ForwardingHeaders.
------- Comment #4 From 2009-10-22 12:07:00 PST -------
AFAIK, there's no V8::initializeThreading defined anywhere?
------- Comment #5 From 2009-10-23 09:58:12 PST -------
Created an attachment (id=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 From 2009-10-23 11:08:42 PST -------
(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.
------- Comment #7 From 2009-10-26 11:45:40 PST -------
Created an attachment (id=41878) [details]
Patch 3 for Bug 30678

> (From update of attachment 41730 [details] [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 From 2009-10-27 17:06:26 PST -------
(From update of attachment 41878 [details])
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 From 2009-10-28 06:54:07 PST -------
(From update of attachment 41878 [details])
Commit queue is offline. Landing manually.
------- Comment #10 From 2009-10-28 06:55:38 PST -------
Committed r50215: <http://trac.webkit.org/changeset/50215>
------- Comment #11 From 2009-10-28 09:56:30 PST -------
Sorry.  The commit-queue recently got the hiccups.  :(  bug 30869