Bug 65781 - [NRWT] REGRESSION: Local loader tests are failing on machines that lost /tmp/LayoutTests symlink
Summary: [NRWT] REGRESSION: Local loader tests are failing on machines that lost /tmp/...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on: 64001 64135 67953 67954
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-05 11:26 PDT by Ryosuke Niwa
Modified: 2011-09-20 17:47 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.18 KB, patch)
2011-09-08 18:18 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
work in progress (4.81 KB, patch)
2011-09-16 13:05 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2011-09-16 16:00 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (7.53 KB, patch)
2011-09-20 15:03 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-08-05 11:26:13 PDT
It seems like there have been some loader regressions.  The following tests are failing on Snow Leopard very frequently:
fast/loader/local-CSS-from-local.html
fast/loader/local-JavaScript-from-local.html
fast/loader/local-image-from-local.html
http/tests/cookies/third-party-cookie-relaxing.html
http/tests/security/contentSecurityPolicy/media-src-allowed.html
http/tests/security/local-user-CSS-from-remote.html
Comment 2 Alexey Proskuryakov 2011-09-07 11:21:08 PDT
> 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.
Comment 3 Alexey Proskuryakov 2011-09-07 11:27:04 PDT
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.
Comment 4 Eric Seidel (no email) 2011-09-07 11:27:29 PDT
Will look today.
Comment 5 Dirk Pranke 2011-09-07 11:31:54 PDT
presumably this is related to bug 64135? I thought we were leaning to fixing this in DRT, not in NRWT.
Comment 6 Ryosuke Niwa 2011-09-07 11:37:33 PDT
(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.
Comment 7 Alexey Proskuryakov 2011-09-07 11:49:35 PDT
> 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.
Comment 8 Jarred Nicholls 2011-09-07 12:04:01 PDT
(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.
Comment 9 Ryosuke Niwa 2011-09-07 12:07:02 PDT
(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.
Comment 10 Eric Seidel (no email) 2011-09-07 16:13:17 PDT
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.
Comment 11 Alexey Proskuryakov 2011-09-07 16:50:31 PDT
I'm myself puzzled about fast/loader tests, but it seems necessary for the http one.
Comment 12 Jarred Nicholls 2011-09-07 16:52:44 PDT
(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.
Comment 13 Ryosuke Niwa 2011-09-08 00:55:12 PDT
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?
Comment 14 Jarred Nicholls 2011-09-08 05:13:52 PDT
(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.
Comment 15 Eric Seidel (no email) 2011-09-08 17:35:11 PDT
I think this is just a dupe of bug 64135.
Comment 16 Eric Seidel (no email) 2011-09-08 18:05:45 PDT
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.
Comment 17 Eric Seidel (no email) 2011-09-08 18:18:19 PDT
Created attachment 106824 [details]
Patch
Comment 18 Alexey Proskuryakov 2011-09-08 19:06:38 PDT
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.
Comment 19 Jarred Nicholls 2011-09-08 19:48:09 PDT
@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 :)
Comment 20 Eric Seidel (no email) 2011-09-08 20:43:16 PDT
(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.  :)
Comment 21 Alexey Proskuryakov 2011-09-08 21:03:23 PDT
We try to make them work in a browser though - that's quite helpful in many respects.
Comment 22 Ryosuke Niwa 2011-09-08 21:16:55 PDT
(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.
Comment 23 Alexey Proskuryakov 2011-09-08 22:40:14 PDT
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.
Comment 24 Ryosuke Niwa 2011-09-08 22:43:54 PDT
(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.
Comment 25 Alexey Proskuryakov 2011-09-08 23:02:19 PDT
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.
Comment 26 Jarred Nicholls 2011-09-09 03:31:28 PDT
(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.
Comment 27 Jarred Nicholls 2011-09-09 04:42:22 PDT
(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.
Comment 28 Eric Seidel (no email) 2011-09-12 12:42:44 PDT
I'm hitting this on my personal machine too.  I believe this is a good solution to the problem.  Can haz review?
Comment 29 Ryosuke Niwa 2011-09-12 13:19:51 PDT
Comment on attachment 106824 [details]
Patch

looks sane to me.
Comment 30 Alexey Proskuryakov 2011-09-12 13:22:52 PDT
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).
Comment 31 Eric Seidel (no email) 2011-09-12 13:53:26 PDT
Committed r94976: <http://trac.webkit.org/changeset/94976>
Comment 32 Ryosuke Niwa 2011-09-12 14:46:12 PDT
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
Comment 33 Eric Seidel (no email) 2011-09-15 15:30:03 PDT
I am able to reproduce the failure locally.  Investigating.
Comment 34 Eric Seidel (no email) 2011-09-15 18:06:43 PDT
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. :)
Comment 35 Eric Seidel (no email) 2011-09-16 12:25:52 PDT
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. :(
Comment 36 Eric Seidel (no email) 2011-09-16 12:33:45 PDT
Hm... I'm starting to like the LOCAL_RESOURCE_ROOT environment variable option.  Basically an absolute path to the LayoutTests directory.
Comment 37 Jarred Nicholls 2011-09-16 12:52:50 PDT
(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.
Comment 38 Dirk Pranke 2011-09-16 12:54:39 PDT
(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?
Comment 39 Eric Seidel (no email) 2011-09-16 12:56:54 PDT
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.
Comment 40 Jarred Nicholls 2011-09-16 12:57:51 PDT
(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 :)
Comment 41 Eric Seidel (no email) 2011-09-16 13:05:39 PDT
Created attachment 107711 [details]
work in progress
Comment 42 Eric Seidel (no email) 2011-09-16 16:00:07 PDT
Created attachment 107740 [details]
Patch
Comment 43 Alexey Proskuryakov 2011-09-16 16:43:52 PDT
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 44 Ryosuke Niwa 2011-09-16 16:57:39 PDT
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 45 Ryosuke Niwa 2011-09-16 16:58:13 PDT
Comment on attachment 107740 [details]
Patch

I'd say r- given ap's & my comments.
Comment 46 Eric Seidel (no email) 2011-09-20 14:00:58 PDT
(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.
Comment 47 Eric Seidel (no email) 2011-09-20 14:11:46 PDT
(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!
Comment 48 Alexey Proskuryakov 2011-09-20 14:26:53 PDT
> > 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.
Comment 49 Eric Seidel (no email) 2011-09-20 14:29:29 PDT
(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.
Comment 50 Alexey Proskuryakov 2011-09-20 14:41:21 PDT
It's fine with me if it's a separate pass.
Comment 51 Eric Seidel (no email) 2011-09-20 15:03:31 PDT
Created attachment 108058 [details]
Patch
Comment 52 Eric Seidel (no email) 2011-09-20 15:04:44 PDT
This has been one hell of a yak shave. :)
Comment 53 Ryosuke Niwa 2011-09-20 15:13:58 PDT
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 54 WebKit Review Bot 2011-09-20 17:47:22 PDT
Comment on attachment 108058 [details]
Patch

Clearing flags on attachment: 108058

Committed r95588: <http://trac.webkit.org/changeset/95588>
Comment 55 WebKit Review Bot 2011-09-20 17:47:29 PDT
All reviewed patches have been landed.  Closing bug.