Bug 36239 - [v8] Avoid reentry into v8 after TerminateExecution() on a worker thread.
Summary: [v8] Avoid reentry into v8 after TerminateExecution() on a worker thread.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dmitry Titov
URL:
Keywords:
Depends on: 36241
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-17 13:03 PDT by Dmitry Titov
Modified: 2010-03-17 15:39 PDT (History)
5 users (show)

See Also:


Attachments
Patch. (7.25 KB, patch)
2010-03-17 13:14 PDT, Dmitry Titov
dglazkov: review+
dimich: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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