[Webkit-unassigned] [Bug 57090] [V8] Possible data race problem in v8/WorkerScriptController (reported by tsan)

Fri Mar 25 11:52:16 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=57090

Dmitry Titov <dimich at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ap at webkit.org

--- Comment #4 from Dmitry Titov <dimich at chromium.org>  2011-03-25 11:52:16 PST ---
(In reply to comment #3)
> (In reply to comment #2)
> Looking at http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/WorkerScriptController.cpp, I don't understand why the mutex is ever necessary (by similar reasoning to the above).

Good point. The lock was grandfathered (euphemism for "just copied by me"), it was added by Alexey for JSC: http://trac.webkit.org/changeset/38689

However, termination code was changed and I think it is not needed now, at least in V8 case.

Here is how it works:
- V8::TerminateExecution() is invoked from any thread. It takes internal lock in V8 and sets a flag.
- On a worker thread, we can be either in v8 code already or in C++ WebCore code. The latter case may eventually enter V8 code as well. It's not a problem, since it's ok to enter the v8 code once after the termination flag was set
- V8 discovers a termination flag (after taking an internal lock) and raises the internal exception that unwinds us out of V8 code into C++ WebCore frame. If there are more v8 frames on the stack, the C++ code must check the result and if we have uncatchable exception unwinding (v8::TryCatch::CanContinue() == false), it should gracefully exit to allow the exception to unwind to the bottom. During this unwinding, V8 fast-returns with trivial results from all V8 APIs if they are called anyways.
- After we get out of the last V8 frame, it is not legal to call into V8 again. This is the only place where reentry into V8 is harmful. We should terminate worker then.

One change that makes sense here is to stop setting m_executionForbidden on a non-worker thread, and instead set it on worker thread, the moment we return from v8 frame unwinding termination exception. This will prevent us from re-entering v8 and make this flag set/examined from the single thread.

Removing r? flag from Kinuko's patch for now. Give me a day or two I'll upload updated one after running some tests and looking at the code, since threading is tricky :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.