Add V8 bindings for Worker (Part I).

Add V8 bindings for Worker.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8414

[dglazkov, 2009-01-07 23:01:39]

I think I understand the general approach.

My feeling is that we need a ScriptThreading class, which abstracts out
threading ops across platforms.

I am also not sure I am qualified to review this fully. Perhaps mbelshe would be
willing to take a look?

[Mike Belshe, 2009-01-08 18:44:30]

Wow - lots of work in here!  Sorry for all the comments, I hope they are useful.

Three primary concerns:
  * I think the condition variable may need more work
  * Not sure that we're using the v8::Locker correctly in the renderer yet
  * Would like to see unit tests on the Worker threads to ensure that we're
cleaning everything up properly.

http://codereview.chromium.org/17246/diff/1/18
File base/scoped_handle_win.h (right):

http://codereview.chromium.org/17246/diff/1/18#newcode133
Line 133: #ifdef NOGDI
Should this just go into a different CL?  Not sure how its related to workers.

http://codereview.chromium.org/17246/diff/1/19
File base/thread.h (right):

http://codereview.chromium.org/17246/diff/1/19#newcode104
Line 104: // The thread ID.
This comment doesn't match the comment on line 136, that this is for debugging
purposes.

http://codereview.chromium.org/17246/diff/1/10
File webkit/port/bindings/v8/ThreadingWin.cpp (right):

http://codereview.chromium.org/17246/diff/1/10#newcode43
Line 43: // Starting the code copied from
third_party/WebKit/JavaScriptCore/wtf/ThreadingWin.cpp
Given that the threading code below already uses base::, can we use the base::
locking as well?  There is a condition variable in base too.

http://codereview.chromium.org/17246/diff/1/10#newcode45
Line 45: static Mutex* atomicallyInitializedStaticMutex;
nit: I like to explicitly initialize to NULL.

http://codereview.chromium.org/17246/diff/1/10#newcode74
Line 74: //setThreadName(mainThreadIdentifier, "Main Thread");
Take out this line?

http://codereview.chromium.org/17246/diff/1/10#newcode80
Line 80: m_mutex.m_recursionCount = 0;
FYI - The CRITICAL_SECTION already contains a recursion count.  You can see it
in winnt.h, look for the structure _RTL_CRITICAL_SECTION.

http://codereview.chromium.org/17246/diff/1/10#newcode136
Line 136: m_condition.m_mutex = ::CreateMutex(0, 0, 0);
This probably should be a CRITICAL_SECTION?

http://codereview.chromium.org/17246/diff/1/10#newcode157
Line 157: PlatformMutex& cs = mutex.impl();
I'd avoid using the PlatformMutex here and use the mutex.

http://codereview.chromium.org/17246/diff/1/10#newcode162
Line 162: ++m_condition.m_blocked;
I'm pretty sure this code is broken.  The member variables of the
PlatformCondition are sometimes protected by m_gate (such as in this case), and
other times protected by m_mutex (see signal(), line 234)

http://codereview.chromium.org/17246/diff/1/10#newcode166
Line 166: ::LeaveCriticalSection(&cs.m_internalMutex);
Shouldn't this be mutex.unlock()?  Otherwise the recursion count in the mutex
will get hoarked when someone else grabs the lock external to this function?

http://codereview.chromium.org/17246/diff/1/10#newcode174
Line 174: size_t wasTimedOut = m_condition.m_timedOut;
Something seems wrong here; I don't see any case where m_timedOut is ever
non-zero?  Is this because the timedWait call is stubbed out?  Can we get rid of
all this code dealing with timeouts?

http://codereview.chromium.org/17246/diff/1/10#newcode397
Line 397: int WebKitThreadManager::WaitForThreadCompletion(ThreadIdentifier
thread_id,
API nit:  perhaps this is out of our control, but usually this api is called
"Join".

http://codereview.chromium.org/17246/diff/1/10#newcode399
Line 399: base::Thread* thread = NULL;
Should we remove from the list before calling join?  Or shoule we remove from
the list after we've joined?

http://codereview.chromium.org/17246/diff/1/10#newcode423
Line 423: // Nothing to do.
I would expect this would remove the thread from the known threads list?  (so
that it doesn't get killed on cleanup and such?)

http://codereview.chromium.org/17246/diff/1/4
File webkit/port/bindings/v8/V8WorkerCustom.cpp (right):

http://codereview.chromium.org/17246/diff/1/4#newcode48
Line 48: CALLBACK_FUNC_DECL(WorkerConstructor) {
Overall:  We need test cases for all of these.  We'll also need to run leak
tests to make sure that we don't leak listeners.  Can you add layout tests?  Or
do they already exist?

http://codereview.chromium.org/17246/diff/1/4#newcode93
Line 93: // TODO(mbelshe) - merge these with XHR's CreateHiddenXHRDependency
Ha!  nice :-)

http://codereview.chromium.org/17246/diff/1/4#newcode105
Line 105: object->SetInternalField(V8Custom::kXMLHttpRequestCacheIndex, cache);
kWorkerRequestCacheIndex

http://codereview.chromium.org/17246/diff/1/9
File webkit/port/bindings/v8/WorkerScriptController.cpp (right):

http://codereview.chromium.org/17246/diff/1/9#newcode71
Line 71: if (!m_proxy.get()) {
I think a comment about what is getting stubbed out here and why might help.

http://codereview.chromium.org/17246/diff/1/9#newcode72
Line 72: static ChromeClient* dummyChromeClient = new EmptyChromeClient;
As statics, these all leak.  I'm not sure enough about these classes to know how
they behave when reused.  Should the just be members to the
WorkerScriptController instead?

http://codereview.chromium.org/17246/diff/1/9#newcode92
Line 92: v8::Locker locker;
Have we added locker code to the existing v8 bindings?

[Feng Qian, 2009-01-08 19:45:20]

How big is the change in third_party/WebKit/JavaScriptCore
/wtf/ThreadingWin.cpp?

If it is relatively small, can we use PLATFORM(CHROMIUM) macros?

http://codereview.chromium.org/17246/diff/1/12
File webkit/build/JavaScriptCore/SConscript (right):

http://codereview.chromium.org/17246/diff/1/12#newcode163
Line 163: '$WTF_DIR/ThreadSpecificWin.cpp',
Is ThreadSpecificWin.cpp a new file?

[jianli, 2009-01-08 21:38:25]

On 2009/01/08 19:45:20, Feng Qian wrote:
> How big is the change in third_party/WebKit/JavaScriptCore
> /wtf/ThreadingWin.cpp?
> 
> If it is relatively small, can we use PLATFORM(CHROMIUM) macros?

Unfortunately it is not that small. I am in talk with WebKit guys to see if we
can solve this problem more elegantly by introducing some kind of client
interface.
 
> 
> http://codereview.chromium.org/17246/diff/1/12
> File webkit/build/JavaScriptCore/SConscript (right):
> 
> http://codereview.chromium.org/17246/diff/1/12#newcode163
> Line 163: '$WTF_DIR/ThreadSpecificWin.cpp',
> Is ThreadSpecificWin.cpp a new file?

Yes, it has just landed into WebKit and will be pulled down in our next merge.

[jianli, 2009-01-08 22:54:16]

http://codereview.chromium.org/17246/diff/1/18
File base/scoped_handle_win.h (right):

http://codereview.chromium.org/17246/diff/1/18#newcode133
Line 133: #ifdef NOGDI
On 2009/01/08 18:44:31, Mike Belshe wrote:
> Should this just go into a different CL?  Not sure how its related to workers.

This change is needed because otherwise we will get the following error while
compiling port\bindings\v8\ThreadingWin.cpp. Some header file in
ThreadingWin.cpp includes scoped_handle_win.h which refers to DeleteDC function.
However, NOGDI is implied while building V8Bindings.

http://codereview.chromium.org/17246/diff/1/19
File base/thread.h (right):

http://codereview.chromium.org/17246/diff/1/19#newcode104
Line 104: // The thread ID.
On 2009/01/08 18:44:31, Mike Belshe wrote:
> This comment doesn't match the comment on line 136, that this is for debugging
> purposes.

Removed the incorrect comment on line 136.

http://codereview.chromium.org/17246/diff/1/10
File webkit/port/bindings/v8/ThreadingWin.cpp (right):

http://codereview.chromium.org/17246/diff/1/10#newcode43
Line 43: // Starting the code copied from
third_party/WebKit/JavaScriptCore/wtf/ThreadingWin.cpp
On 2009/01/08 18:44:31, Mike Belshe wrote:
> Given that the threading code below already uses base::, can we use the base::
> locking as well?  There is a condition variable in base too.

I rather keep the following code copied from WTF unchanged since I am going to
introduce threading client interface in WebKit such that we do not need to copy
this part. Does it sound OK to you?

http://codereview.chromium.org/17246/diff/1/10#newcode397
Line 397: int WebKitThreadManager::WaitForThreadCompletion(ThreadIdentifier
thread_id,
On 2009/01/08 18:44:31, Mike Belshe wrote:
> API nit:  perhaps this is out of our control, but usually this api is called
> "Join".

I agreed. However, WTF names it like that.

http://codereview.chromium.org/17246/diff/1/10#newcode399
Line 399: base::Thread* thread = NULL;
On 2009/01/08 18:44:31, Mike Belshe wrote:
> Should we remove from the list before calling join?  Or shoule we remove from
> the list after we've joined?

I think we should remove the thread from the list before calling join because we
do not want to join again if WaitForThreadCompletion() happens to be called
again from another thread before the first call ends.

http://codereview.chromium.org/17246/diff/1/10#newcode423
Line 423: // Nothing to do.
On 2009/01/08 18:44:31, Mike Belshe wrote:
> I would expect this would remove the thread from the known threads list?  (so
> that it doesn't get killed on cleanup and such?)

The WTF implementation of detachThread only closes thread handle. In addition,
detachThread seems not be referred in WebKit at all. I think we still want to
keep it in the list so that it can be teared down at exit.

http://codereview.chromium.org/17246/diff/1/4
File webkit/port/bindings/v8/V8WorkerCustom.cpp (right):

http://codereview.chromium.org/17246/diff/1/4#newcode48
Line 48: CALLBACK_FUNC_DECL(WorkerConstructor) {
On 2009/01/08 18:44:31, Mike Belshe wrote:
> Overall:  We need test cases for all of these.  We'll also need to run leak
> tests to make sure that we don't leak listeners.  Can you add layout tests? 
Or
> do they already exist?

Layout tests for workers have already been added in the WebKit side. Currently
building workers in Chrome is off by default. I will turn on building workers
and enable layout tests once I have this checked in and finish some remaining
work to make it fully functional, like make postMessage work.

http://codereview.chromium.org/17246/diff/1/4#newcode93
Line 93: // TODO(mbelshe) - merge these with XHR's CreateHiddenXHRDependency
On 2009/01/08 18:44:31, Mike Belshe wrote:
> Ha!  nice :-)

Copied from other place :)

http://codereview.chromium.org/17246/diff/1/4#newcode105
Line 105: object->SetInternalField(V8Custom::kXMLHttpRequestCacheIndex, cache);
On 2009/01/08 18:44:31, Mike Belshe wrote:
> kWorkerRequestCacheIndex

My bad. Forgot to change this. Fixed.

http://codereview.chromium.org/17246/diff/1/9
File webkit/port/bindings/v8/WorkerScriptController.cpp (right):

http://codereview.chromium.org/17246/diff/1/9#newcode71
Line 71: if (!m_proxy.get()) {
On 2009/01/08 18:44:31, Mike Belshe wrote:
> I think a comment about what is getting stubbed out here and why might help.

Added comment.

http://codereview.chromium.org/17246/diff/1/9#newcode72
Line 72: static ChromeClient* dummyChromeClient = new EmptyChromeClient;
On 2009/01/08 18:44:31, Mike Belshe wrote:
> As statics, these all leak.  I'm not sure enough about these classes to know
how
> they behave when reused.  Should the just be members to the
> WorkerScriptController instead?

Thanks for catching this. Changed to use scoped_ptr to avoid leaks.

http://codereview.chromium.org/17246/diff/1/9#newcode92
Line 92: v8::Locker locker;
On 2009/01/08 18:44:31, Mike Belshe wrote:
> Have we added locker code to the existing v8 bindings?

I was told that adding the lock as at the beginning of this function is enough.
Feng, can you confirm this?

[jianli, 2009-01-09 06:15:03]

As to the threading issue, I've found out a much simpler fix to make WebKit
thread work with our message loop. File port/binding/v8/ThreadingWin.cpp has
been replaced with port/binding/v8/Threading.cpp. This file Threading.cpp is
forked from JavascriptCore/wtf/Threading.cpp with very small change in
threadEntryPoint() to add our message loop hookup. Please review the latest
patch. Thanks.

[Feng Qian, 2009-01-14 17:52:55]

My biggest concern is that the WorkerScriptController uses V8Proxy::Evaluate, if
the worker does not have DOM accesses, it can use v8 api directly, so no need to
create dummy api, etc.

http://codereview.chromium.org/17246/diff/52/235
File webkit/port/bindings/v8/V8WorkerCustom.cpp (right):

http://codereview.chromium.org/17246/diff/52/235#newcode165
Line 165: proxy->FindOrCreateObjectEventListener(value, false);
Why the dependency to the old listener isn't removed? This is not your problem,
I saw other classes have the same issue. I might be the original author, but
re-looking at the code, the old hidden dependency should be removed.

What do you think?

http://codereview.chromium.org/17246/diff/52/240
File webkit/port/bindings/v8/WorkerScriptController.cpp (right):

http://codereview.chromium.org/17246/diff/52/240#newcode31
Line 31: 
#include "config.h" ???

http://codereview.chromium.org/17246/diff/52/240#newcode53
Line 53: , m_executionForbidden(false)
What's the workflow of m_executionForbidden, it seems can only go from false to
true, and can never revert back to false.

http://codereview.chromium.org/17246/diff/52/240#newcode72
Line 72: if (!m_proxy.get()) {
I feel there is no need to use V8Proxy::Evaluate here. Why not use v8 api to
compile and run script directly?

A worker context does not have DOM accesses, right?

http://codereview.chromium.org/17246/diff/52/239
File webkit/port/bindings/v8/WorkerScriptController.h (right):

http://codereview.chromium.org/17246/diff/52/239#newcode35
Line 35: class JSValue;
Why need JSC::JSValue here?

http://codereview.chromium.org/17246/diff/52/239#newcode38
Line 38: class V8Proxy;
Why V8Proxy does not have namespace WebCore?

http://codereview.chromium.org/17246/diff/52/239#newcode46
Line 46: class String;
Not used in declaration.

http://codereview.chromium.org/17246/diff/52/239#newcode61
Line 61: Mutex m_sharedDataMutex;
Where is Mutex from?

http://codereview.chromium.org/17246/diff/52/239#newcode66
Line 66: RefPtr<Frame> m_frame;
What is the points-to graph here? Need to make sure no cyclic ref count pointers
created here.

http://codereview.chromium.org/17246/diff/52/238
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/17246/diff/52/238#newcode1783
Line 1783: #endif  // ENABLE(WORKERS)
// WORKERS, make it consistent with previous change.

[jianli, 2009-01-14 18:52:30]

I've also fixed the handling of the first argument in WorkerConstructor of
V8WorkerCustom.cpp. This fix makes the layout test worker-constructor.html pass
(not turned on because I still need to do some work to make other worker related
layout test pass).

http://codereview.chromium.org/17246/diff/52/235
File webkit/port/bindings/v8/V8WorkerCustom.cpp (right):

http://codereview.chromium.org/17246/diff/52/235#newcode165
Line 165: proxy->FindOrCreateObjectEventListener(value, false);
On 2009/01/14 17:52:55, Feng Qian wrote:
> Why the dependency to the old listener isn't removed? This is not your
problem,
> I saw other classes have the same issue. I might be the original author, but
> re-looking at the code, the old hidden dependency should be removed.
> 
> What do you think?

Done.

http://codereview.chromium.org/17246/diff/52/240
File webkit/port/bindings/v8/WorkerScriptController.cpp (right):

http://codereview.chromium.org/17246/diff/52/240#newcode31
Line 31: 
On 2009/01/14 17:52:55, Feng Qian wrote:
> #include "config.h" ???

Done.

http://codereview.chromium.org/17246/diff/52/240#newcode53
Line 53: , m_executionForbidden(false)
On 2009/01/14 17:52:55, Feng Qian wrote:
> What's the workflow of m_executionForbidden, it seems can only go from false
to
> true, and can never revert back to false.

Yes, it only goes from false to true when WorkerThread::stop() (in WebKit) is
called to stop the running of the worker thread.

http://codereview.chromium.org/17246/diff/52/240#newcode72
Line 72: if (!m_proxy.get()) {
On 2009/01/14 17:52:55, Feng Qian wrote:
> I feel there is no need to use V8Proxy::Evaluate here. Why not use v8 api to
> compile and run script directly?
> 
I would like to use V8 api directly. Do you have any sample or document I can
refer to?

> A worker context does not have DOM accesses, right?

Yes, I have another CL that disables DOMWindow and enables WorkerContext as an
execution context. I am still fixing some issues for it.

http://codereview.chromium.org/17246/diff/52/239
File webkit/port/bindings/v8/WorkerScriptController.h (right):

http://codereview.chromium.org/17246/diff/52/239#newcode35
Line 35: class JSValue;
On 2009/01/14 17:52:55, Feng Qian wrote:
> Why need JSC::JSValue here?

Removed. This is the leftover from cleaning the one we forked from JSC side of
the file.

http://codereview.chromium.org/17246/diff/52/239#newcode38
Line 38: class V8Proxy;
On 2009/01/14 17:52:55, Feng Qian wrote:
> Why V8Proxy does not have namespace WebCore?

Removed it because I removed the use of V8Proxy in this header file.

http://codereview.chromium.org/17246/diff/52/239#newcode46
Line 46: class String;
On 2009/01/14 17:52:55, Feng Qian wrote:
> Not used in declaration.

Removed.

http://codereview.chromium.org/17246/diff/52/239#newcode61
Line 61: Mutex m_sharedDataMutex;
On 2009/01/14 17:52:55, Feng Qian wrote:
> Where is Mutex from?

Mutex is from WTF because this file is forked from JSC version. I could change
to use our lock if needed.

http://codereview.chromium.org/17246/diff/52/239#newcode66
Line 66: RefPtr<Frame> m_frame;
On 2009/01/14 17:52:55, Feng Qian wrote:
> What is the points-to graph here? Need to make sure no cyclic ref count
pointers
> created here.

The frame contains the page. I removed the creation of the new V8Proxy and used
the one in m_frame->script().

http://codereview.chromium.org/17246/diff/52/238
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/17246/diff/52/238#newcode1783
Line 1783: #endif  // ENABLE(WORKERS)
On 2009/01/14 17:52:55, Feng Qian wrote:
> // WORKERS, make it consistent with previous change.

Done.

[Feng Qian, 2009-01-14 19:44:41]

http://codereview.chromium.org/17246/diff/426/71
File webkit/port/bindings/v8/WorkerScriptController.cpp (right):

http://codereview.chromium.org/17246/diff/426/71#newcode65
Line 65: ScriptValue WorkerScriptController::evaluate(const ScriptSourceCode&
sourceCode)
The logic is similar to V8Proxy::RunScript, except that this function does not
need to keep a frame alive. Here is the scratch code:

v8::Local<v8::Script> script = V8Proxy::CompileScript(code, name, baseline);

// add recursion checking code here

v8::Local<v8::Value> result = script->Run();

if (V8Proxy::HandleOutOfMemory())
    return v8::Local<v8::Value>();

return result;

[Feng Qian, 2009-01-15 04:43:13]

It looks much better, with some small issues.

http://codereview.chromium.org/17246/diff/435/91
File webkit/port/bindings/v8/WorkerScriptController.cpp (right):

http://codereview.chromium.org/17246/diff/435/91#newcode37
Line 37: #include "Settings.h"
Settings.h seems not needed anymore.

http://codereview.chromium.org/17246/diff/435/91#newcode51
Line 51: WorkerScriptController::~WorkerScriptController()
If the caller does not call forbidExecution, whole context will leak. It is
safer to call Dispose here too. Or assert that m_context.IsEmpty().

http://codereview.chromium.org/17246/diff/435/91#newcode86
Line 86: if (m_context.IsEmpty()) {
How about rename InitContext to InitContextIfNeeded and move this logic into
InitContextIfNeeded?

http://codereview.chromium.org/17246/diff/435/91#newcode88
Line 88: m_context = v8::Context::New(NULL, global_template, m_global);
m_global is not needed since no need to preserve object identity, V8 will create
a new global object.

[jianli, 2009-01-15 05:03:10]

All fixed. Thanks.

http://codereview.chromium.org/17246/diff/435/91
File webkit/port/bindings/v8/WorkerScriptController.cpp (right):

http://codereview.chromium.org/17246/diff/435/91#newcode37
Line 37: #include "Settings.h"
On 2009/01/15 04:43:13, Feng Qian wrote:
> Settings.h seems not needed anymore.

Done.

http://codereview.chromium.org/17246/diff/435/91#newcode51
Line 51: WorkerScriptController::~WorkerScriptController()
On 2009/01/15 04:43:13, Feng Qian wrote:
> If the caller does not call forbidExecution, whole context will leak. It is
> safer to call Dispose here too. Or assert that m_context.IsEmpty().

Done.

http://codereview.chromium.org/17246/diff/435/91#newcode86
Line 86: if (m_context.IsEmpty()) {
On 2009/01/15 04:43:13, Feng Qian wrote:
> How about rename InitContext to InitContextIfNeeded and move this logic into
> InitContextIfNeeded?

Done.

http://codereview.chromium.org/17246/diff/435/91#newcode88
Line 88: m_context = v8::Context::New(NULL, global_template, m_global);
On 2009/01/15 04:43:13, Feng Qian wrote:
> m_global is not needed since no need to preserve object identity, V8 will
create
> a new global object.

Done.

[Feng Qian, 2009-01-15 17:26:59]

lgtm

Sorry for the delay.

On 2009/01/15 05:03:10, jianli wrote:
> All fixed. Thanks.
> 
> http://codereview.chromium.org/17246/diff/435/91
> File webkit/port/bindings/v8/WorkerScriptController.cpp (right):
> 
> http://codereview.chromium.org/17246/diff/435/91#newcode37
> Line 37: #include "Settings.h"
> On 2009/01/15 04:43:13, Feng Qian wrote:
> > Settings.h seems not needed anymore.
> 
> Done.
> 
> http://codereview.chromium.org/17246/diff/435/91#newcode51
> Line 51: WorkerScriptController::~WorkerScriptController()
> On 2009/01/15 04:43:13, Feng Qian wrote:
> > If the caller does not call forbidExecution, whole context will leak. It is
> > safer to call Dispose here too. Or assert that m_context.IsEmpty().
> 
> Done.
> 
> http://codereview.chromium.org/17246/diff/435/91#newcode86
> Line 86: if (m_context.IsEmpty()) {
> On 2009/01/15 04:43:13, Feng Qian wrote:
> > How about rename InitContext to InitContextIfNeeded and move this logic into
> > InitContextIfNeeded?
> 
> Done.
> 
> http://codereview.chromium.org/17246/diff/435/91#newcode88
> Line 88: m_context = v8::Context::New(NULL, global_template, m_global);
> On 2009/01/15 04:43:13, Feng Qian wrote:
> > m_global is not needed since no need to preserve object identity, V8 will
> create
> > a new global object.
> 
> Done.