Bug 35137 – Chromium: Use V8::TerminateExecution to actually terminate workers.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dmitry Titov]

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.

[Dmitry Titov]

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.

[WebKit Commit Bot]

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

[Dmitry Titov]

Oops indeed. Set cq+ by mistake. Should commit-queue try to land patch without r+? Anyways, cq- now .

[Andrew Wilson]

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.

[Eric Seidel (no email)]

(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.

[Dmitry Titov]

Comment on attachment 49054 [details]
Patch.

removing the r? to address concern brought by Drew.

[Dmitry Titov]

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.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/55180