Bug 16853 – http/tests/loading has different results when run with run-webkit-tests -1

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Holger Freyther]

The Gmail assert on load issue from bug #15765 made me aware of this. The tests in http/tests/loading enable the dumpFrameLoadCallbacks feature. This means that the result of one test depends on which test ran before. Some tests have a line that a certain frame will be closed that was created in the previous test case. This makes adding new tests to the directory more difficult.

[Holger Freyther]

Created attachment 18407 [details]
Run tests in loading/ as if they were invoked with -1

Run loading/ tests "singly".

[Holger Freyther]

This issue blocks bug #1576.

[Darin Adler]

Comment on attachment 18407 [details]
Run tests in loading/ as if they were invoked with -1

This is going to make these tests run a lot slower. Is there any other solution?

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> This issue blocks bug #1576.

Bug 15765

[Alexey Proskuryakov]

I think we can probably fix this by modifying the guilty tests - no need to make DRT 100% fool-proof if we can work around its deficiencies in tests themselves.

Perhaps a simple window.stop() would help here.

[Holger Freyther]

Yes, we talk about one second more and gain determinism. Closing the DRT at every test will happen for tests in http/tests/loading and webarchive/loading together these are  

ls -w1 LayoutTests/http/tests/loading/*.html LayoutTests/webarchive/loading/*.html | wc -l
7
 

[Holger Freyther]

(In reply to comment #5)
> I think we can probably fix this by modifying the guilty tests - no need to
> make DRT 100% fool-proof if we can work around its deficiencies in tests
> themselves.

We get run-webkit-tests 100% fool-proof regarding this test and pay one second.


> Perhaps a simple window.stop() would help here.
Yes we will have to make sure to replace the mainFrame with something like about:blank before going on to the next test. The downside is that 2/3 of the new -expected.txt will come from the tear down of the test which is kind of ugly as well. 

[Alexey Proskuryakov]

This problem is not limited to http/tests/loading and webarchive/loading - I've just fixed something similar in fast/loader <http://trac.webkit.org/projects/webkit/changeset/29340>. I think fast/dom may have some tricky loading cases, too.

> The downside is that 2/3 of the new -expected.txt will come from the tear down of
> the test which is kind of ugly as well. 

Indeed. Perhaps it would be possible to disable delegate call logging when we no longer need it?

[Holger Freyther]

(In reply to comment #8)
> This problem is not limited to http/tests/loading and webarchive/loading - I've
> just fixed something similar in fast/loader
> <http://trac.webkit.org/projects/webkit/changeset/29340>. I think fast/dom may
> have some tricky loading cases, too.

You are wrong, at least my problem is limited to the dumping of FrameLoaderClient callbacks and this applies only to paths containing "loading/", which applies only to the above two directories and tests calling layoutTestController.dumpFrameLoadCallbacks (which is one test in a loading/ directory).

What I didn't do was to compare the results of "run-webkit-tests -1" to "run-webkit-tests", I only compared http/tests/loading because I want to add a test case there and refuse to pick a file name that will hide this issue (which will break on filesystems giving a listing in random order anyway, e.g. reiser4).


> Indeed. Perhaps it would be possible to disable delegate call logging when we
> no longer need it?

I sincerly believe that my proposed fix is the proper one. It doesn't artificially change the tests, it keeps them simple and clear, and is removing dead code from DRT. The downside is that we spend some more time on these 7 tests.

On the other hand we can try to change the tests and DRT. This will make the tests itself more complex, e.g. adding a tear down phase, will add more complexity to DRT. Which includes one command to make the DRT blind. DRT has to wait until the tear down was completed, has to verify that all subframes are gone and this has to work for Mac,Win,Gtk+ and Qt. I don't think the added complexity to add a test-case is helping us in anyway.

[Alexey Proskuryakov]

(In reply to comment #9)
> You are wrong, at least my problem is limited to the dumping of
> FrameLoaderClient callbacks and this applies only to paths containing
> "loading/",

Please do check the referenced patch - it's basically the same problem. Something in a test was changing DRT state in a way that affected following tests. We've seen similar problems (not always related to loading) many times before, there's really not much special about FrameLoaderClient callbacks that warrants changing tracks now.

Of course, always running the whole test suite with -1 is not an option.

[Holger Freyther]

(In reply to comment #10)
> Please do check the referenced patch - it's basically the same problem.
> Something in a test was changing DRT state in a way that affected following
> tests. We've seen similar problems (not always related to loading) many times
> before, there's really not much special about FrameLoaderClient callbacks that
> warrants changing tracks now.
> 
> Of course, always running the whole test suite with -1 is not an option.
> 

Again my short term goal is to get bug #15765 fixed and this bug is blocking it.

We have two options (at least):
   - Change the title and make it a general (e.g. "run-webkit-tests -1 gives other results than run-webkit-tests"). This means bug #15765 will not be fixed any time soon.

   - Run "run-webkit-tests -1" and try to find classes of failures, e.g. dumpFrameLoadCallbacks would be one class and file one bug report for each class as "run-webkit-tests -1" is not an option (with having one meta bug).

Sure we have to keep the big picture of DRT and run-webkit-tests interaction in mind but I think instead of hand waving and questioning the whole interaction let us fix the issues case by case. This would be done case by case.

[Holger Freyther]

Alternatively I can add a bit of complexity to the Qt version of the DRT to workaround this issue, this needs someone to do something similar for mac/windows then. I hope to post a patch tomorrow.

[Holger Freyther]

Created attachment 18429 [details]
Change Qt's DRT to throw the view away before running a loading/ test

This is a proof of concept. When throwing the View aways before we run a test from the loading directory we get deterministic results. Regarding the speed it is a lot faster than restarting the DRT. For mac/win someone could try if the speed can be imrpvoed further when about:blank is loaded instead of the view recreation.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> I sincerly believe that my proposed fix is the proper one. It doesn't
> artificially change the tests, it keeps them simple and clear, and is removing
> dead code from DRT. The downside is that we spend some more time on these 7
> tests.

Generally speaking, tests should be independent.  They should not depend on a previous test, and they should not affect a later test.  If either of these two conditions occur, either the test is broken or the test tool is broken, and one or both should be fixed.

It should also be possible to run tests in a completely random order and still have them all pass.  

At minimum, if a work-around is committed to run these 7 tests singly, then a follow-up bug should be filed to fix the tests and/or DRT so that they're independent again.  Otherwise, the issue(s) should be fixed before the patch lands.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> It should also be possible to run tests in a completely random order and still
> have them all pass.  

Filed Bug 16869.

[Holger Freyther]

Created attachment 18438 [details]
Run tests in loading/ as if they were invoked with -1 unless a patched DRT exists

This combines the previous patch with attachtment from yesterday:

-Run the tests signly unless a patched DRT exists. With the attachment from yesterday only the Qt DRT is capable of it. I see this like the TemporaryLinkStubs someone will make the Mac, Win and Gtk+ work but the issue should not be blocked due this.

[Darin Adler]

Comment on attachment 18438 [details]
Run tests in loading/ as if they were invoked with -1 unless a patched DRT exists

r=me, even though I am worried about slowing down regression tests.

+        postProcessOneTest($old_base, 1) unless isQt();

I think the fact that isQt is a list of DumpRenderTree implementations that support a new mode is quite unclear here. Maybe there's a better way of writing this?

+    if ($test =~ /loading\//) {

This seems like it will fire for any directory with a suffix of "loading" -- maybe the expression should be more specific?

+        postProcessOneTest($old_base, 1) unless isQt();

Would be nicer to say "true" rather than "1" here, and even nicer to have a name rather than just a magic value -- maybe a different function name for this?

+    my ($base,$forceClose) = @_;

Missing space after comma.

+    if (($count + 1) % $testsPerDumpTool == 0 || $count == $#tests || $forceClose == 1) {

No need for "== 1" here. I think it makes things more confusing.

[Alexey Proskuryakov]

Having more and more directories with obscure special behavior makes me quite sad - it's already pretty hard to keep track of those. Especially given that there is nothing special about these particular directories in this regard.

[Beth Dakin]

I landed http://bugs.webkit.org/show_bug.cgi?id=15765 a bit hastily without noticing this bug. I suppose that makes this patch a bit more pressing unless we want to roll out http://bugs.webkit.org/show_bug.cgi?id=15765 for now.

[Beth Dakin]

Sorry! I was very eager to get Gmail working again after finding a bad regression with Kevin.

[Beth Dakin]

I papered over the problem that I created by checking in new (incorrect) results with http://trac.webkit.org/projects/webkit/changeset/29659

This is by NO MEANS to suggest that we should not address the real problem, or to suggest that the real problem does not have real priority. It's just that this problem has probably existed for a while and should not block Gmail working in Debug builds. I also did not want to leave the tests failing for the time being, because that could slow productivity in other areas of work while this issue is hashed out.

[Holger Freyther]

I have talked with Ap and Darin on irc and we agreed to consider the following:

The failing things in the test result is the output of "willCloseFrame" for subframes and maybe "windowObjectsCleared". For the current test cases willCloseFrame/windowObjectsCleared is of no interest so we are going to remove the output.

For the recently added gmail-assert test we do actually close a frame on load but we have other ways to find out if the iframe got closed.

I plan to work on it on Sunday/Monday.

[Holger Freyther]

Created attachment 18722 [details]
Do not output willCloseFrame and didClearWindowObject

As proposed on IRC do not output willCloseFrame and didClearWindowObject. The loading tests will now have the same result regardless of how we run the tests (--random, --reverse, -1).

[Darin Adler]

Comment on attachment 18722 [details]
Do not output willCloseFrame and didClearWindowObject

Seems like an OK solution for the short term.

Long term it would be nice to resolve the underlying problem too and maybe log these again.

r=me

[Holger Freyther]

Landed the patch in r30014. Should we leave this bug open until the DRT can easily load about:blank and wait for it to be loaded and we can revert this patch?

[Mark Rowe (bdash)]

No, a separate bug should be used to track any other necessary fixes and this should be closed.