Diff: 7 PASS: internalFrame.contentWindow.location.host should be :0 and is. 7FAIL: internalFrame.contentWindow.location.host should be :0 but instead is .
Skipped in http://trac.webkit.org/changeset/94173
Rachel or Adam, maybe you have some insight into why this test is failing on GTK+?
I'm not quite sure why this would be happening - GTK uses KURL as-is for URL parsing as I read it. (Please correct if I'm wrong there) I'll try to scrounge up a GTK build environment so I can repro.
Interesting. If this test is using crazy unicode characters, I think we've noticed that Gtk has different unicode error handling in some cases.
The test is not using any unicode characters. All it is doing is setting the port property on location. Local builds of GTK work fine both for me and mrobinson - only the bots fail on that. I believe mrobinson is looking at that?
I was able to reproduce this failure locally (i don't recall if it was on a Release or Debug build) and reverting that change had the effect to make the test pass again. I'll try again tomorrow!
I guess I missed something but I don't see how the test can pass in the first place, with and without the change to Location.cpp :( I wonder why the failure happens only in GTK, I reverted the change to Location.cpp and the test still fails, which makes me think of a bug in the KURL parsing code. KURL::parse() is a bit scary but I'll try to hunt the possible bug down... To sum up, the bug is that setting port to 0 on a file url, hostname becomes empty string instead of ":0".
pnormand: Keep in mind that it passes for Safari, which uses KURL unmodified. Also, mrobinson and I ran it on our local machines and saw the tests pass, too. ::parse treats all consecutive digits in the port field as port number, so setting to 0 should work fine. (And setPort doesn't have any exception for port 0) Could you elaborate why you think the test shouldn't be able to pass at all?
(In reply to comment #8) > pnormand: Keep in mind that it passes for Safari, which uses KURL unmodified. Also, mrobinson and I ran it on our local machines and saw the tests pass, too. > Did you unskip it before running it? Because I noticed that if a test is skipped and I try to run it directly with run-webkit-tests it will just be ignored. This is a new behavior compared to old-run-webkit-tests, I think. > ::parse treats all consecutive digits in the port field as port number, so setting to 0 should work fine. (And setPort doesn't have any exception for port 0) > > Could you elaborate why you think the test shouldn't be able to pass at all? the bug is that setting port to 0 on a file url, hostname becomes empty string instead of ":0". Location::host() returns: return url.hasPort() ? url.host() + ":" + String::number(url.port()) : url.host(); In the location-port.html test, when port is set to 0, it is expected for host to become ":0": in checkTest3(): internalFrame.contentWindow.location.port = 0; in checkTest4(): shouldBe('internalFrame.contentWindow.location.host', ':0'); But this is not what I observe locally :/ Oh, well. I guess I'm just becoming crazy ;-)
Created attachment 111947 [details] Patch This seems to be an issue with libSoup API. If port is 0, it is not explicitly specified in SoupURI. <http://maemo.org/api_refs/5.0/5.0-final/libsoup-2.4/libsoup-24-SoupURI.html#soup-uri-set-port> Hence adding an explicit check for the port before updating ResourceRequest with SoupMessage fixes this issue. I have attached the patch for review.
Attachment 111947 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:95: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 111957 [details] Patch after fixing style error Fixed the style error. Ideally, removing following statement should also fix this issue: m_url = soupURIToKURL(soup_message_get_uri(soupMessage)); I am not sure if soupURIToKURL is required, as m_url is already set in ResourceRequest. Any thoughts?
Comment on attachment 111957 [details] Patch after fixing style error I think it would be better to handle this in soupURIToKURL by looking up the port part of the SoupURI nad setting it on the new KURL.
(In reply to comment #13) > (From update of attachment 111957 [details]) > I think it would be better to handle this in soupURIToKURL by looking up the port part of the SoupURI nad setting it on the new KURL. The port part of SoupURI is by default 0, i.e. even if a port is not specified. For example, consider following two URLs: 1. file:///:0/home/code/WebKit/LayoutTests/fast/loader/location-port.html 2. file:///home/code/WebKit/LayoutTests/fast/loader/location-port.html In both the cases soup_uri_to_string would return URL #2 and port as 0. Whereas, for case #1 KURL will return port as 0 and KURL::hasPort would return true and for case #2 port is 0 and KURL::hasPort returns false. Thus I don't think we can figure out if a port is set to 0 from SoupURI. So if we try to handle it in soupURIToKURL, test case #2 fails in location-port.html i.e. shouldBe('internalFrame.contentWindow.location.port == ""', true); As mentioned earlier, we should not be setting m_url again in ResourceRequest::updateFromSoupMessage as it is already filled. It is ok to do so in ResourceResponse.
Comment on attachment 111957 [details] Patch after fixing style error View in context: https://bugs.webkit.org/attachment.cgi?id=111957&action=review This is an okay work-around for now, but the real fix should be in libsoup. > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:97 > + bool hasPort = m_url.hasPort(); > + unsigned short port = m_url.port(); > m_url = soupURIToKURL(soup_message_get_uri(soupMessage)); > > + // If port is 0, SoupURI does not have an explicitly specified port. > + // By default the port in KURL is 0, hence hasPort is used to determine if > + // a port was explicitly specified. > + if (hasPort && !port) > + m_url.setPort(port); > + I think it would be clearer here to do: bool shouldPortBeResetToZero = m_url.hasPort() && !m_url.port(); m_url = soupURIToKURL(soup_message_get_uri(soupMessage)); // SoupURI cannot differeniate between an explicitly specified port 0 and no port specified. if (shouldPortBeResetToZero) m_url.setPort(0);
CC'd Dan, who might hvae another opinion on the matter.
(In reply to comment #15) > This is an okay work-around for now, but the real fix should be in libsoup. It would be possible to patch SoupURI so that it remembered when a 0 was explicitly specified vs when it's using it to mean "unspecified port". But keep in mind that no real-world URI would ever use port 0, since it's not possible to set up a listening server on that port since bind() treats 0 as meaning "pick a random port". So the code is really only going to get used for this one test case.
(In reply to comment #17) > But keep in mind that no real-world URI would ever use port 0, since it's not possible to set up a listening server on that port since bind() treats 0 as meaning "pick a random port". So the code is really only going to get used for this one test case. Okay. Seems fine to keep this work-around in WebKit then, since it's not really useful to the rest of the libsoup users in the world.
Created attachment 113444 [details] Updated patch
Comment on attachment 113444 [details] Updated patch Clearing flags on attachment: 113444 Committed r99157: <http://trac.webkit.org/changeset/99157>
All reviewed patches have been landed. Closing bug.