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.
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?
> 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.
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.
Created attachment 83617 [details] patch (only applies to this issue)
Comment on attachment 83617 [details] patch (only applies to this issue) OK.
(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.
Comment on attachment 83617 [details] patch (only applies to this issue) Clearing flags on attachment: 83617 Committed r79625: <http://trac.webkit.org/changeset/79625>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/79625 might have broken GTK Linux 64-bit Debug
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.
Created attachment 83790 [details] path to fix more tests
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?
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.
(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.
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.
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.
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
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.
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.
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.
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
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.
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?
(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.
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
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.
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>