Bug 75707 - [Windows] Many css2.1 tests fail on Apple's Windows port under NRWT
Summary: [Windows] Many css2.1 tests fail on Apple's Windows port under NRWT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar, NRWT, PlatformOnly
Depends on:
Blocks: 38756
  Show dependency treegraph
 
Reported: 2012-01-06 09:10 PST by Adam Roben (:aroben)
Modified: 2013-05-28 16:20 PDT (History)
6 users (show)

See Also:


Attachments
Initial work in progress. UTF-8 encoding is not being handled properly. (3.46 KB, patch)
2013-05-27 23:02 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (4.33 KB, patch)
2013-05-28 13:03 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Updated to include export definition changes needed for clean build. (6.90 KB, patch)
2013-05-28 14:28 PDT, Brent Fulgham
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2012-01-06 09:10:49 PST
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.
Comment 1 Radar WebKit Bug Importer 2012-01-06 09:11:18 PST
<rdar://problem/10654655>
Comment 2 Adam Roben (:aroben) 2012-01-06 09:12:23 PST
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.
Comment 3 Adam Roben (:aroben) 2012-01-06 09:12:52 PST
ORWT's support was added in http://trac.webkit.org/changeset/29822
Comment 4 Eric Seidel (no email) 2012-01-06 10:34:25 PST
Wow.  I never knew these existed!  I guess they're only used for Win currently?
Comment 5 Adam Roben (:aroben) 2012-01-06 11:46:38 PST
Yes, I think only Windows uses them currently.
Comment 6 Dirk Pranke 2012-01-06 13:50:50 PST
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?
Comment 7 Adam Roben (:aroben) 2012-01-06 14:01:50 PST
(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.
Comment 8 Dirk Pranke 2012-01-06 14:07:21 PST
(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.
Comment 9 Adam Roben (:aroben) 2012-01-09 09:07:46 PST
(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.
Comment 10 Dirk Pranke 2012-01-09 10:29:40 PST
(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.
Comment 11 Ojan Vafai 2012-01-09 11:17:36 PST
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.
Comment 12 Brent Fulgham 2013-05-24 10:20:22 PDT
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.
Comment 13 Brent Fulgham 2013-05-27 23:02:38 PDT
Created attachment 203022 [details]
Initial work in progress. UTF-8 encoding is not being handled properly.
Comment 14 Darin Adler 2013-05-28 09:09:50 PDT
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.
Comment 15 Brent Fulgham 2013-05-28 13:03:49 PDT
Created attachment 203080 [details]
Patch
Comment 16 Brent Fulgham 2013-05-28 13:04:36 PDT
The attached patch permits the css2.1 tests to now pass under NRWT.
Comment 17 Build Bot 2013-05-28 13:34:55 PDT
Comment on attachment 203080 [details]
Patch

Attachment 203080 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/738123
Comment 18 Brent Fulgham 2013-05-28 14:28:27 PDT
Created attachment 203088 [details]
Updated to include export definition changes needed for clean build.
Comment 19 Darin Adler 2013-05-28 14:33:16 PDT
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.
Comment 20 Brent Fulgham 2013-05-28 16:17:55 PDT
Committed r150851: <http://trac.webkit.org/changeset/150851>
Comment 21 Brent Fulgham 2013-05-28 16:20:35 PDT
See Bug 116898 to reduce use of private WebCore methods.