Bug 36646 – REGRESSION: Worker termination via JS timeout may cause worker tests like fast/workers/worker-terminate.html fail.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

fast/workers/worker-terminate.html failed (timeout) on Leopard Debug Bot

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r56607%20(11979)/fast/workers/worker-terminate-diffs.txt
--- /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug-tests/build/layout-test-results/fast/workers/worker-terminate-expected.txt	2010-03-26 00:28:17.000000000 -0700
+++ /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug-tests/build/layout-test-results/fast/workers/worker-terminate-actual.txt	2010-03-26 00:28:17.000000000 -0700
@@ -1,3 +1,4 @@
+CONSOLE MESSAGE: line 3: JavaScript execution exceeded timeout.
 Test Worker.terminate().
 
 Received message from worker4.

Test is from 9 months ago:
http://trac.webkit.org/browser/trunk/LayoutTests/fast/workers/worker-terminate.html

I think we've seen several other failures related to "JavaScript execution exceeded timeout."  They seem to be recent.

[Dmitry Titov]

For JSC, workers are terminated like this:

void WorkerScriptController::forbidExecution()
{
   ...
    m_globalData->timeoutChecker.setTimeoutInterval(1); // 1ms is the smallest timeout that can be set.
}

So it seems the timeout exceeded coudl be a normal way for a worker to die. Perhaps we should prevent the console message in this case?

[Dmitry Titov]

Note from Darin Adler in webkit-dev: 
"There is precedent in JavaScriptCore for using a special exception to terminate code. InterruptedExecutionError is used for this purpose. I could imagine having something similar for workers. A timeout sounds like a poor way to do it."

Indeed, this could be better. For workers to be able to use it, it should be possible to inject it from another thread.

[Adam Barth]

Failed again:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r56876%20(12228)/fast/workers/worker-terminate-pretty-diff.html

[Eric Seidel (no email)]

*** Bug 36585 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Also causing fast/workers/termination-with-port-messages.html to fail, as seen in Bug 36585.

[Eric Seidel (no email)]

I think we should be able to add the Snow Leopard bots to the core builders once this bug is fixed.  This causes about one failure ever 5 builds or so right now.

[Eric Seidel (no email)]

Another victim:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r57011%20(12530)/fast/workers/termination-with-port-messages-diffs.txt

[Adam Barth]

Again: http://build.webkit.org/builders/Leopard%20Intel%20Release%20(Tests)/builds/12559/steps/layout-test/logs/stdio

[Adam Barth]

Again:
http://build.webkit.org/builders/Leopard%20Intel%20Debug%20(Tests)/builds/12384/steps/layout-test/logs/stdio

[Adam Barth]

Committed r57043: <http://trac.webkit.org/changeset/57043>

[Adam Barth]

I've skipped this test because the failure rate is obscenely high.  That doesn't mean we should drop this test.  I'd like it back, but we can't have tests randomly failing on all the bots 25% of the time.

[Adam Barth]

termination-with-port-messages.html this time:
http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/10385/steps/layout-test/logs/stdio

[Eric Seidel (no email)]

Another one:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r57062%20(12579)/fast/workers/termination-with-port-messages-diffs.txt

I don't think skipping any single test will solve this problem.

However, this seems to be our #1 cause of flakiness at the moment.

[Adam Barth]

Right.  The correct solution looks to be the one proposed in Comment #2.

[Adam Barth]

Created attachment 52523 [details]
Patch

[Adam Barth]

I'm not 100% happy with this patch, but it gets the job done (as far as I can tell).  Suggestions welcome, as always.  :)

[Eric Seidel (no email)]

Attachment 52523 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1630248

[WebKit Review Bot]

Attachment 52523 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1677070

[Early Warning System Bot]

Attachment 52523 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1651187

[Darin Adler]

Comment on attachment 52523 [details]
Patch

> +        (JSC::Terminator::termianteSoon):

I took a brief look, and did not review everything -- I hope someone more expert than me at JavaScriptCore will review -- but I did spot a spelling error in the name of this function.

We should s/ermiant/erminat/g

[Adam Barth]

Created attachment 52549 [details]
Patch

[Adam Barth]

> We should s/ermiant/erminat/g

Thanks for looking.  I was vacillating on the names until the last minute (hence the build errors and misspellings).

[Eric Seidel (no email)]

Comment on attachment 52549 [details]
Patch

This looks sane to me, but I am not much of a JavaScriptCore master these days, so I may have missed something subtle.

The CHECK_FOR_TIMEOUT() change and DEFINE_STUB_FUNCTION(int, timeout_check)
change may require a sunspider run, however it looks like all of this code is "slow-path", only run after "tickCount" iterations, or in the case of actual failures, and thus should not affect performance.

[Eric Seidel (no email)]

CCing the real JSC masters in case they spot something wrong that I've missed.

[Eric Seidel (no email)]

Comment on attachment 52549 [details]
Patch

This looks good to me.  As far as I can tell these are all slow-paths, so this should have not affect on performance.

[WebKit Commit Bot]

Comment on attachment 52549 [details]
Patch

Clearing flags on attachment: 52549

Committed r57192: <http://trac.webkit.org/changeset/57192>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.