Bug 75707 – [Windows] Many css2.1 tests fail on Apple's Windows port under NRWT

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Roben (:aroben)]

To reproduce (after working around bug 64468):

1. new-run-webkit-tests css2.1

Many tests fail. Compare this to "old-run-webkit-tests css2.1", where all the tests pass.

[Radar WebKit Bug Importer]

<rdar://problem/10654655>

[Adam Roben (:aroben)]

It looks like NRWT doesn't support epilogues/prologues like ORWT does, which we use to set up custom font fallback for tests in the css2.1 and svg/W3C-SVG-1.1 directories.

[Adam Roben (:aroben)]

ORWT's support was added in http://trac.webkit.org/changeset/29822

[Eric Seidel (no email)]

Wow.  I never knew these existed!  I guess they're only used for Win currently?

[Adam Roben (:aroben)]

Yes, I think only Windows uses them currently.

[Dirk Pranke]

Hm. This is a problematic feature in the face of parallelism.

How would this be supposed to work with multiple DRTs running at the same time? Does the persistent user style sheet affect any DRT running on the machine, or is it only in memory? I.e., how do I keep this from affecting DRTs running tests in other directories?

I'm actually working on a slightly different approach to this sort of thing where NRWT will change the command line flags passed to the process on a per-directory configurable basis (as part of getting rid of the chromium GPU configuration).

Could we maybe do something like that instead where every test would be told to do something different before/after/during the test but it was in-memory?

Failing that, given that this seems to only be used on the Windows port at the moment, I suggest we hack this into the windows Driver.run_test() to detect when it's entering and leaving directories with these files, and fix the windows # of workers at 1 until we can figure out a better way to get this sort of functionality.

WDYT?

[Adam Roben (:aroben)]

(In reply to comment #6)
> Hm. This is a problematic feature in the face of parallelism.
> 
> How would this be supposed to work with multiple DRTs running at the same time? Does the persistent user style sheet affect any DRT running on the machine, or is it only in memory? I.e., how do I keep this from affecting DRTs running tests in other directories?

Right now I think it gets persisted out to disk with the rest of DRT's preferences, but I don't think that is a requirement. In fact, it's probably an accident that DRT's preferences get written to disk. So I don't think this presents a problem for concurrent testing, as long as we stop writing out preferences to disk.

> Could we maybe do something like that instead where every test would be told to do something different before/after/during the test but it was in-memory?

Perhaps. Presumably that would require DRT changes. I also wonder if it would slow things down, since we'd be loading 3 files (prologue + test + epilogue) for every css2.1 test.

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #6)
> > Hm. This is a problematic feature in the face of parallelism.
> > 
> > How would this be supposed to work with multiple DRTs running at the same time? Does the persistent user style sheet affect any DRT running on the machine, or is it only in memory? I.e., how do I keep this from affecting DRTs running tests in other directories?
> 
> Right now I think it gets persisted out to disk with the rest of DRT's preferences, but I don't think that is a requirement. In fact, it's probably an accident that DRT's preferences get written to disk. So I don't think this presents a problem for concurrent testing, as long as we stop writing out preferences to disk.
> 
> > Could we maybe do something like that instead where every test would be told to do something different before/after/during the test but it was in-memory?
> 
> Perhaps. Presumably that would require DRT changes. I also wonder if it would slow things down, since we'd be loading 3 files (prologue + test + epilogue) for every css2.1 test.

The general model I have in mind is that the command lines would potentially change per directory, and so when DRT was handed a test in a new directory, it would get restarted with the change. 

So, for something like this we could add a --use-user-style=$foo flag (or some equivalent) for this directory, and any test run in that directory would get handed that flag. We could add a more general --prologue and --epilogue flags to run those files on startup and shutdown (and the epilogue might not even be needed if the scripts weren't changing persistent settings).

Obviously this would require some DRT changes, but it doesn't seem like they'd be too bad.

[Adam Roben (:aroben)]

(In reply to comment #8)
> The general model I have in mind is that the command lines would potentially change per directory, and so when DRT was handed a test in a new directory, it would get restarted with the change. 

I wonder what effect this would have on performance for directories that don't contain many tests? I guess we could optimize by comparing the old and new command lines and just reusing the existing DRT if they are the same.

[Dirk Pranke]

(In reply to comment #9)
> (In reply to comment #8)
> > The general model I have in mind is that the command lines would potentially change per directory, and so when DRT was handed a test in a new directory, it would get restarted with the change. 
> 
> I wonder what effect this would have on performance for directories that don't contain many tests? I guess we could optimize by comparing the old and new command lines and just reusing the existing DRT if they are the same.

Right, that was what I had in mind.

[Ojan Vafai]

Alternately, we could just hard-code this logic into Apple's WIN DRT and save ourselves a lot of work of trying to make this general.

[Brent Fulgham]

It appears that the only cases where the prologue/epilogue feature is being used is to support the setting of a persistent user style sheet.  No other ports appear to be using this feature.

Given that roughly five years have passed since this mechanism was created with no additional use cases, I think it makes sense to build this into Window's DRT.

[Brent Fulgham]

Created attachment 203022 [details]
Initial work in progress. UTF-8 encoding is not being handled properly.

[Darin Adler]

Comment on attachment 203022 [details]
Initial work in progress. UTF-8 encoding is not being handled properly.

View in context: https://bugs.webkit.org/attachment.cgi?id=203022&action=review

r=me if you fix the storage leak

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:962
> +String createPathIfURLHasLogueDirectory(const CFURLRef& url)

Crazy use of the word “logue” here to mean prologue or epilogue!?

Type here should just be CFURLRef, not const CFURLRef&.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:967
> +        return "";

empytString() is the more efficient alternative to "" when returning a String.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:971
> +    UInt8* buffer = new UInt8[length];

This buffer leaks. One fix is to use an OwnArrayPtr. Another is to use Vector<UInt8>.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:973
> +    if (-1 == length)

Normally we don’t do the “constant on the left” thing like this. Also, it’s impossible for CFURLGetBytes to return length if we are passing it a length we got from calling it the first time, so this check is guaranteed dead code.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:978
> +    String loguePath(buffer + pathRange.location, pathRange.length);
> +
> +    loguePath = WebCore::pathByAppendingComponent(loguePath, "resources/");

Seems like we could do this with CFURL and CFString functions rather than dropping into WebCore internal types to do it.

CFURLCreateCopyAppendingPathComponent and CFURLGetFileSystemRepresentation could be used to do the job this function does without involving the CFURLGetByte functions.

But I suppose this is OK as is.

[Brent Fulgham]

Created attachment 203080 [details]
Patch

[Brent Fulgham]

The attached patch permits the css2.1 tests to now pass under NRWT.

[Build Bot]

Comment on attachment 203080 [details]
Patch

Attachment 203080 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/738123

[Brent Fulgham]

Created attachment 203088 [details]
Updated to include export definition changes needed for clean build.

[Darin Adler]

Comment on attachment 203088 [details]
Updated to include export definition changes needed for clean build.

View in context: https://bugs.webkit.org/attachment.cgi?id=203088&action=review

I’d like to see these patches make less use of internal WebCore data structures and functions.

> Source/WebKit/ChangeLog:9
> +        * WebKit.vcxproj/WebKitExportGenerator/WebKitExports.def.in:
> +        Export the WebCore::directoryName method for use by DRT.

I don’t think this is a step in the right direction. Ideally DumpRenderTree is “outside WebKit” code, and we should not be exporting more functions for it to use. Especially functions that are just file system helpers that have no special WebKit magic in them.

[Brent Fulgham]

Committed r150851: <http://trac.webkit.org/changeset/150851>

[Brent Fulgham]

See Bug 116898 to reduce use of private WebCore methods.