Bug 54942 - In layout tests, iframe's onload handler should not use script function defined after the iframe
Summary: In layout tests, iframe's onload handler should not use script function defin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 54355
  Show dependency treegraph
 
Reported: 2011-02-22 00:37 PST by Xianzhu Wang
Modified: 2011-03-07 00:20 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 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.
Comment 1 Tony Gentilcore 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?
Comment 2 Xianzhu Wang 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.
Comment 3 Xianzhu Wang 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.
Comment 4 Xianzhu Wang 2011-02-24 01:21:53 PST
Created attachment 83617 [details]
patch (only applies to this issue)
Comment 5 Eric Seidel (no email) 2011-02-24 02:25:41 PST
Comment on attachment 83617 [details]
patch (only applies to this issue)

OK.
Comment 6 Tony Gentilcore 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.
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2011-02-24 14:41:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2011-02-24 16:07:45 PST
http://trac.webkit.org/changeset/79625 might have broken GTK Linux 64-bit Debug
Comment 10 Xianzhu Wang 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.
Comment 11 Xianzhu Wang 2011-02-25 03:52:22 PST
Created attachment 83790 [details]
path to fix more tests
Comment 12 Tony Gentilcore 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?
Comment 13 Xianzhu Wang 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.
Comment 14 Xianzhu Wang 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.
Comment 15 Xianzhu Wang 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.
Comment 16 Tony Gentilcore 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.
Comment 17 WebKit Commit Bot 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
Comment 18 Xianzhu Wang 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.
Comment 19 WebKit Review Bot 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.
Comment 20 Johnny(Jianning) Ding 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.
Comment 21 WebKit Commit Bot 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
Comment 22 Xianzhu Wang 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.
Comment 23 Darin Adler 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?
Comment 24 Tony Gentilcore 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.
Comment 25 WebKit Commit Bot 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
Comment 26 Johnny(Jianning) Ding 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.
Comment 27 WebKit Commit Bot 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>
Comment 28 WebKit Commit Bot 2011-03-07 00:20:21 PST
All reviewed patches have been landed.  Closing bug.