Bug 54942

Summary:

In layout tests, iframe's onload handler should not use script function defined after the iframe

Product:

WebKit

Reporter:

Xianzhu Wang <wangxianzhu>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, commit-queue, eric, japhet, jnd, mihaip, tonyg, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

54355

Attachments:

Description	Flags
patch (only applies to this issue)	none
path to fix more tests	tonyg: review+, commit-queue: commit-queue-
patch added missing platform/mac/fast/loader/non-html-load-event-expected.txt	commit-queue: commit-queue-
Patch adding more missing new expected files	none

Xianzhu Wang

Reported 2011-02-22 00:37:40 PST

Some layout tests (e.g. fast/dom/wrapper-classes.html) contain code like the following: <iframe onload="frameLoaded()" src="data:.."></iframe> <script> .... function frameLoaded() { ...... } </script> It's possible that the iframe's onload handler is called before the later script element is executed, causing the tests to fail.

Attachments
patch (only applies to this issue) (10.16 KB, patch) 2011-02-24 01:21 PST, Xianzhu Wang	no flags	Details Formatted Diff Diff
path to fix more tests (33.65 KB, patch) 2011-02-25 03:52 PST, Xianzhu Wang	tonyg: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
patch added missing platform/mac/fast/loader/non-html-load-event-expected.txt (34.06 KB, patch) 2011-03-01 02:25 PST, Xianzhu Wang	commit-queue: commit-queue-	Details Formatted Diff Diff
Patch adding more missing new expected files (36.16 KB, patch) 2011-03-01 14:38 PST, Xianzhu Wang	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Tony Gentilcore

Comment 1 2011-02-22 11:15:17 PST

There's a larger problem that a lot of layout tests aren't resilient to parser yields. This is for a variety of reasons including the one you mention. Technically speaking, the parser can yield to service the message loop between any two tokens. Typically this doesn't happen in layout tests since tests are usually small and the parser doesn't explicitly yield until 500ms and 4,096 tokens go by. However a smaller than usual disk or network read could have the effect of sporadically introducing yields which the tests didn't expect. One way to find all the tests which aren't resilient to parser yields is to run tests with the yielding constants in HTMLParserScheduler set so that it yields between every token: static const int defaultParserChunkSize = 1; static const double defaultParserTimeLimit = 0.000; I'll try to get a list and we can try to tackle them together?

Xianzhu Wang

Comment 2 2011-02-22 17:40:21 PST

> I'll try to get a list and we can try to tackle them together? OK. Thanks Tony. I'll also try to get a list in my chromium-linux environment and we can compare the lists.

Xianzhu Wang

Comment 3 2011-02-23 22:12:37 PST

I ran all layout tests on chromium-linux with parser yielding enabled, and there were 37 new timeouts (3 flaky), 1 different crash, 216 new text mismatches (6 flaky). I tried to analyze some of them, and found that though the reasons of some new failures are obvious, many of them need time to figure out. I suggest to handle the failures step by step. The first step is to fix all failures with the reason described in this issue and close this issue, and then the others.

Xianzhu Wang

Comment 4 2011-02-24 01:21:53 PST

Created attachment 83617 [details] patch (only applies to this issue)

Eric Seidel (no email)

Comment 5 2011-02-24 02:25:41 PST

Comment on attachment 83617 [details] patch (only applies to this issue) OK.

Tony Gentilcore

Comment 6 2011-02-24 09:05:43 PST

(In reply to comment #3) > I tried to analyze some of them, and found that though the reasons of some new failures are obvious, many of them need time to figure out. Yeah, there are lots of things going on. Another really common theme I'm finding is this: <body onload="alert('body')" <iframe onload="alert('frame')" src="data:text/html,synchronous"></iframe> </body> If the parser never yields (which many tests assume), the body's onload will fire before the iframe's onload (note that this isn't compliant w/ the spec). However, if there are any yields then the iframe's onload will properly fire before the body's. This causes some tests to hang if they have notifyDone in the wrong place. Other tests appear to fail because the dump is taken earlier than the test expects, but if you view the test manually, it is clear that it still passes.

WebKit Commit Bot

Comment 7 2011-02-24 14:41:14 PST

Comment on attachment 83617 [details] patch (only applies to this issue) Clearing flags on attachment: 83617 Committed r79625: <http://trac.webkit.org/changeset/79625>

WebKit Commit Bot

Comment 8 2011-02-24 14:41:19 PST

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 9 2011-02-24 16:07:45 PST

http://trac.webkit.org/changeset/79625 might have broken GTK Linux 64-bit Debug

Xianzhu Wang

Comment 10 2011-02-25 03:51:36 PST

The first patch was for the layout tests found problems with parser yielding. However, I found more problematic tests using text grep. I tried to let DumpRenderTree access these tests through http:// instead of file://, and some tests did fail because of the problem.

Xianzhu Wang

Comment 11 2011-02-25 03:52:22 PST

Created attachment 83790 [details] path to fix more tests

Tony Gentilcore

Comment 12 2011-02-25 08:58:41 PST

Comment on attachment 83790 [details] path to fix more tests This is awesome work! A couple of questions below. This patch is large and I want to make sure we don't accidentally change any tests so that they don't test what they were originally designed to test. View in context: https://bugs.webkit.org/attachment.cgi?id=83790&action=review > LayoutTests/editing/execCommand/find-after-replace.html:7 > +<iframe src="../resources/contenteditable-iframe-src.html"></iframe> Rather than moving the runTest to the body's onload, why not just move the script block above the iframe? Same question on a few other tests. > LayoutTests/fast/frames/viewsource-plain-text-tags.html:36 > +<hr> This used to be <hr><iframe><hr><div> and now it is <hr><div><hr><iframe>. I'm surprised there is no change in the expectations. Does it really pass? Perhaps it is disabled on the port where you ran tests?

Xianzhu Wang

Comment 13 2011-02-26 18:10:24 PST

Comment on attachment 83790 [details] path to fix more tests View in context: https://bugs.webkit.org/attachment.cgi?id=83790&action=review >> LayoutTests/editing/execCommand/find-after-replace.html:7 >> +<iframe src="../resources/contenteditable-iframe-src.html"></iframe> > > Rather than moving the runTest to the body's onload, why not just move the script block above the iframe? Same question on a few other tests. This is to avoid expectation changes, especially for the tests that have platform rebaselines. >> LayoutTests/fast/frames/viewsource-plain-text-tags.html:36 >> +<hr> > > This used to be <hr><iframe><hr><div> and now it is <hr><div><hr><iframe>. I'm surprised there is no change in the expectations. Does it really pass? Perhaps it is disabled on the port where you ran tests? I was also surprised then I found this test just overwrites the whole text result with "document.open(); document.write(resultText); document.close()", so the text dump isn't affected by change of sequence of the div, hr and iframe.

Xianzhu Wang

Comment 14 2011-02-26 18:19:53 PST

(In reply to comment #13) > (From update of attachment 83790 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83790&action=review > > >> LayoutTests/editing/execCommand/find-after-replace.html:7 > >> +<iframe src="../resources/contenteditable-iframe-src.html"></iframe> > > > > Rather than moving the runTest to the body's onload, why not just move the script block above the iframe? Same question on a few other tests. > > This is to avoid expectation changes, especially for the tests that have platform rebaselines. > P.S. more details: The tests contain code like the following: <iframe src="... onload="test()"></iframe> <div id="console"></div> <script>function test() { ... getElementById("console")... } </script> Because the "console" div is used in the test function which is possibly not ready when iframe onload is executed, it should be moved above the iframe, which will cause text expectation change. Moving the test() function from iframe onload to body onload can avoid the change. A few tests depend on the behavior of iframe onload, so their expectation changes are unavoidable.

Xianzhu Wang

Comment 15 2011-02-27 17:35:25 PST

Comment on attachment 83790 [details] path to fix more tests View in context: https://bugs.webkit.org/attachment.cgi?id=83790&action=review >>>> LayoutTests/editing/execCommand/find-after-replace.html:7 >>> >>> Rather than moving the runTest to the body's onload, why not just move the script block above the iframe? Same question on a few other tests. >> >> This is to avoid expectation changes, especially for the tests that have platform rebaselines. > > P.S. more details: > > The tests contain code like the following: > > <iframe src="... onload="test()"></iframe> > <div id="console"></div> > <script>function test() { ... getElementById("console")... } </script> > > Because the "console" div is used in the test function which is possibly not ready when iframe onload is executed, it should be moved above the iframe, which will cause text expectation change. Moving the test() function from iframe onload to body onload can avoid the change. > > A few tests depend on the behavior of iframe onload, so their expectation changes are unavoidable. Sorry. The above P.S. are for other tests that moved onload handler from iframe to body. For this particular test, it dumps render tree, so if I move the script block above the iframe, there will be a render tree change about an invisible text block. Moving the onload from iframe to body can avoid the change.

Tony Gentilcore

Comment 16 2011-02-28 10:35:24 PST

Comment on attachment 83790 [details] path to fix more tests Thanks again for working on these. I went through the tests as carefully as I could and can't find anything that looks like it might be changing the intent of the test. For future reference, if you have more patches in this area, please open a separate bug for each. If you want to tie them all together, you can have a master meta bug and mark each bug with your patches as blocking that master bug.

WebKit Commit Bot

Comment 17 2011-02-28 13:08:25 PST

Comment on attachment 83790 [details] path to fix more tests Rejecting attachment 83790 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2 Last 500 characters of output: leScript ... platform/mac/fast/canvas . platform/mac/fast/css . platform/mac/fast/dom .... platform/mac/fast/dom/HTMLImageElement . platform/mac/fast/encoding . platform/mac/fast/events ... platform/mac/fast/forms ....... platform/mac/fast/loader .... platform/mac/fast/loader/non-html-load-event.html -> failed Exiting early after 1 failures. 13312 tests run. 390.04s total testing time 13311 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 11 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8073417

Xianzhu Wang

Comment 18 2011-03-01 02:25:04 PST

Created attachment 84202 [details] patch added missing platform/mac/fast/loader/non-html-load-event-expected.txt Added missing platform/mac/fast/loader/non-html-load-event-expected.txt. Try commit queue again.

WebKit Review Bot

Comment 19 2011-03-01 02:25:40 PST

Comment on attachment 84202 [details] patch added missing platform/mac/fast/loader/non-html-load-event-expected.txt Rejecting attachment 84202 [details] from commit-queue. wangxianzhu@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Johnny(Jianning) Ding

Comment 20 2011-03-01 02:43:01 PST

Comment on attachment 84202 [details] patch added missing platform/mac/fast/loader/non-html-load-event-expected.txt According to Xianzhu's request, mark c+ on the new patch.

WebKit Commit Bot

Comment 21 2011-03-01 06:42:30 PST

Comment on attachment 84202 [details] patch added missing platform/mac/fast/loader/non-html-load-event-expected.txt Rejecting attachment 84202 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: ...................................................................................... http/tests/multipart ..... http/tests/navigation .................................................................................................... http/tests/plugins ....... http/tests/plugins/post-url-file.html -> failed Exiting early after 1 failures. 22133 tests run. 615.05s total testing time 22132 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 13 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8078046

Xianzhu Wang

Comment 22 2011-03-01 14:38:48 PST

Created attachment 84296 [details] Patch adding more missing new expected files Sorry for yesterday's hurry and incomplete patch. I believe this one should be complete. Added: http/tests/plugins/post-url-file-expected.txt http/tests/security/listener/xss-inactive-closure-expected.txt http/tests/security/postMessage/invalid-origin-throws-exception-expected.txt They are all about changes of positions of blank lines, and all the related tests don't have platform-specific rebaselines, while they are all expected fail on chromium-linux which was the reason that my script missed them.

Darin Adler

Comment 23 2011-03-01 14:50:16 PST

Comment on attachment 84296 [details] Patch adding more missing new expected files View in context: https://bugs.webkit.org/attachment.cgi?id=84296&action=review > LayoutTests/editing/execCommand/find-after-replace.html:7 > +<body onload="runTest()"> > <script> > if (window.layoutTestController) > layoutTestController.dumpEditingCallbacks(); > </script> > <p>This tests find and replace inside an editable iframe. You should see 'A B A B' below. With bug 4462420, you would see 'A B B A'.</p> > -<iframe onload="runTest()" src="../resources/contenteditable-iframe-src.html"></iframe> > +<iframe src="../resources/contenteditable-iframe-src.html"></iframe> This can’t be right. It’s possible the top level document will finish loading before its subframes, right? I think we need to make these tests run when both have loaded. Am I missing something?

Tony Gentilcore

Comment 24 2011-03-01 15:00:28 PST

(In reply to comment #23) > (From update of attachment 84296 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84296&action=review > > > LayoutTests/editing/execCommand/find-after-replace.html:7 > > +<body onload="runTest()"> > > <script> > > if (window.layoutTestController) > > layoutTestController.dumpEditingCallbacks(); > > </script> > > <p>This tests find and replace inside an editable iframe. You should see 'A B A B' below. With bug 4462420, you would see 'A B B A'.</p> > > -<iframe onload="runTest()" src="../resources/contenteditable-iframe-src.html"></iframe> > > +<iframe src="../resources/contenteditable-iframe-src.html"></iframe> > > This can’t be right. It’s possible the top level document will finish loading before its subframes, right? I think we need to make these tests run when both have loaded. Am I missing something? A loading subframe will block the window's onload event (see FrameLoader::checkCompleted). The case you are probably thinking of is that if the parser never yields throughout the course of parsing, the onload event for the body will fire before the onload event for the iframe. However, in this case the iframe doesn't have an onload event so it doesn't matter.

WebKit Commit Bot

Comment 25 2011-03-01 21:39:38 PST

Comment on attachment 84296 [details] Patch adding more missing new expected files Rejecting attachment 84296 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build'..." exit_code: 2 Last 500 characters of output: ork/Resources/pbxcp -exclude .DS_Store -exclude CVS -exclude .svn -strip-debug-symbols -resolve-src-symlinks /mnt/git/webkit-commit-queue/WebKitBuild/Release/WebProcess.app /mnt/git/webkit-commit-queue/WebKitBuild/Release/WebKit2.framework ** BUILD FAILED ** The following build commands failed: WebKit2: PhaseScriptExecution "Check For Inappropriate Files In Framework" /mnt/git/webkit-commit-queue/WebKitBuild/WebKit2.build/Release/WebKit2.build/Script-5DF408D1131DDBEC00130071.sh (1 failure) Full output: http://queues.webkit.org/results/8071805

Johnny(Jianning) Ding

Comment 26 2011-03-06 23:57:35 PST

Comment on attachment 84296 [details] Patch adding more missing new expected files The previous failure in commit-queue looked not related with this patch, try to re-land again.

WebKit Commit Bot

Comment 27 2011-03-07 00:20:14 PST

Comment on attachment 84296 [details] Patch adding more missing new expected files Clearing flags on attachment: 84296 Committed r80456: <http://trac.webkit.org/changeset/80456>

WebKit Commit Bot

Comment 28 2011-03-07 00:20:21 PST

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.