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.
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?
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.
Failed again: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r56876%20(12228)/fast/workers/worker-terminate-pretty-diff.html
*** Bug 36585 has been marked as a duplicate of this bug. ***
Also causing fast/workers/termination-with-port-messages.html to fail, as seen in Bug 36585.
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.
Another victim: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r57011%20(12530)/fast/workers/termination-with-port-messages-diffs.txt
Again: http://build.webkit.org/builders/Leopard%20Intel%20Release%20(Tests)/builds/12559/steps/layout-test/logs/stdio
Again: http://build.webkit.org/builders/Leopard%20Intel%20Debug%20(Tests)/builds/12384/steps/layout-test/logs/stdio
Committed r57043: <http://trac.webkit.org/changeset/57043>
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.
termination-with-port-messages.html this time: http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/10385/steps/layout-test/logs/stdio
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.
Right. The correct solution looks to be the one proposed in Comment #2.
Created attachment 52523 [details] Patch
I'm not 100% happy with this patch, but it gets the job done (as far as I can tell). Suggestions welcome, as always. :)
Attachment 52523 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/1630248
Attachment 52523 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1677070
Attachment 52523 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/1651187
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
Created attachment 52549 [details] Patch
> 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).
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.
CCing the real JSC masters in case they spot something wrong that I've missed.
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.
Comment on attachment 52549 [details] Patch Clearing flags on attachment: 52549 Committed r57192: <http://trac.webkit.org/changeset/57192>
All reviewed patches have been landed. Closing bug.