Bug 65781 – [NRWT] REGRESSION: Local loader tests are failing on machines that lost /tmp/LayoutTests symlink

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

Will look today.

[Dirk Pranke]

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

[Ryosuke Niwa]

(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.

[Alexey Proskuryakov]

> 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.

[Jarred Nicholls]

(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.

[Ryosuke Niwa]

(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.

[Eric Seidel (no email)]

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.

[Alexey Proskuryakov]

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

[Jarred Nicholls]

(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.

[Ryosuke Niwa]

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?

[Jarred Nicholls]

(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.

[Eric Seidel (no email)]

I think this is just a dupe of bug 64135.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 106824 [details]
Patch

[Alexey Proskuryakov]

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.

[Jarred Nicholls]

@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 :)

[Eric Seidel (no email)]

(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.  :)

[Alexey Proskuryakov]

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

[Ryosuke Niwa]

(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.

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

(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.

[Alexey Proskuryakov]

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.

[Jarred Nicholls]

(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.

[Jarred Nicholls]

(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.

[Eric Seidel (no email)]

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

[Ryosuke Niwa]

Comment on attachment 106824 [details]
Patch

looks sane to me.

[Alexey Proskuryakov]

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).

[Eric Seidel (no email)]

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

[Ryosuke Niwa]

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

[Eric Seidel (no email)]

I am able to reproduce the failure locally.  Investigating.

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

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. :(

[Eric Seidel (no email)]

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

[Jarred Nicholls]

(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.

[Dirk Pranke]

(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?

[Eric Seidel (no email)]

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.

[Jarred Nicholls]

(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 :)

[Eric Seidel (no email)]

Created attachment 107711 [details]
work in progress

[Eric Seidel (no email)]

Created attachment 107740 [details]
Patch

[Alexey Proskuryakov]

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?

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

Comment on attachment 107740 [details]
Patch

I'd say r- given ap's & my comments.

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

(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!

[Alexey Proskuryakov]

> > 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.

[Eric Seidel (no email)]

(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.

[Alexey Proskuryakov]

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

[Eric Seidel (no email)]

Created attachment 108058 [details]
Patch

[Eric Seidel (no email)]

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

[Ryosuke Niwa]

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.

[WebKit Review Bot]

Comment on attachment 108058 [details]
Patch

Clearing flags on attachment: 108058

Committed r95588: <http://trac.webkit.org/changeset/95588>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.