Bug 30721 – [GTK] DRT reports error in the wrong test

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alejandro G. Castro]

DRT is closing the result output before doing the complete finalization of the test, basically the reset of the state before launching other tests is done after adding the EOF. In some situations this code is causing a crash and the crash is reported in the next test. We have to move the last flush after all the operations are finished. For instance this crash does not come from this test it comes from the opaque-base-url.html which was executed before:

http://build.webkit.org/results/GTK Linux 64-bit Debug/r49969 (490)/fast/loader/plain-text-document-stderr.txt

[Alejandro G. Castro]

(In reply to comment #0)
>
> [...]
> 
> http://build.webkit.org/results/GTK Linux 64-bit Debug/r49969
> (490)/fast/loader/plain-text-document-stderr.txt

Better link:

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r49969%20(490)/fast/loader/plain-text-document-stderr.txt

[Alejandro G. Castro]

Created attachment 41855 [details]
Proposed patch moving the flush to the end of the test

[Holger Freyther]

Hmm. If dump() is not the right place to dump() maybe we call dump() at the wrong time?

[Alejandro G. Castro]

(In reply to comment #3)
> Hmm. If dump() is not the right place to dump() maybe we call dump() at the
> wrong time?

The problem is that after dump we do some "cleaning" of the webview state, and that cleaning is not part of the test but in some cases it causes a crash, currently it is happening if you check gtk+ debug bots; and because the EOF was already dumped the crash is reported in the next test. That code should not crash (actually I'm trying to check the problem in this case bug 30724) but if it crashes it must report the correct test.

IMO this means that the test does not end until you stop using the webview, not after the last dump of the test. Maybe a cleaner option would be to add some support to the run tests script and the DRT to point out it is going to start checking the next test, currently it seems is just uses the results log.

[Eric Seidel (no email)]

Comment on attachment 41855 [details]
Proposed patch moving the flush to the end of the test

Seems like this should be wrapped into a little helper function with a nice name.  Leaving it bare as-is seems strange when adding it to runTests.

[Eric Seidel (no email)]

Comment on attachment 41855 [details]
Proposed patch moving the flush to the end of the test

Chromium recently had similar troubles.  I still think putting this in some nicely named function would make this much clearer.  As-is, if we ever need to modify the "test shut down" procedure, it's difficult to tell where it is or what all of the code should be moved were this ever to move again.  r- because I really think this needs to be put in a function with a nice name and called from this runTests() location.

sendPixelResultsEOF() or something even more descriptive?

[Alejandro G. Castro]

(In reply to comment #6)
> (From update of attachment 41855 [details])
> Chromium recently had similar troubles.  I still think putting this in some
> nicely named function would make this much clearer.  As-is, if we ever need to
> modify the "test shut down" procedure, it's difficult to tell where it is or
> what all of the code should be moved were this ever to move again.  r- because
> I really think this needs to be put in a function with a nice name and called
> from this runTests() location.
> 
> sendPixelResultsEOF() or something even more descriptive?

Right, refactoring the patch now ...

[Alejandro G. Castro]

Created attachment 43593 [details]
Added a function with the termination code

[Adam Barth]

style-queue successfully ran check-webkit-style on attachment 43593 [details] without any errors

[Eric Seidel (no email)]

Comment on attachment 43593 [details]
Added a function with the termination code

LGTM.

[WebKit Commit Bot]

Comment on attachment 43593 [details]
Added a function with the termination code

Clearing flags on attachment: 43593

Committed r51525: <http://trac.webkit.org/changeset/51525>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.