Bug 65781

e.g. http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r92484%20(32034)/results.html

> fast/loader/local-CSS-from-local.html > fast/loader/local-JavaScript-from-local.html > fast/loader/local-image-from-local.html > http/tests/security/local-user-CSS-from-remote.html These fail because NRWT doesn't create a symlink from /tmp/LayoutTests to actual LayoutTests directory. It should. > http/tests/security/contentSecurityPolicy/media-src-allowed.html Not sure if that's the same case. > http/tests/cookies/third-party-cookie-relaxing.html Definitely separate, but I'm not sure if this is still happening. Let's make a separate bug if it does.

I have manually created the symlinks on apple-xserve-5 and apple-xserve-6 for now, but this really needs to be fixed in NRWT soon.

Will look today.

presumably this is related to bug 64135? I thought we were leaning to fixing this in DRT, not in NRWT.

(In reply to comment #5) > presumably this is related to bug 64135? I thought we were leaning to fixing this in DRT, not in NRWT. It'll be nice if we didn't have to create symlinks but I don't think we should be forcing each port to implement this after the fact. I think we should fix this in NRWT and then slowly migrate to DRT-based solutions in the long term.

> http/tests/cookies/third-party-cookie-relaxing.html Turns out that there's already bug 64001 for that. > presumably this is related to bug 64135? I thought we were leaning to fixing this in DRT, not in NRWT. That bug is a P2, and it has been languishing for two months. Unless someone is willing to change the tests and all DRT's very soon (and remove symlink creation from ORWT), I think that NRWT should make it easy to have green bots without manually re-creating the symlink.

(In reply to comment #7) > > presumably this is related to bug 64135? I thought we were leaning to fixing this in DRT, not in NRWT. > > That bug is a P2, and it has been languishing for two months. Unless someone is willing to change the tests and all DRT's very soon (and remove symlink creation from ORWT), I think that NRWT should make it easy to have green bots without manually re-creating the symlink. None of the tests need to be changed, but DRT implementations would need to happen. I think a stop-gap solution in NRWT is acceptable but ought to be removed when bug #67239 clears - and might as well remove from ORWT when that happens too. Qt's landed today (bug #67254) and Chromium already handles it.

(In reply to comment #8) > None of the tests need to be changed, but DRT implementations would need to happen. I think a stop-gap solution in NRWT is acceptable but ought to be removed when bug #67239 clears - and might as well remove from ORWT when that happens too. Qt's landed today (bug #67254) and Chromium already handles it. Yeah, not having to add symlinks will be really nice, and we should move towards that direction. But in the short term, we probably need this trick be implemented in NRWT.

Can you help me understand why these tests use absolute file:// paths instead of relative file paths? I'm sure there is a good reason, but I'd like to understand as that will inform my thinking on fixing this bug or bug 67239, etc.

I'm myself puzzled about fast/loader tests, but it seems necessary for the http one.

(In reply to comment #11) > I'm myself puzzled about fast/loader tests, but it seems necessary for the http one. Indeed, particularly the security tests (remote => local origin requests). See comments in bug #67239.

storage/domstorage/localstorage/storagetracker/storage-tracker-7-usage.html is failing on Snow Leopard Release bot: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r94746%20(32915)/results.html Is this failure related to this bug?

(In reply to comment #13) > storage/domstorage/localstorage/storagetracker/storage-tracker-7-usage.html is failing on Snow Leopard Release bot: > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r94746%20(32915)/results.html > > Is this failure related to this bug? Doubtful. Does that bot run the tests serially or w/ multiple workers? Those tests are meant to run serially and will be flaky or fail if ran out of order.

I think this is just a dupe of bug 64135.

I've decided the proper solution to this is to make the pathToLocalResource magically relative to the test in question. I've written the code for Mac now, if it works it's written in such a fashion as it could be run on any platform and this problem should just go away everywhere w/o us needing the symlink.

Created attachment 106824 [details] Patch

No symlink would mean that tests cannot be run in browser though? I'm not sure why most commenters are in agreement that symlink is bad in the long run.

@Eric this won't apply to Qt, but Qt can look at the loaded URL and do the same derivation; I may take the time to do it this way. @ap this would be helpful for contributors running windows and vanilla (no cygwin) windows bots running NRWT. If someone needs to test in a browser manually, they can change the path before running the test (in windows) or create the symlink themselves. Note that in Windows, they would have to change the path anyways to run the test in a browser. The only tests that really need an absolute URL are the http ones (http/security, etc.). The rest (fast/loader) can be changed to relative URLs. I think this remapping mechanism will very rarely be needed in future tests, but may come in handy; those few use cases are the whole reason pathToLocalResource exists. Overall, this is a small fish with only a couple of affected tests so we don't want to over engineer around it. But, deriving the absolute path is so simple and arguably more robust that there's no sense in not doing it. Just MHO. I don't mind symlinks, but I also don't dev in Windows :)

(In reply to comment #18) > No symlink would mean that tests cannot be run in browser though? > > I'm not sure why most commenters are in agreement that symlink is bad in the long run. You would have to create the symlink yourself. It is generally true that many LayoutTests cannot be run in a browser. :)

We try to make them work in a browser though - that's quite helpful in many respects.

(In reply to comment #21) > We try to make them work in a browser though - that's quite helpful in many respects. Right. But I think pros - less platform dependency - outweigh cons - not being able to run tests - in this case given there are only few tests that require symlink.

I don't think that it's accurate to describe it this way. You get the symlink and the ability to run the test in browser on Unix platforms, but the test still works on lesser platforms due to the pathToLocalResource() hack. This is why I'm surprised by the general sentiment about symlink somehow being a bad long term direction.

(In reply to comment #23) > I don't think that it's accurate to describe it this way. You get the symlink and the ability to run the test in browser on Unix platforms, but the test still works on lesser platforms due to the pathToLocalResource() hack. > > This is why I'm surprised by the general sentiment about symlink somehow being a bad long term direction. Mn... but you can always create a symlink to run the test manually even after this patch is landed, right? We could even add a script to do that automatically.

Yes, but this approach also means that all DRTs on Unix platforms have to implement pathToLocalResource(). So, it's more work and less convenience, all for unclear benefit. Not a tremendously big deal either way though.

(In reply to comment #25) > Yes, but this approach also means that all DRTs on Unix platforms have to implement pathToLocalResource(). So, it's more work and less convenience, all for unclear benefit. All the ports but Qt load the test URL in a similar manner and can be solved in one patch/place it seems. And, Qt's already working around it. I understand your point, but instead of implementing two fragmented solutions to the same problem (use symlink in NRWT for nix ports, and fix DRT on several other ports) it's nice to solve it in one place. The ugly cygwin registry lookups in the win port can disappear, Qt can run DRT in windows now, etc etc. > > Not a tremendously big deal either way though. Agreed.

(In reply to comment #26) > (In reply to comment #25) > The ugly cygwin registry lookups in the win port can disappear Mm, that's actually not true if building and running within a cygwin environment (assuming getcwd is returning a /cygdrive path and python returns a /cygdrive path for module.__file__). But, it might make sense for NRWT, on Windows, to run cygpath and pass the Windows style path to DRT instead.

I'm hitting this on my personal machine too. I believe this is a good solution to the problem. Can haz review?

Comment on attachment 106824 [details] Patch looks sane to me.

Comment on attachment 106824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106824&action=review r+ with comments addressed. > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:361 > + // The passed in path will be an absolute path to the layout test starting > + // at /tmp/LayoutTests. Historically we created a /tmp/LayoutTests symlink Do you want to assert that the passed path starts with that? For a future improvement, I think that this method shouldn't be making one use the weird /tmp/LayoutTests path, if everyone else still agrees that symlink is bad. > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:365 > + // but not all platforms support symlinks and dependence on such a symlink > + // made it impossible for multiple users to run the layout tests (permission conflicts). > + // Instead we use the absolute path for the test from m_testPathOrURL > + // to create an absolute path for the requested local resource. I think that the amount of history is a bit excessive here. > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:369 > + size_t resourceAsUTF8Length = JSStringGetUTF8CString(localResource, resourceAsUTF8, maxResourceLength); This is wrong - the returned value includes null bytes, so it's not length, but buffer size. As a result, you get a trailing null byte in std:string below - which doesn't cause immediate trouble because the final result is based on c_str, but is very fragile. > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:371 > + std::string resourceAsWString(resourceAsUTF8, resourceAsUTF8Length); What is W in WString? > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:376 > + std::string resolvedResource = m_testPathOrURL.substr(0, testLayoutTestsIndex) + resourceAsWString.substr(resourceLayoutTestsIndex); I'd call this resolvedResourcePath (as well as other path variables here).

Committed r94976: <http://trac.webkit.org/changeset/94976>

This patch broke a bunch of tests in http/tests/security/: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r94976%20(33025)/results.html

I am able to reproduce the failure locally. Investigating.

OK, so the code I wrote does the wrong thing for HTTP tests. Example: test: http://127.0.0.1:8000/security/frame-loading-via-document-write.html resolved resource: http://127.0.0.1:8000/security/frame-loading-via-document-write.html/LayoutTests/fast/dom/resources/abe.png Note there is no LayoutTests in the original path (like ap warned about). Will investigate further tomorrow. For now, I need to eat. :)

I'm not sure how to solve this w/o leaking information about our directory layout into DumpRenderTree. We could add an environment variable to expose some sort of LOCAL_RESOURCE_ROOT, or a flag to DRT. But neither of those quite feel right. The symlink in /tmp/ is problematic for all the reasons stated above, but that may just be the most expedient solution. :(

Hm... I'm starting to like the LOCAL_RESOURCE_ROOT environment variable option. Basically an absolute path to the LayoutTests directory.

(In reply to comment #36) > Hm... I'm starting to like the LOCAL_RESOURCE_ROOT environment variable option. Basically an absolute path to the LayoutTests directory. I think that's good, maybe prefixed DRT_ and of course set by the test harness.

(In reply to comment #36) > Hm... I'm starting to like the LOCAL_RESOURCE_ROOT environment variable option. Basically an absolute path to the LayoutTests directory. Seems reasonable. I tend to prefer command line args over env variables, generally, but I don't know if I have a specific leaning in this case. Did you consider using an arg instead?

It's possible to do an arg instead. I'm testing the code now and will post. If folks feel strongly about an arg instead of an environment variable I can change things.

(In reply to comment #37) > > I think that's good, maybe prefixed DRT_ or something more generic to WebKit as a whole (In reply to comment #38) > > Seems reasonable. I tend to prefer command line args over env variables, generally, but I don't know if I have a specific leaning in this case. Did you consider using an arg instead? Ditto, though I retracted my comment before submitting because it's six one way, half a dozen the other. env vars are easy to grab and use but the naming is a good bike shedding topic :)

Created attachment 107711 [details] work in progress

Created attachment 107740 [details] Patch

Comment on attachment 107740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107740&action=review > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:363 > + size_t utf8StringLength = JSStringGetUTF8CString(jsString, utf8Buffer, maxBufferSize); > + std::string stdString = std::string(utf8Buffer, utf8StringLength); This has the same off-by-one error as before. The string will have an extra null byte at the end. Also, you can get rid of assignment, and just construct stdString directly. > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:375 > + // This fallback approach works for non-http tests and is useful > + // in the case when we're running DRT directly from the command line. I'm still confused why we ever need this machinery for non-http tests. Can't we just modify those tests to have relative paths?

Comment on attachment 107740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107740&action=review > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:377 > + size_t indexOfRootNameStart = testPathOrURL.rfind(expectedRootName); I think we should be calling rfind(serachKey) here. > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:406 > + std::string searchKey = "/" + expectedRootName + "/"; > + size_t indexOfRootNameStart = localResourceString.rfind(searchKey); > + if (indexOfRootNameStart == std::string::npos) { > + ASSERT_NOT_REACHED(); > + return localResourceJSString; > + } > + > + size_t indexOfSeparatorAfterRootName = indexOfRootNameStart + searchKey.length() - 1; This code is duplicated in resourceRootAbsolutePath. Can we extract as a function?

Comment on attachment 107740 [details] Patch I'd say r- given ap's & my comments.

(In reply to comment #43) > (From update of attachment 107740 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107740&action=review > > > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:363 > > + size_t utf8StringLength = JSStringGetUTF8CString(jsString, utf8Buffer, maxBufferSize); > > + std::string stdString = std::string(utf8Buffer, utf8StringLength); > > This has the same off-by-one error as before. The string will have an extra null byte at the end. Fixed. > Also, you can get rid of assignment, and just construct stdString directly. Fixed. > > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:375 > > + // This fallback approach works for non-http tests and is useful > > + // in the case when we're running DRT directly from the command line. > > I'm still confused why we ever need this machinery for non-http tests. Can't we just modify those tests to have relative paths? It's unclear to me. fast/loader/local-CSS-from-local.html seems to be testing that one file:// url page can load a stylesheet from another file:// url. Unclear if it's trying to test that that URL be absolute or not, or if it could just be changed to be a relative URL and still test whatever it thinks its testing.

(In reply to comment #44) > (From update of attachment 107740 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107740&action=review > > > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:377 > > + size_t indexOfRootNameStart = testPathOrURL.rfind(expectedRootName); > > I think we should be calling rfind(serachKey) here. Fixed. > > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:406 > > + std::string searchKey = "/" + expectedRootName + "/"; > > + size_t indexOfRootNameStart = localResourceString.rfind(searchKey); > > + if (indexOfRootNameStart == std::string::npos) { > > + ASSERT_NOT_REACHED(); > > + return localResourceJSString; > > + } > > + > > + size_t indexOfSeparatorAfterRootName = indexOfRootNameStart + searchKey.length() - 1; > > This code is duplicated in resourceRootAbsolutePath. Can we extract as a function? Fixed. That's much nicer!

> > I'm still confused why we ever need this machinery for non-http tests. Can't we just modify those tests to have relative paths? > > It's unclear to me. fast/loader/local-CSS-from-local.html seems to be testing that one file:// url page can load a stylesheet from another file:// url. Unclear if it's trying to test that that URL be absolute or not, or if it could just be changed to be a relative URL and still test whatever it thinks its testing. Looking at svn logs, it seems that these tests don't care about absolute vs. relative; they only use absolute URLs because they were copy/pasted from http: counterparts.

(In reply to comment #48) > > > I'm still confused why we ever need this machinery for non-http tests. Can't we just modify those tests to have relative paths? > > > > It's unclear to me. fast/loader/local-CSS-from-local.html seems to be testing that one file:// url page can load a stylesheet from another file:// url. Unclear if it's trying to test that that URL be absolute or not, or if it could just be changed to be a relative URL and still test whatever it thinks its testing. > > Looking at svn logs, it seems that these tests don't care about absolute vs. relative; they only use absolute URLs because they were copy/pasted from http: counterparts. I'm happy to remove all callers of layoutTestController.pathToLocalResource from the file:// based tests and add an ASSERT that tests are always HTTP tests in a second pass? I think that should be separate from this bug however.

It's fine with me if it's a separate pass.

Created attachment 108058 [details] Patch

This has been one hell of a yak shave. :)

Comment on attachment 108058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108058&action=review seems sane to me. > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:409 > + } else { > + ASSERT_NOT_REACHED(); // pathToLocalResource was passed a path it doesn't know how to map. > + } Nit: curly brackets around a single line statement.

Comment on attachment 108058 [details] Patch Clearing flags on attachment: 108058 Committed r95588: <http://trac.webkit.org/changeset/95588>

All reviewed patches have been landed. Closing bug.