Bug 35137

Summary: Chromium: Use V8::TerminateExecution to actually terminate workers.
Product: WebKit Reporter: Dmitry Titov <dimich>
Component: WebCore JavaScriptAssignee: Dmitry Titov <dimich>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, atwilson, commit-queue, dglazkov, eric, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch.
commit-queue: commit-queue-
Updated patch. levin: review+, dimich: commit-queue-

Description Dmitry Titov 2010-02-18 19:33:01 PST
Uncooperative workers may run for a long time after being closed or terminated. The chromium has a special timer that will kill the worker process but it's causing crashes on process destruction which triggers ReportCrash on Mac and may leave zombie processes behind. Need to actually stop the JS execution.

Patch will follow soon.
Comment 1 Dmitry Titov 2010-02-18 19:43:06 PST
Created attachment 49054 [details]
Patch.

This will fix the existing test fast/workers/stress-js-execution.html which is currently crashes on Chromium Mac and Linux bots and is disabled.
Comment 2 WebKit Commit Bot 2010-02-18 21:33:14 PST
Comment on attachment 49054 [details]
Patch.

Rejecting patch 49054 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
	M	WebCore/ChangeLog
	M	WebCore/bindings/v8/WorkerContextExecutionProxy.cpp
	M	WebCore/bindings/v8/WorkerScriptController.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 558


Full output: http://webkit-commit-queue.appspot.com/results/289144
Comment 3 Dmitry Titov 2010-02-19 08:13:28 PST
Oops indeed. Set cq+ by mistake. Should commit-queue try to land patch without r+? Anyways, cq- now .
Comment 4 Andrew Wilson 2010-02-19 09:10:34 PST
Per our email discussion, I'm concerned that many of our uses of v8::TryCatch will now be incorrect once we start using this. We should double-check that with Mads before landing.
Comment 5 Eric Seidel (no email) 2010-02-19 09:22:51 PST
(In reply to comment #3)
> Oops indeed. Set cq+ by mistake. Should commit-queue try to land patch without
> r+? Anyways, cq- now .

Yup.  We added this feature intentionally a while ago.  It allows committers to queue their own patches for commit w/o requiring a second round of approval from a reviewer for instance.

Adam Barth built a "land-safely" command around this (at least I think he did) which should be in webkit-patch.
Comment 6 Dmitry Titov 2010-02-19 10:53:07 PST
Comment on attachment 49054 [details]
Patch.

removing the r? to address concern brought by Drew.
Comment 7 Dmitry Titov 2010-02-23 16:07:37 PST
Created attachment 49335 [details]
Updated patch.

Updated patch as per email discussion with Mads Ager and Drew Wilson.
The way to check that v8 has returned from JS block because of TerminateExecution() is to check TryCatch::CanContinue(). Added this check.
The v8 will unwind JS frames upon return from C++ frames because termination exception is 'sticky'.

Independently from this patch, V8 team plans to look into a generic issue of possible re-entering V8 while termination is in effect. It is unclear if we even have this scenario in today's worker code.
Comment 8 Dmitry Titov 2010-02-23 18:06:56 PST
Landed: http://trac.webkit.org/changeset/55180