Bug 112306

Summary:

Fix flaky layout tests that rely on setTimeout()s firing after <body> is parsed

Product:

WebKit

Reporter:

James Robinson <jamesr>

Component:

New Bugs

Assignee:

James Robinson <jamesr>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, darin, dbates, eric, tonyg, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch for landing	none
Patch for landing	webkit.review.bot: commit-queue-

James Robinson

Reported 2013-03-13 18:02:36 PDT

Fix flaky layout tests that rely on setTimeout()s firing after <body> is parsed

Attachments
Patch (15.65 KB, patch) 2013-03-13 18:06 PDT, James Robinson	no flags	Details Formatted Diff Diff
Patch for landing (15.62 KB, patch) 2013-03-13 18:14 PDT, James Robinson	no flags	Details Formatted Diff Diff
Patch for landing (17.28 KB, patch) 2013-03-14 11:27 PDT, James Robinson	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

James Robinson

Comment 1 2013-03-13 18:06:07 PDT

Created attachment 193033 [details] Patch

James Robinson

Comment 2 2013-03-13 18:09:27 PDT

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

Eric Seidel (no email)

Comment 3 2013-03-13 18:09:38 PDT

Comment on attachment 193033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193033&action=review LGTM, I suspect you'll see one failure from this. > LayoutTests/fast/block/float/floats-not-cleared-crash.html:9 > +window.onload = crash; > function crash() This doesn't look right. crash isn't defined yet. :)

James Robinson

Comment 4 2013-03-13 18:12:19 PDT

James Robinson

Comment 5 2013-03-13 18:12:34 PDT

(In reply to comment #3) > (From update of attachment 193033 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193033&action=review > > LGTM, I suspect you'll see one failure from this. > > > LayoutTests/fast/block/float/floats-not-cleared-crash.html:9 > > +window.onload = crash; > > function crash() > > This doesn't look right. crash isn't defined yet. :) You forget this is JavaScript where craziness like this works A-OK :)

James Robinson

Comment 6 2013-03-13 18:14:07 PDT

Created attachment 193034 [details] Patch for landing

Adam Barth

Comment 7 2013-03-13 18:14:23 PDT

Comment on attachment 193033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193033&action=review >>> LayoutTests/fast/block/float/floats-not-cleared-crash.html:9 >>> function crash() >> >> This doesn't look right. crash isn't defined yet. :) > > You forget this is JavaScript where craziness like this works A-OK :) THIS IS JAVASCRIPT

WebKit Review Bot

Comment 8 2013-03-13 18:19:52 PDT

Comment on attachment 193034 [details] Patch for landing Rejecting attachment 193034 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 193034, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: fast/innerHTML/innerHTML-iframe.html patching file LayoutTests/fast/js/same-origin-subframe-about-blank.html patching file LayoutTests/fast/multicol/span/removal-of-multicol-span-crash.html patching file LayoutTests/fast/text/international/bidi-neutral-in-mixed-direction-run-crash.html patching file LayoutTests/fast/writing-mode/overhanging-float-legend-crash.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://webkit-commit-queue.appspot.com/results/17181210

Alexey Proskuryakov

Comment 9 2013-03-14 10:11:34 PDT

Comment on attachment 193034 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=193034&action=review Another nice catch! > LayoutTests/fast/encoding/script-in-head.html:8 > - 'setTimeout(function () {' + > + 'window.onload = function () {' + This changes how the test works - previously, the script normally ran after onload, not during it. A less invasive fix would be to start a zero-delay timer in onload. Or if the test is changed as currently proposed, then it doesn't need to be async, waitUntilDone/notifyDone can be removed. Either of these changes probably needs to be done to most of the tests fixed here, I didn't check each one.

James Robinson

Comment 10 2013-03-14 10:28:08 PDT

(In reply to comment #9) > (From update of attachment 193034 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193034&action=review > > Another nice catch! > > > LayoutTests/fast/encoding/script-in-head.html:8 > > - 'setTimeout(function () {' + > > + 'window.onload = function () {' + > > This changes how the test works - previously, the script normally ran after onload, not during it. A less invasive fix would be to start a zero-delay timer in onload. Yes, it's a change. Reading the test, it won't make a difference in the test coverage. > > Or if the test is changed as currently proposed, then it doesn't need to be async, waitUntilDone/notifyDone can be removed. That's a good catch. I removed the waitUntilDone/notifyDone pairs from most tests, missed this one. Will check the rest. > > Either of these changes probably needs to be done to most of the tests fixed here, I didn't check each one. I did check each one to see if it's sensitive to setTimeout or just needed to run after the rest of the test was parsed. I've left tests alone that are specifically about setTimeout timing (i.e. some security ones that intentionally set up races to make sure we don't crash).

James Robinson

Comment 11 2013-03-14 11:27:10 PDT

Created attachment 193158 [details] Patch for landing

WebKit Review Bot

Comment 12 2013-03-14 11:30:04 PDT

Comment on attachment 193158 [details] Patch for landing Rejecting attachment 193158 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 193158, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: fast/innerHTML/innerHTML-iframe.html patching file LayoutTests/fast/js/same-origin-subframe-about-blank.html patching file LayoutTests/fast/multicol/span/removal-of-multicol-span-crash.html patching file LayoutTests/fast/text/international/bidi-neutral-in-mixed-direction-run-crash.html patching file LayoutTests/fast/writing-mode/overhanging-float-legend-crash.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://webkit-commit-queue.appspot.com/results/17024990

James Robinson

Comment 13 2013-03-14 12:24:29 PDT

(In reply to comment #12) > (From update of attachment 193158 [details]) > Rejecting attachment 193158 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 193158, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue > > Last 500 characters of output: > fast/innerHTML/innerHTML-iframe.html > patching file LayoutTests/fast/js/same-origin-subframe-about-blank.html > patching file LayoutTests/fast/multicol/span/removal-of-multicol-span-crash.html > patching file LayoutTests/fast/text/international/bidi-neutral-in-mixed-direction-run-crash.html > patching file LayoutTests/fast/writing-mode/overhanging-float-legend-crash.html > > Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue > > Full output: http://webkit-commit-queue.appspot.com/results/17024990 Hmm, I think svn-apply may be barfing on the CRLF files.

James Robinson

Comment 14 2013-03-14 17:04:38 PDT

Committed r145857: <http://trac.webkit.org/changeset/145857>

Daniel Bates

Comment 15 2013-03-19 12:19:46 PDT

(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 193158 [details] [details]) > > Rejecting attachment 193158 [details] [details] from commit-queue. > > >> [...] > > > > Full output: http://webkit-commit-queue.appspot.com/results/17024990 > > Hmm, I think svn-apply may be barfing on the CRLF files. Filed bug 112732 for this issue.

Note You need to log in before you can comment on or make changes to this bug.