Bug 36239

Summary: [v8] Avoid reentry into v8 after TerminateExecution() on a worker thread.
Product: WebKit Reporter: Dmitry Titov <dimich>
Component: WebCore JavaScriptAssignee: Dmitry Titov <dimich>
Status: RESOLVED FIXED    
Severity: Normal CC: antonm, atwilson, dglazkov, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 36241    
Bug Blocks:    
Attachments:
Description Flags
Patch. dglazkov: review+, dimich: commit-queue-

Description Dmitry Titov 2010-03-17 13:03:41 PDT
V8 will crash or return Null values if used after v8::V8::TerminateExecution is invoked. We use it to terminate a worker. That exits JS stack but if the C++ code tries to execute JS again, crashes happen.

For example, when worker has a MessagePort and receives a lot of messages, the MessagePort::dispatchMessages() will loop over accumulated messages and fire onmessage event. There are multiple places like that. So some sort of check for possible continuation should be made.

The v8 exposes a instance-wide flag, IsExecutionTerminating(), and we have a flag in WorkerScritpController that indicates we have called TerminateExecution. I've looked over the code and realized that adding yet another check to various existing ones (valid proxy, canExecuteScripts, IsPaused etc) would only complicate things. Instead, it seems most JS execution requires to obtain a v8 context, in the form of WorkerContextExecutionProxy - and most places already check for it being 0 and bail out. So I've added a check for termination into WorkerScriptController::proxy() accessor and fixed up couple of places that didn't have check for 0. this way, we can stop v8 reentry w/o additional check sprinkled around.

Patch coming.
Comment 1 Dmitry Titov 2010-03-17 13:14:30 PDT
Created attachment 50947 [details]
Patch.
Comment 2 WebKit Review Bot 2010-03-17 13:18:58 PDT
Attachment 50947 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/845051
Comment 3 Dmitry Titov 2010-03-17 13:23:17 PDT
Please ignore Chromium EWS build failure - it is using older v8 revision. I'll
submit updated DEPS for it before landing this patch, to avoid chromium build
bot break.

Separately, this patch went through chromium try bots on 3 platforms with
green.
Comment 4 Dimitri Glazkov (Google) 2010-03-17 13:55:25 PDT
Comment on attachment 50947 [details]
Patch.

ok.
Comment 5 Dmitry Titov 2010-03-17 15:39:02 PDT
Landed: http://trac.webkit.org/changeset/56131