WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
36646
REGRESSION: Worker termination via JS timeout may cause worker tests like fast/workers/worker-terminate.html fail.
https://bugs.webkit.org/show_bug.cgi?id=36646
Summary
REGRESSION: Worker termination via JS timeout may cause worker tests like fas...
Eric Seidel (no email)
Reported
2010-03-26 00:43:14 PDT
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.
Attachments
Patch
(24.66 KB, patch)
2010-04-05 03:26 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Patch
(24.64 KB, patch)
2010-04-05 11:36 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Dmitry Titov
Comment 1
2010-03-29 17:16:28 PDT
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
Comment 2
2010-03-29 17:46:30 PDT
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
Comment 3
2010-03-31 17:17:14 PDT
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)
Comment 4
2010-03-31 22:50:42 PDT
***
Bug 36585
has been marked as a duplicate of this bug. ***
Eric Seidel (no email)
Comment 5
2010-03-31 22:51:33 PDT
Also causing fast/workers/termination-with-port-messages.html to fail, as seen in
Bug 36585
.
Eric Seidel (no email)
Comment 6
2010-04-01 20:47:27 PDT
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)
Comment 7
2010-04-02 11:36:53 PDT
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
Comment 8
2010-04-02 22:03:17 PDT
Again:
http://build.webkit.org/builders/Leopard%20Intel%20Release%20(Tests)/builds/12559/steps/layout-test/logs/stdio
Adam Barth
Comment 9
2010-04-02 22:42:38 PDT
Again:
http://build.webkit.org/builders/Leopard%20Intel%20Debug%20(Tests)/builds/12384/steps/layout-test/logs/stdio
Adam Barth
Comment 10
2010-04-02 22:49:38 PDT
Committed
r57043
: <
http://trac.webkit.org/changeset/57043
>
Adam Barth
Comment 11
2010-04-02 22:50:31 PDT
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
Comment 12
2010-04-03 12:09:51 PDT
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)
Comment 13
2010-04-05 00:38:12 PDT
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
Comment 14
2010-04-05 00:42:38 PDT
Right. The correct solution looks to be the one proposed in
Comment #2
.
Adam Barth
Comment 15
2010-04-05 03:26:33 PDT
Created
attachment 52523
[details]
Patch
Adam Barth
Comment 16
2010-04-05 03:27:19 PDT
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)
Comment 17
2010-04-05 03:40:41 PDT
Attachment 52523
[details]
did not build on mac: Build output:
http://webkit-commit-queue.appspot.com/results/1630248
WebKit Review Bot
Comment 18
2010-04-05 03:57:46 PDT
Attachment 52523
[details]
did not build on gtk: Build output:
http://webkit-commit-queue.appspot.com/results/1677070
Early Warning System Bot
Comment 19
2010-04-05 08:23:41 PDT
Attachment 52523
[details]
did not build on qt: Build output:
http://webkit-commit-queue.appspot.com/results/1651187
Darin Adler
Comment 20
2010-04-05 08:38:22 PDT
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
Comment 21
2010-04-05 11:36:39 PDT
Created
attachment 52549
[details]
Patch
Adam Barth
Comment 22
2010-04-05 11:41:37 PDT
> 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 23
2010-04-05 17:30:23 PDT
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)
Comment 24
2010-04-05 17:31:05 PDT
CCing the real JSC masters in case they spot something wrong that I've missed.
Eric Seidel (no email)
Comment 25
2010-04-06 19:10:07 PDT
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 26
2010-04-06 21:38:31 PDT
Comment on
attachment 52549
[details]
Patch Clearing flags on attachment: 52549 Committed
r57192
: <
http://trac.webkit.org/changeset/57192
>
WebKit Commit Bot
Comment 27
2010-04-06 21:38:39 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug